Span::setAttribute null-checking of value may be detrimental

[prydin]

This came up as an issue when working on the auto-instrumentation project.

The Span::setAttribute() method is commonly used in interceptors to copy attributes from an invocation to span attributes. This method will, by design, throw a NullPointerException if a null value is supplied. Given that many frameworks may treat certain attributes as optional, there's a chance that attributes with null values are present on the invocation objects.

The current implementation instrumenters/decorators are wrapping the calls to setAttribute() in null-checking if-statements whenever it is suspected that a null value may be supplied.

However, since the exact rules for which attributes may contain null aren't well documented for all frameworks, there's a high risk that a null-check is omitted by mistake, causing hard to find errors at runtime.

Should we implement a setAttributeOrNull() method that can be used when the presence of null values isn't known? Should we change the behavior of setAttribute()? What is the reason we don't allow null values in the first place? Could we change the behavior to make an assignment of a null value to an attribute a no-op?

[jkwatson]

Would you want the null value to propagate and end up being reported somewhere, or should it always be ignored? To rephrase: are there use-cases for null valued attributes?

[prydin]

As far as I'm concerned, a null value on an attribute is perfectly fine. But there may be other parts of the code that doesn't agree with me. :)

My particular use case is this: The auto instrumenter picks up a bunch of values from a call to some service that we're instrumenting. We pick up those values and stick them as attribute on a Span. Sometimes the values we get extract from the call are null, which is just fine in many cases.

The code may look something like this:

span.setAttribute("anOptionalAttribute", request.getSomeValueThatCanSometimesBeNull())

If we don't check for null on EVERY instance of such value propagation, we risk crashing out of our interceptor on a NullPointerException.

[jkwatson]

Our 3 options:

1. Leave the API as-is, reject null valued attributes, requiring instrumenters to filter out nulls.
2. Add an additional method on the API to allow setting possibly null-valued attributes
3. Change the API to allow null valued attributes and pass those on to exporters to deal with.

I'd love some input from other folks. @tylerbenson, @trask, @bogdandrutu , your thoughts?

[tylerbenson]

Decision from the call: We are going to update the API to allow nullable values (but not keys), and that null values will result in the call being a noop.

[arminru]

From today's SIG call:
We agree that it would make things easier for use cases like yours if we don't throw on null. We tend towards changing the existing API to accept null values (not keys) and just drop them immediately as if they were never provided, so the null value wouldn't be propagated.

[trask]

Any thoughts on what to do with empty strings? E.g. DataDog's agent also removes the attribute if passed an empty string

I'm not sure I have a preference one way or another, just wanted to raise while this is being discussed. Thx!

[jkwatson]

I think the empty string should be considered the same as a null for this change.

[Oberon00]

I'm not happy with the empty string thing, but I think this needs clarification in the spec, so I created open-telemetry/opentelemetry-specification#431 there.