Composite propagation doesn't work because the last propagator sets the span, potentially invalid, even if the previous one finds a valid span

[ecourreges-orange]

Describe your environment
v1.13.0 using bazel

Steps to reproduce
Remove this line from the unit test:

opentelemetry-cpp/api/test/context/propagation/composite_propagator_test.cc

Line 80 in a232328

{"b3", "80f198ee56343ba864fe8b2a57d3eff7-e457b5a2e4d86bd1-1-05e3ac9a4f6e3b90"}};

And you will see that the resulting traceId is 0000000..., whereas it should be the one from the valid traceparent header line 79.

What is the expected behavior?
The composite propagator should only set the active_span in the context to a valid one

What is the actual behavior?
The composite propagator sets the active_span at each propagator in its list, whatever the outcome, so the last one always wins, even if invalid.
You can see here:

opentelemetry-cpp/api/include/opentelemetry/trace/propagation/b3_propagator.h

Line 54 in a232328

return trace::SetSpan(context, sp);

and here:

opentelemetry-cpp/api/include/opentelemetry/trace/propagation/http_trace_context.h

Line 57 in a232328

return trace::SetSpan(context, sp);

Additional context
I see 3 ways of fixing it.

• disallow setting invalid spans with SetSpan? But then it might affect many parts of the code?
• CompositePropagator to do the filtering? Sounds complex because CompositePropagator does not know what is written to the context by the Extractors.
• I think the best and pretty simple is in all propagators to replace the line that does return trace::SetSpan(context, sp); with these:

    if (span_context.IsValid()) 
    {
      return trace::SetSpan(context, sp);
    } else {
      return context;
    }

If you agree, I will post a PR with that.

[lalitb]

@ecourreges-orange Yes, this is a valid issue. The proposal (third option) looks good, feel free to raise a PR for this. Thanks.