Add SpanContextFromContext()

[XSAM]

Resolves #1251

Many places use a way to attain SpanContext, and it will panic if SpanFromContext() returns nil.

otel.SpanFromContext(ctx).SpanContext()

So, I add a new API SpanContextFromContext to replace that part without worry about panic.

[codecov]

Codecov Report

Merging #1255 into master will increase coverage by 0.0%.
The diff coverage is 87.5%.

@@          Coverage Diff           @@
##           master   #1255   +/-   ##
======================================
  Coverage    76.9%   76.9%           
======================================
  Files         122     122           
  Lines        5924    5927    +3     
======================================
+ Hits         4557    4561    +4     
  Misses       1120    1120           
+ Partials      247     246    -1     

Impacted Files	Coverage Δ
trace.go	80.0% <66.6%> (-0.6%)	⬇️
internal/trace/parent/parent.go	21.0% <100.0%> (ø)
oteltest/config.go	94.4% <100.0%> (ø)
oteltest/tracer.go	100.0% <100.0%> (ø)
propagators/trace_context.go	65.5% <100.0%> (ø)
sdk/trace/batch_span_processor.go	80.6% <0.0%> (+2.1%)	⬆️

[MrAlias]

💯

[Aneurysm9]

I'm not sure how I feel about this change. Particularly with the addition of SpanContextFromContext() it seems like just moving things around. It seems to me that the better experience is to have SpanFromContext() always return a usable span, even if it is no-op. If an instrumentor is concerned with avoiding expensive operations for non-recording or no-op spans then the span.IsRecording() method is available to them.

[Aneurysm9]

After discussing this during the SIG meeting we came to the conclusion that adding SpanContextFromContext() is useful, but that SpanFromContext() should continue to return a noopSpan{}. The Span API provides sufficient mechanisms for instrumentors to determine whether a given Span is recording and they should perform potentially expensive operations.

[XSAM]

I don't understand the meaning that SpanFromContext returns noopSpan rather than nil.

In that situation, users can only invoke IsRecording to determine whether Span is active or not, but cannot determine whether the Context has Span or not.

I think users can easily use span == nil to determine the existence of Span if SpanFromContext returns nil. Also, it is quite common that gophers check whether the interface is nil before using it.

[Aneurysm9]

I don't understand the meaning that SpanFromContext returns noopSpan rather than nil.

In that situation, users can only invoke IsRecording to determine whether Span is active or not, but cannot determine whether the Context has Span or not.

Looked at another way, what value does knowing whether the Context carries a Span provide? If it is only to know that no valid span is active and that potentially expensive calculations can be avoided, IsRecording() provides the same capability and does not require nil checking.

I think users can easily use span == nil to determine the existence of Span if SpanFromContext returns nil. Also, it is quite common that gophers check whether the interface is nil before using it.

It can be done, but it doesn't need to be done and it is a better DX to not require it be done. Also, since it has not been required to this point we should assume that there is a significant body of code out there already that doesn't perform any such check and will be susceptible to runtime errors attempting to invoke methods on nil values. This would be asking a significant remediation effort from all of our users for a questionable benefit for some of them.

[jmacd]

that SpanFromContext() should continue to return a noopSpan{}

I agree with this statement, and was going to ask if it wouldn't be safer for SpanFromContext not to return nil.