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

Empty string should be a valid attribute value #5501

Closed
trask opened this issue Jun 2, 2023 · 4 comments · Fixed by #5616
Closed

Empty string should be a valid attribute value #5501

trask opened this issue Jun 2, 2023 · 4 comments · Fixed by #5616
Labels
Bug Something isn't working

Comments

@trask
Copy link
Member

trask commented Jun 2, 2023

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute:

Attribute values expressing a numerical value of zero, an empty string, or an empty array are considered meaningful and MUST be stored and passed on to processors / exporters.

Empty strings are important for url.query where empty strings are important to preserve to represent urls like http://xyz/path?

The Span API javadoc currently says that the behavior for an empty string value is "undefined" (I haven't checked what the actual implementation does):

* <p>If a null or empty String {@code value} is passed in, the behavior is undefined, and hence
* strongly discouraged.

@jack-berg
Copy link
Member

It looks like the implementation ignores null values, but sets empty strings. Should update the javadoc and confirm with a test (if none exists) while we're at it.

@erharjotsingh
Copy link

/assign erharjotsingh

@erharjotsingh
Copy link

erharjotsingh commented Jun 15, 2023

hello @jack-berg @trask
Could you please summarize the issue and give me some pointers. Please assign to me and I can pick this up. Thanks!

@breedx-splk
Copy link
Contributor

It looks like the implementation ignores null values, but sets empty strings. Should update the javadoc and confirm with a test (if none exists) while we're at it.

There are tests that covers it for the SdkSpan:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants