-
Notifications
You must be signed in to change notification settings - Fork 933
Extend the set of attribute value types #4651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Copilot <[email protected]>
This reverts commit c17aec6.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
reflects that LogRecord attributes are expected to model data produced from | ||
external log APIs, which do not necessarily have the same value type | ||
restrictions as the standard attribute definition. | ||
Implementation MUST by default ensure that the exported attribute collections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is partially duplicated above on line 80. Can we have this requirement consolidated in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do it after the part where line 80 is located is stable. Right now it is hard to consolidate.
- **Status**: [Development](../document-status.md) - if it is an array of [AnyValue](#anyvalue), | ||
then apply the limit to each element of the array separately (and recursively), | ||
- **Status**: [Development](../document-status.md) - if it is a [map](#mapstring-anyvalue), | ||
then apply the limit to each value within the map separately (and recursively), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies only to the "value" of the map but not the "key" of the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. We were never applying limits for the keys. Moreover, the limit is called "attribute value limit".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
specification/common/README.md
Outdated
- **Status**: [Development](../document-status.md) - a byte array. | ||
- **Status**: [Development](../document-status.md) - an array of `AnyValue`, | ||
- **Status**: [Development](../document-status.md) - a [`map<string, AnyValue>`](#mapstring-anyvalue), | ||
- **Status**: [Development](../document-status.md) - an empty value (e.g. `null`, `undefined` in JavaScript/TypeScript, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changing to now support null
? It isn't able to be represented by the protos still, right? I know below it says to then convert to a value like 0
or false
, but why allow it for AnyValue
in the first place?
And can it change to say, "if supported by the language"? There is not a true null
or undefined
in Erlang and as this is written it sounds like there is need to support representing this rather than putting it on the user to do their own mapping to an allowed value like 0
or ""
if they don't want a string representation of the atom they used to represent the lack of a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changing to now support
null
?
It is currently supported by any
from the Logs Data Model.
More context: #3853
And can it change to say, "if supported by the language"?
Personally, I am fine with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4651 (comment)
EDIT: 6c8418e
Changes
AnyValue
type supporting complex data structures (empty value, byte arrays, heterogeneous arrays and maps).AnyValue
type.Extend attribute types
Prior-art (this PR guides towards this): #4636
Prototype: open-telemetry/opentelemetry-go#6809
Follows #4614
Related to #4602
Related proto PR: open-telemetry/opentelemetry-proto#707
https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/4485-extending-attributes-to-support-complex-values.md#how describes how languages should add support for new attribute value types.
OTEP changes
Notice that this PR has changed the strategy for https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/4485-extending-attributes-to-support-complex-values.md#api from:
to simply:
This is the agreement up to this point: #4651 (comment)
The other change in the OTEP is because of #4651 (comment).
Attribute limit updates
Towards #4487
This also proposes minimal and non-breaking additions for the attribute limits: https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/4485-extending-attributes-to-support-complex-values.md#attribute-limits.
The proposal for attribute count limit is proposed because of #4651 (comment).
The proposal for attribute value length limit seems the most logical to me.