opentelemetry-java icon indicating copy to clipboard operation
opentelemetry-java copied to clipboard

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

Open trask opened this issue 2 years ago • 1 comments

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 https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/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 avatar Apr 04 '22 18:04 trask

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 avatar Apr 04 '22 20:04 jkwatson