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

Officially treat @Nullable value in AttributesBuilder.put() as a no-op #4336

Open
trask opened this issue Apr 4, 2022 · 1 comment
Open
Labels
blocked:spec blocked on open or unresolved spec Feature Request Suggest an idea for this project

Comments

@trask
Copy link
Member

trask commented Apr 4, 2022

Is your feature request related to a problem? Please describe.
It is very convenient for instrumentation to call AttributesBuilder.put() with a possibly null value. Currently AttributesBuilder.put() treats this as a no-op, but this behavior is not officially documented, so we are not able to rely on it and instead must check for null value before calling put().

Describe the solution you'd like
setAttribute() value parameter to be officially @Nullable, and officially document that it treats null values as a no-op.

I think it's probably too late to change this behavior anyways, as it would probably break many instrumentations, even if it is not technically a breaking change.

Describe alternatives you've considered
Creating an internal helper method in the instrumentation repo that we use everywhere (see open-telemetry/opentelemetry-java-instrumentation#5749):

  public static <T> void set(
      AttributesBuilder attributes, AttributeKey<T> key, @Nullable T value) {
    if (value != null) {
      attributes.put(key, value);
    }
  }

My main concern with this internal helper method is that I believe it will be common for people to copy-paste the instrumentation repo's AttributeExtractors and so they will end up relying on this internal method.

@trask trask added the Feature Request Suggest an idea for this project label Apr 4, 2022
@jkwatson
Copy link
Contributor

jkwatson commented Apr 4, 2022

I assume you'd also want this on SpanBuilder.setAttribute and Span.setAttribute?

Unfortunately, we're currently adhering strictly to the spec:

Attribute values of null are not valid and attempting to set a null value is undefined behavior.

So, if we want to make the current behavior explicit, I think we'd need to update the spec.

@jkwatson jkwatson added the blocked:spec blocked on open or unresolved spec label Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:spec blocked on open or unresolved spec Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

2 participants