Hide default context propagators implementation behind interface.

[anuraaga]

This seems to be more consistent with our other default implementations like DefaultTracer.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@c6d0df4). Click here to learn what that means.
The diff coverage is 86.04%.

@@            Coverage Diff            @@
##             master    #2089   +/-   ##
=========================================
  Coverage          ?   85.35%           
  Complexity        ?     2099           
=========================================
  Files             ?      239           
  Lines             ?     8032           
  Branches          ?      879           
=========================================
  Hits              ?     6856           
  Misses            ?      848           
  Partials          ?      328           

Impacted Files	Coverage Δ	Complexity Δ
...lemetry/context/propagation/TextMapPropagator.java	60.00% <60.00%> (ø)	3.00 <3.00> (?)
...context/propagation/DefaultContextPropagators.java	46.66% <80.00%> (ø)	4.00 <3.00> (?)
...try/context/propagation/NoopTextMapPropagator.java	83.33% <83.33%> (ø)	5.00 <5.00> (?)
...ava/io/opentelemetry/api/DefaultOpenTelemetry.java	88.88% <100.00%> (ø)	17.00 <0.00> (?)
...emetry/context/propagation/ContextPropagators.java	100.00% <100.00%> (ø)	2.00 <2.00> (?)
...ry/context/propagation/MultiTextMapPropagator.java	100.00% <100.00%> (ø)	8.00 <8.00> (?)
...elemetry/sdk/testing/junit4/OpenTelemetryRule.java	95.65% <100.00%> (ø)	7.00 <0.00> (?)
...try/sdk/testing/junit5/OpenTelemetryExtension.java	96.96% <100.00%> (ø)	10.00 <0.00> (?)
...try/sdk/metrics/AbstractSynchronousInstrument.java	76.92% <0.00%> (ø)	6.00% <0.00%> (?%)
...java/io/opentelemetry/sdk/trace/data/SpanData.java	100.00% <0.00%> (ø)	0.00% <0.00%> (?%)
... and 237 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6d0df4...f9c8c47. Read the comment docs.

[anuraaga]

I realized if we removed ContexrPropagators and only used TextMapPropagator with a multi-implementation it becomes consistent with span processor/exporter. If we plan to extend the interface with more methods, we should keep it but does anyone know if there are currently any ideas for that?

[jkwatson]

I realized if we removed ContexrPropagators and only used TextMapPropagator with a multi-implementation it becomes consistent with span processor/exporter. If we plan to extend the interface with more methods, we should keep it but does anyone know if there are currently any ideas for that?

I know there are plans for BinaryPropagators, but I haven't been following when that might land.

[Oberon00]

The spec issue seems rather dormant: open-telemetry/opentelemetry-specification#437

[jkwatson]

Having an interface for a builder, which is just a list of propagators and a constructor call feels like overkill to me. Is the rationale here to make the agent more easily able to bridge the builder?

Could we ditch the builder entirely and use a pattern like in #2091 where we just have a static method on the ContextPropagators that takes the list of TextMapPropagators?

[anuraaga]

Yup - it came to mind after sending the PR but before also having the idea of killing ContextPropagators completely :) Will do so since now I see the binary propagation idea is probably the main reason for this interface.

[anuraaga]

Thanks - I made some bigger changes, curious what people think.

TextMapPropagator.composite instead of doing so on ContextPropagators - while it does make registering multiple propagators a bit more annoying, it seems to reflect the actual semantics better. In particular, I can imagine us adding ContextPropagators.create(TextMapPropagator, BinaryMapPropagator) in the future which is very easy with this API. What do you think?

[jkwatson]

this usage looks really good to me. 👍

[carlosalberto]

I preferred the Builder but hiding the default instance looks great. On that note, TraceMultiPropagator may need to be lifted too, i.e. no Builder.

[anuraaga]

Thanks for calling out - let me followup with consistency in TraceMultiPropagator in a bit

[jkwatson]

Thanks!