[API] Reduce exception handling in TextMapPropagator#extract

[hosamaly]

Previous code used to throw an error from TraceParent.from_string when given a nil, which happens when the carrier is empty.

This pull request changes the implementation to return early instead. It follows the same pattern that was introduced in Baggage::TextMapPropagator in PR #835.

[robertlaurin]

Hey @hosamaly just would like to understand the issue this is meant to correct a little better.

Previous code used to throw an error from TraceParent.from_string when given a nil, which happens when the carrier is empty.

I believe we are handling nil cases in that method

opentelemetry-ruby/api/test/opentelemetry/trace/tracestate_test.rb

Lines 14 to 17 in 8a538e7

	it 'returns an empty Tracestate for nil' do
	tracestate = OpenTelemetry::Trace::Tracestate.from_string(nil)
	_(tracestate).must_be :empty?
	end

Could provide an example that would trigger the error?

Edit: I commented on this a bit too quickly, it appears this is specific to the TraceParent.from_string, not TraceState.from_string.

[robertlaurin]

I believe I understand the intention of this PR now.

This change is intends to stop relying on exceptions for control flow in the (potentially common) case where the TRACEPARENT key is not present in the carrier, so instead we return the context early if we failed to extract any value from that key.

So this is not resolving any bug but instead meant to improve performance, is that correct @hosamaly?

If we do take this approach, we should not return early if the trace_state_value is nil, as the Tracestate.from_string can handle that case.

Suggested change

	trace_parent_value = getter.get(carrier, TRACEPARENT_KEY)
	trace_state_value = getter.get(carrier, TRACESTATE_KEY)
	return context if trace_parent_value.nil? || trace_state_value.nil?
	tp = TraceParent.from_string(trace_parent_value)
	tracestate = Tracestate.from_string(trace_state_value)
	return context unless trace_parent_value = getter.get(carrier, TRACEPARENT_KEY)
	tp = TraceParent.from_string(trace_parent_value)
	tracestate = Tracestate.from_string(getter.get(carrier, TRACESTATE_KEY))

[fbogsany]

Unfortunately, we can't parse tracestate before parsing traceparent:

If the vendor failed to parse traceparent, it MUST NOT attempt to parse tracestate.

[hosamaly]

Indeed, you're absolutely right. I've committed your suggestion and reworded the commit to indicate that it's a performance improvement rather than a fix.

[fbogsany]

The W3C Trace Context states that:

If the vendor failed to parse traceparent, it MUST NOT attempt to parse tracestate. Note that the opposite is not true: failure to parse tracestate MUST NOT affect the parsing of traceparent.

so we must not return early if trace_state is nil.

Suggested change

	trace_parent_value = getter.get(carrier, TRACEPARENT_KEY)
	trace_state_value = getter.get(carrier, TRACESTATE_KEY)
	return context if trace_parent_value.nil? || trace_state_value.nil?
	trace_parent_value = getter.get(carrier, TRACEPARENT_KEY)
	trace_state_value = getter.get(carrier, TRACESTATE_KEY)
	return context if trace_parent_value.nil?

[fbogsany]

This is both a perf optimization and an attempt to reduce the use of exceptions for "normal" flow control. In general, I think it would be worth investing the effort to remove the use of exceptions in TraceParent parsing as well, but this is valid regardless.

[robertlaurin]

Just waiting on CI, will merge when complete.

Thanks for the PR!

[arielvalentin]

Linter error https://github.com/open-telemetry/opentelemetry-ruby/runs/5628071090?check_suite_focus=true#step:5:64

lib/opentelemetry/trace/propagation/trace_context/text_map_propagator.rb:48:54: W: Lint/AssignmentInCondition: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
[65](https://github.com/open-telemetry/opentelemetry-ruby/runs/5628071090?check_suite_focus=true#step:5:65)
            return context unless trace_parent_value = getter.get(carrier, TRACEPARENT_KEY)

[hosamaly]

Linter error

Fixed