Skip to content

Empty arrays are meaningful #760

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 6, 2020
Merged

Empty arrays are meaningful #760

merged 1 commit into from
Aug 6, 2020

Conversation

@thisthat
Copy link
Member

@thisthat thisthat commented Aug 6, 2020

Changes

Stress that empty arrays must be forwarded to span processors/exporters.

@thisthat thisthat requested review from a team August 6, 2020 06:59
Oberon00
Oberon00 approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I agree. See open-telemetry/opentelemetry-java#1437 (comment).

@Oberon00
Copy link
Member

Oberon00 commented Aug 6, 2020

I think this should also apply to resource attributes and thus be moved to the common attribute spec.

@carlosalberto
Copy link
Contributor

carlosalberto commented Aug 6, 2020

This makes sense to me. Let's wait for other's feedback ;)

@carlosalberto
Copy link
Contributor

carlosalberto commented Aug 6, 2020

Btw, since Attributes are used in other places, maybe it makes sense to review and maybe updates other parts of the Specification?

@carlosalberto carlosalberto added area:api Cross language API specification issue release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory labels Aug 6, 2020
@jkwatson
Copy link
Contributor

jkwatson commented Aug 6, 2020

I think this should also apply to resource attributes and thus be moved to the common attribute spec.

Let's not muddy this PR with a discussion of Resource attributes. I recommend creating an issue or a PR to deal with that, independently.

@carlosalberto
Copy link
Contributor

carlosalberto commented Aug 6, 2020

Let's not muddy this PR with a discussion of Resource attributes

Fair enough, will create a follow up issue.

bogdandrutu
bogdandrutu reviewed Aug 6, 2020
View reviewed changes
specification/trace/api.md
attribute's value.

Attribute values expressing a numerical value of zero or an empty string are
Attribute values expressing a numerical value of zero, an empty string, or an empty array are
Copy link
Member

@bogdandrutu bogdandrutu Aug 6, 2020

Choose a reason for hiding this comment

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

Should this apply to all attributes? If yes probably we should update https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/common/common.md and remove this.

@bogdandrutu bogdandrutu mentioned this pull request Aug 6, 2020
Closed
@bogdandrutu bogdandrutu merged commit 91590d9 into open-telemetry:master Aug 6, 2020
@thisthat thisthat deleted the empty-array branch October 5, 2020 06:45
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@thisthat
Add Empty array (open-telemetry#760)
719ff0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bogdandrutu bogdandrutu bogdandrutu left review comments

@carlosalberto carlosalberto carlosalberto approved these changes

@Oberon00 Oberon00 Oberon00 approved these changes

@arminru arminru arminru approved these changes

+2 more reviewers

@jkwatson jkwatson jkwatson approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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:trace Related to the specification/trace directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@thisthat @Oberon00 @carlosalberto @jkwatson @bogdandrutu @yurishkuro @arminru