Prevent NPE when deleting OpenTracing baggage

[Xyaren]

What Does This Do

Prevent NPE when deleting baggage value by setBaggageItem("key", null)

Motivation

NPE when trying to remove a baggage item.

java.lang.NullPointerException: null
	at java.base/java.util.concurrent.ConcurrentHashMap.putVal(Unknown Source)
	at java.base/java.util.concurrent.ConcurrentHashMap.put(Unknown Source)
	at datadog.trace.agent.core.DDSpanContext.setBaggageItem(DDSpanContext.java:651)
	at datadog.trace.agent.core.DDSpan.setBaggageItem(DDSpan.java:514)
	at datadog.trace.agent.core.DDSpan.setBaggageItem(DDSpan.java:50)
	at datadog.trace.instrumentation.opentracing32.OTSpan.setBaggageItem(OTSpan.java:122)
	at datadog.trace.instrumentation.opentracing32.OTSpan.setBaggageItem(OTSpan.java:17)
	at com.mycomp.tracing.OpenTracingBusinessContextStore.clearBaggageItem(OpenTracingBusinessContextStore.java:55)

Additional Notes

Contributor Checklist

• Format the title according the contribution guidelines
• Assign the type: and (comp: or inst:) labels in addition to any usefull labels
• Squash your commits prior merging or merge using GitHub's Squash and merge
• Don't use close, fix or any linking keywords when referencing an issue.
  Use solves instead, and assign the PR milestone to the issue
• Update the public documentation in case of new configuration flag or behavior

[manuel-alvarez-alvarez]

Should we protect the call when the baggageItems has not yet been initialized (in that case this will throw an unsupported operation exception)?

[Xyaren]

I think this would be unexpected. An outside actor is not aware if something is initialized or not.

[manuel-alvarez-alvarez]

Yep, you can then just check if the baggageItems != EMPTY_BAGGAGE before calling remove

Many thanks for the contribution 😄

[Xyaren]

EMPTY_BAGGAGE is just an empty map, so .remove is a noop.
Would that make a difference ?

[PerfectSlayer]

Dump question but I could not find from OpenTracing spec nor code base that null value must be supported?
Not the null should throw a NPE, it should not but I am not sure that setting a null value should remove the key-value pair... 🤔
Can you help me where you found this behavior?

Moreover, this change would required additional test for core and OpenTracing instrumentation (we can discuss this point later once we find out how the API should behave).

EDIT: I find the the OpenTracing shim from OpenTelemetry (so the implementation of OpenTracing from the OpenTelemetry standard) does skip null keys and values:
https://github.com/open-telemetry/opentelemetry-java/blob/0132d5d98b21cd4cbc2a0354a4d5bcc2867a2969/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanShim.java#L167-L178

So I would not be in favor of changing the baggage behavior, only fixing the NPE.

[Xyaren]

Right, fortunately with opentelemetry you have the option to get the Baggage using getBaggage() turn it into a builder, update it and store it again.

https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java#L75

open-telemetry/opentelemetry-java#2553

I feel this might be an unintended restriction due to the ConcurrentHashMap.

What would my alternative be to remove a baggage Item ?