open-telemetry / opentelemetry-java

OpenTelemetry Java SDK
https://opentelemetry.io
Apache License 2.0
1.99k stars 826 forks source link

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

Open trask opened 2 years ago

trask commented 2 years ago

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.

jkwatson commented 2 years ago

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.