DataDog / dd-trace-java

Datadog APM client for Java
https://docs.datadoghq.com/tracing/languages/java
Apache License 2.0
588 stars 290 forks source link

Prevent NPE when deleting OpenTracing baggage #7637

Closed Xyaren closed 2 weeks ago

Xyaren commented 2 months ago

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

Xyaren commented 2 months ago

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

https://github.com/open-telemetry/opentelemetry-java/issues/2553

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

What would my alternative be to remove a baggage Item ?

lucaspimentel commented 1 month ago

What would my alternative be to remove a baggage Item ?

Do we need one? It looks like OpenTracing didn't have a way to remove baggage:

Granted it's also not clear either what should happen when calling OpenTracing's Span.setBaggageItem with a null value.

Xyaren commented 1 month ago

I can overwrite them but not remove them ? Sounds quuestionable.

Empty strings it is then...

PerfectSlayer commented 2 weeks ago

Closing in favor of #7848