Updates to the propagators interface

[tsloughter]

Opening as a draft because I still need to add a lot of comments/docs and am not 100% on breaking the old configuration.

One thing I like about using closures for stuff like inject/extract (which this PR removes and instead just calls inject or extract on a module name) is it gives the ability to add runtime state into the functions without having to pass around an options variable. But probably not worth it or needed for this case and is cleaner the way it is now.

Also going to add a Changelog to better document how to upgrade.

[codecov]

Codecov Report

Merging #269 (c0223e1) into main (c0487cf) will decrease coverage by 0.05%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
- Coverage   36.39%   36.34%   -0.06%     
==========================================
  Files          41       43       +2     
  Lines        3165     3178      +13     
==========================================
+ Hits         1152     1155       +3     
- Misses       2013     2023      +10     

Flag	Coverage Δ
api	61.82% <66.47%> (-1.25%)	⬇️
elixir	15.35% <0.00%> (-0.64%)	⬇️
erlang	36.32% <66.66%> (-0.07%)	⬇️
exporter	19.75% <ø> (+0.14%)	⬆️
sdk	78.72% <75.00%> (-0.38%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
apps/opentelemetry/src/otel_tracer_default.erl	100.00% <ø> (ø)
...ps/opentelemetry_api/lib/open_telemetry/baggage.ex	37.50% <ø> (+4.16%)	⬆️
apps/opentelemetry_api/src/otel_baggage.erl	80.00% <ø> (+9.56%)	⬆️
apps/opentelemetry_api/src/otel_span.erl	73.07% <ø> (-1.00%)	⬇️
apps/opentelemetry_api/src/otel_tracer.erl	50.00% <ø> (-9.26%)	⬇️
...ntelemetry_exporter/src/opentelemetry_exporter.erl	77.03% <ø> (+0.56%)	⬆️
apps/opentelemetry_api/src/otel_propagator.erl	60.00% <60.00%> (-21.82%)	⬇️
.../opentelemetry_api/src/otel_propagator_baggage.erl	63.23% <63.23%> (ø)
...opentelemetry_api/src/otel_propagator_text_map.erl	65.21% <65.21%> (ø)
apps/opentelemetry/src/otel_configuration.erl	77.46% <66.66%> (-1.11%)	⬇️
... and 10 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 c0487cf...c0223e1. Read the comment docs.

[bryannaegele]

Looking good to me! This will make other propagators easier to write now.

I do think there is benefit to the composite propagators requirement and that we should implement it even if it comes in a different PR.

[bryannaegele]

Is there any harm in continuing to denote this as w3c like we do with b3?

[tsloughter]

No harm. I changed it because I looked at the other implementations and they all seemed to use "trace context". I'm going to double check and look at some more of them to see if it is really all of them or just Go and Python.

[ferd]

So I'm curious about what's the exact extent of the interface change there, and to which extent it relates to the spec? You mentioned writing in the changelog (not part of the PR), but I'm wondering how this reflects in versioning since there's the spec version and the lib version.

[tsloughter]

Oops, was supposed to be a changelog included in this PR. I'll get that added and hopefully it answers your question.

[tsloughter]

Added the chagnelog but realized it doesn't answer your question.

The changes aren't really spec related. Everything still acts the same it is just clearer (I think).

Before the propagators configuration value was a list of functions which returned a 2-tuple of {Injector, Extractor}. The interface of each of those individual functions hasn't really changed, but there is now a behaviour that must be followed instead and so configuration just needs to be the module to call.

The individual propagator inject/extract functions look more different than they are in this PR because the earlier version abstracted some parts around context out by providing a function in otel_ctx that would take an anonymous function and wrap it in another function that handled getting and setting the value from the context -- this was optional, so still followed the otel spec.

Now all that context stuff is done in each propagator, making it clearer to follow.

Oh, one thing that was added to match the spec (though in the spec it is optional, so we weren't not meeting the spec requirements) is allowing a getter and setter to be passed for a carrier.

[tsloughter]

@bryannaegele sorry, I missed your comment earlier about the composite propagator. Can you expand on why you think it is useful?

My understanding is that the only effect having a composite propagator would have is configuration would have to be something like:

{text_map_propagator, {composite, [tracecontext, baggage]}}

[bryannaegele]

Correct. And enable users to define their own composites. It's a requirement to offer it according to the spec which is why I brought it up.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#composite-propagator

There MUST be functions to accomplish the following operations.

• Create a composite propagator
• Extract from a composite propagator
• Inject into a composite propagator

It's been marked as N/A in the compliance spreadsheet but I'm not aware of a reason now with these changes that we shouldn't comply.

[tsloughter]

Right, it is N/A instead of - because I didn't think it was applicable to the Erlang implementation.

Maybe I'm missing part of its required functionality, what do you mean by users creating their own? How would it be different than the list of propagators the user gives now?

[bryannaegele]

Users can create custom propagators. I believe in the current implementation the only way to configure which are in use is via static config, but maybe that's wrong?

I guess I'm just not clear on why it's un-applicable to Erlang. I thought the only justification for skipping any requirement in the spec was for language limitations, not implementation choices.

[tsloughter]

opentelemetry exports functions for setting propagators at runtime if the user doesn't know or want to at runtime.

Being not applicable can also mean the functionality doesn't fit or isn't needed in an implementation.

I don't see what it adds in our case other than making the configuration that'll be used 99% of the time confusing. I'm still open to adding it but want to make sure where either it or a non-composite propagator (as opposed to a single element list like it is now) would be used, and think we are likely talking pased each other and once we figure out the miscommunication one of us will be happy with the others solution :)

[bryannaegele]

I'm not unhappy with this solution :)

I'm just asking for my own information and a broader discussion (we can take it elsewhere) where we are and are not saying non-optional spec requirements are ok to ignore if it isn't due to a language/runtime limitation.

In this particular discussion about composites, having a way to create a composite propagator would simplify the interface for creating a single inject/extract API for AMQP propagation, for instance, setting aside that it's not technically a textmap propagator. There's a legitimate use case I think for just going ahead and providing a native facility defined in the spec for creating composite propagators that aren't then set as a global. Maybe it won't work out exactly that way in this particular case, but the above question still remains.

The other situation that is relevant - correct me if I'm wrong - is we only have a global propagator implementation afaik. There aren't any facilities to scope which propagators are in use at a call level. Calling text_map_inject/1 takes the carrier and then grabs the globally-set propagators.

Using w3c as an example, traceparent and baggage are the defaults as a list. For clients calling within your infra, that's probably just fine. But for external client calls to third parties, you probably don't want to be sending baggage. Or maybe your default case is the opposite, where most calls are external, so you omit baggage, then want to add it for internal calls. In that use case, you have to create your own "composite" or use a provided one which keeps the same API semantics.

Maybe one solution to investigate (outside this PR - I think this PR is fine) is implementing this Propagators, depending on the language, MAY be set up using various dependency injection techniques or available as global accessors.

I hear the argument that 99% of users may not care about it. Maybe you think it's a superfluous requirement. But it's there as a MUST and defaults are listed as If pre-configured, Propagators SHOULD default to a composite Propagator containing the W3C Trace Context Propagator and the Baggage Propagator specified in the Baggage API. I'm not sure what is potentially confusing to users to see a default composite of otel_propagator_http_w3c that as the prescribed inject/1 and extract/1 that internally calls the two propagators (traceparent and baggage).

We can move this to a discussion or when I get around to the AMQP contrib then maybe I can open a PR with the example implementation so we have something more concrete to discuss. :)

[tsloughter]

Oh, this reminded me I never mentioned how I think the AMQP case may be solved in this PR. If I understood correctly the issue was the AMQP headers were a 3-tuple, the key, value and type? Now that a custom setter can be used you should have no issue using the same propagator setup for AMQP as you do for HTTP, you'll just need to define and pass a function that can take the key, value and AMQP headers and update them with the key/value/type.

And I didn't mean 99% wouldn't care, its 100% that shouldn't care (assuming this all works the same as the spec which I think it does) but 99% that won't even notice because setting a single propagator that isn't composite isn't a concern to them. Since really we only have a composite propagator, there is no non-composite for us.

The point about using a different set of propagators for a different set of calls is a good one since I don't think we have a way to do that anymore after this PR :). It needs to export the function that takes a list of propagators to the user instead of only making use of the global propagators.

I want to add that and tests before merging this.

Hm, I guess we'd have to go back to using a closure or carrying some state returned by a propagator "create" function if a composite propagator was added, since it'll need state.

I at least now think I see where you are coming from. You are thinking, "I want to define a propagator X I use in certain places that combines Y and Z", and right now that is only done by setting a list of global propagators? Which I think is fine as a list, but I see now why I might be wrong and need to look this over some more. Am I at least heading in the right direction with where you are coming from?

[bryannaegele]

You are thinking, "I want to define a propagator X I use in certain places that combines Y and Z", and right now that is only done by setting a list of global propagators?

Pretty much. It's the ability to let folks even have a simple abstraction of creating and storing their own composite propagators and still having the inject/extract interface.

There are shortcomings I feel in the spec around all of this, to be sure. The closest thing to flexibility is the ability to add dependency injection support so you could at least have a simple data structure and reduce a list of propagators.

[tsloughter]

Yea, the spec I guess assumes users will call propagators directly and not use the general propagator interface that uses the global ones when they need this.

Of course that could end badly if a library thinks it knows better or it simply makes a mistake and renders the user of the library incapable of overriding the propagators it uses -- but maybe not way around that.

Anyway, doing so was possible before this PR and I'll be updating it to the same today.

The question then becomes do we need propagators with state so must do:

Propagator = otel_propagator_composite_text_map:create([a, b, c]),
otel_propagator_text_map:inject(Propagator, Carrier)

Or keep them as they are in this PR but support:

Propagators = [a, b, c],
otel_propagator_text_map:inject(Propagators, Carrier)

And I should have mentioned that a reason for being N/A is also if it isn't idiomatic for the language.

[bryannaegele]

Oh to have a struct 😆

If we added the composite create and let's say used a tuple or record for the data structure, I think we can support everything. I also don't think we have to scope the create to a type of propagator as we don't have a way to verify what it is, I believe. It would be up to the user to not mix and match because the carrier would be different?

It's probably worthwhile to keep the inject interface remain the same and just add a /2 that accepts a composite? What do you think?

Propagators = otel_propagators:create_composite([a, b, c]),
%% {composite_propagator, [a,b,c]}
otel_propagator_text_map:inject(Carrier, {composite_propagator, Propagators}).

otel_propagator_text_map:inject(Carrier, Propagator) when is_atom(Propagator).

That maybe gives us additional flexibility to allow passing a single propagator by atom as dependency injection for a single or composite.

[tsloughter]

I've updated the PR to support passing the propagators to inject and extract as options. I'm not sure I like it and the new functions inject_from and extract_to might be confusing names (the "to" and "from" are in reference to the context).

[tsloughter]

@bryannaegele there needs to be the otel_propagator_text_map_composite because the text map propagators have an extended interface that accepts a getter, setter and keys function.

So otel_propagator_text_map:inject needs to be able to call the composite propagator with those extra arguments.

[tsloughter]

Closing after merging #273