Opentelemetry-java: HttpTraceContext::extract() fails on traceparent == null

[prydin]

io.opentelemetry.trace.propagation.HttpTraceContext::extract() can be used to construct a SpanContext from HTTP headers/request attributes. This is very useful when instrumenting servlets, for example.

However, this method fails on an IllegalArgumentException when the traceparent attribute can't be located. I don't think this exception should be called, since root spans should not have this attribute set. I believe the correct behavior should just be to copy the null to the constructed SpanContext.

[prydin]

Also, catching the exception and trying to manually set the parent to null through Span.Builder::setParent() doesn't work either, since it doesn't accept null parameters.

EDIT: My bad. There's setNoParent()

[carlosalberto]

I agree on this, and I think we should try to not throw exceptions for non-fatal cases.

[DotSpy]

I will try fix it, please assign me

[DotSpy]

@pavolloffay how do u think should we return null here (https://github.com/open-telemetry/opentelemetry-java/blob/7b601b1ad08e62d3d0bfb321027fe58b95c3c3ce/api/src/main/java/io/opentelemetry/trace/propagation/HttpTraceContext.java#L113) or SpanContext.INVALID ?
Also i think that it's kinda heavy method, should i also refactor it?

[jkwatson]

I'd love to see some refactoring in there, @DotSpy !

[carlosalberto]

My advise is to keep different PRs for any refactoring (given it's not only about code style, but actual improved readability) and for the null-handling cases.

[DotSpy]

@jkwatson @carlosalberto how do u think what should i return instead of throwing:
null - but here in Java Doc it said https://github.com/open-telemetry/opentelemetry-java/blob/bd4f3737d4dd8af212c7a2066345c3f196a7a2b6/context_prop/src/main/java/io/opentelemetry/context/propagation/HttpTextFormat.java#L98
or SpanContext.INVALID

[jkwatson]

I'm not a Tracing expert, but I think INVALID seems like a great answer.

[carlosalberto]

SpanContext.INVALID definitely.

[DotSpy]

I think we can close this one because it's merged

[jkwatson]

Resolved by #782