Skip to content
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

Centralize attributes definition #722

Merged
merged 8 commits into from
Jul 22, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Jul 21, 2020

Fixes #425
Fixes #446

Changes

Found Attributes defined in at least 3 different places, with different requirements. This PR tries to centralize the definition for the Attributes in a common place and share across (for the moment resources and traces).

@bogdandrutu bogdandrutu requested review from a team July 21, 2020 00:40
@bogdandrutu bogdandrutu added area:api Cross language API specification issue spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory labels Jul 21, 2020
@bogdandrutu bogdandrutu added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 21, 2020
@bogdandrutu
Copy link
Member Author

@open-telemetry/specs-approvers please review

Co-authored-by: Christian Neumüller <[email protected]>
@bogdandrutu
Copy link
Member Author

@Oberon00 thanks for catching my copy paste :)

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good to me, just some nitpicks.

specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/common/common.md Outdated Show resolved Hide resolved
specification/trace/api.md Show resolved Hide resolved
specification/overview.md Outdated Show resolved Hide resolved
`null` values within arrays MUST be preserved as-is (i.e., passed on to span
processors / exporters as `null`). If exporters do not support exporting `null`
values, they MAY replace those values by 0, `false`, or empty strings.
This is required for map/dictionary structures represented as two arrays with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the required here a MUST overriding the previous MAY? I'm not sure how to interpret this sentence

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a note that explains the rationale for both of the above together. It is meant to ban SDKs and exporters from just dropping the null values from the array (changing the index of following elements).

null values within arrays MUST be preserved as-is

Is what basically applies.

exporters [...] MAY replace those values

This is a relaxation for exporters. Notice how they MAY replace those values, but we do not allow them to drop them. So if they choose not to exercise the MAY, the "null values within arrays MUST be preserved as-is" applies.

That is how I read it, and I'm pretty sure that's how it was meant originally (CC @arminru). But it could be worded more clearly.

bogdandrutu and others added 6 commits July 22, 2020 08:54
Co-authored-by: Christian Neumüller <[email protected]>
Co-authored-by: Christian Neumüller <[email protected]>
Co-authored-by: Christian Neumüller <[email protected]>
Co-authored-by: Christian Neumüller <[email protected]>
Co-authored-by: Christian Neumüller <[email protected]>
Co-authored-by: Christian Neumüller <[email protected]>
@Oberon00
Copy link
Member

Oberon00 commented Jul 23, 2020

Actually we should add a release notes entry for this since this adds arrays as allowed resource attribute value type. @bogdandrutu

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* Centralize attributes definition

Signed-off-by: Bogdan Drutu <[email protected]>

* Update specification/common/common.md

Co-authored-by: Christian Neumüller <[email protected]>

* Update specification/common/common.md

Co-authored-by: Christian Neumüller <[email protected]>

* Update specification/common/common.md

Co-authored-by: Christian Neumüller <[email protected]>

* Update specification/common/common.md

Co-authored-by: Christian Neumüller <[email protected]>

* Update specification/common/common.md

Co-authored-by: Christian Neumüller <[email protected]>

* Update specification/common/common.md

Co-authored-by: Christian Neumüller <[email protected]>

* Update specification/overview.md

Co-authored-by: Christian Neumüller <[email protected]>

Co-authored-by: Christian Neumüller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent allowed attribute types Vague spec: Span attributes may be "numeric"
5 participants