Update propagators with composite #273

tsloughter · 2021-09-07T16:26:47Z

This is #269 but with a composite propagator instead of the only way to use propagators being a list.

I wanted to try this in part to see if it would lead me to a better/simpler API. I don't think it did. I open to this version if people see it as clearer, and it does stick closer to the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md

But I think I'd go with #269 -- curious to hear others thoughts?

codecov · 2021-09-07T16:27:33Z

Codecov Report

Merging #273 (45784c3) into main (277c98b) will increase coverage by 0.05%.
The diff coverage is 67.91%.

@@            Coverage Diff             @@
##             main     #273      +/-   ##
==========================================
+ Coverage   36.49%   36.55%   +0.05%     
==========================================
  Files          41       45       +4     
  Lines        3170     3187      +17     
==========================================
+ Hits         1157     1165       +8     
- Misses       2013     2022       +9

Flag	Coverage Δ
api	`62.44% <66.85%> (-0.94%)`	⬇️
elixir	`15.91% <0.00%> (-0.79%)`	⬇️
erlang	`36.50% <67.91%> (+0.04%)`	⬆️
exporter	`19.75% <ø> (+0.14%)`	⬆️
sdk	`78.94% <100.00%> (-0.19%)`	⬇️

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%)`	⬆️
...elemetry_api/src/otel_propagator_text_map_noop.erl	`0.00% <0.00%> (ø)`
apps/opentelemetry_api/src/otel_span.erl	`75.00% <ø> (-0.87%)`	⬇️
apps/opentelemetry_api/src/otel_tracer.erl	`50.00% <ø> (-9.26%)`	⬇️
...ntelemetry_exporter/src/opentelemetry_exporter.erl	`77.03% <ø> (+0.56%)`	⬆️
.../opentelemetry_api/src/otel_propagator_baggage.erl	`63.23% <63.23%> (ø)`
...try_api/src/otel_propagator_text_map_composite.erl	`66.66% <66.66%> (ø)`
.../opentelemetry_api/src/otel_propagator_b3multi.erl	`67.85% <67.85%> (ø)`
... and 14 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 277c98b...45784c3. Read the comment docs.

ferd · 2021-09-09T15:36:28Z

apps/opentelemetry_api/src/opentelemetry.erl

+    CompositeInjector = otel_propagator_text_map_composite:create(List),
+    CompositeExtractor = otel_propagator_text_map_composite:create(List),
+    set_text_map_injector(CompositeInjector),
+    set_text_map_extractor(CompositeExtractor);
 set_text_map_propagators(_) ->
    ok.


Would it make sense here to allow a single entry to work? I.e. people could pass in their own composite (or single) callback there, so long as they respect the sort of {Mod::atom(), Args::list()} format?

Ah yea, there was supposed to be a set_text_map_propagator like there is set_text_map_injector/extractor.

Tho maybe the plural forms should be removed. It is more important that simpler configuration works, which can do this without needing a public function. Would simplify the api, hopefully being less confusing.

Updated to only have the set_text_map_propagator function.

ferd · 2021-09-09T15:43:06Z

as confusing as this is (as mentioned in side-channels, this feels really like OO ontology to please type checkers so it feels a bit foreign here), it sounds like the ability to be able to pass in arguments/state to the callback modules would be an interesting win there, considering the prior ones had no such support.

bryannaegele

This is great. Being able to inject/extract with arbitrary inputs is going to be helpful to have built into the main libs and there's still a nice balance with the default implementations.

Looks like there's a couple of tests to fix up, but otherwise 👍🏻

tsloughter · 2021-09-15T23:03:04Z

@ferd yea, the ability to include options may be useful.

Right now there is no init/setup/create function for any propagator but the composite one, but options can be defined by configuring the propagator as a tuple.

I suppose a create could be added to the behaviour and all propagators either have the option to define it or be required to.

Which is something I don't like the in the current spec, it adds create to only the composite propagator and it isn't clear why. In an OOP language you'll have an init that can already be used to "create" a new composite propagator, and any other propagator, so why this special function only for it...

tsloughter · 2021-09-16T01:29:13Z

@bryannaegele note the other PR also has all those things. But to be clear, you find the composite version to be better (clearer to the user)?

I think we'll probably go with this but there are still the open question of how/whether to have an init for all propagators.

Also the question of configuration. My idea is that we only support text_map_propagators which creates a composite and the only way to not use a composite is to use code to set the propagator.

Related to configuration. What happens if propagators, injectors and extractors are all listed in the configuration? Do they get appended together or one override the other...

This change makes propagators use a behaviour to define the inject and extract functions instead of being anonymous functions. The latest Otel spec is also matched by adding functions for getting all fields a propagator sets and allowing the overriding of the get/set/keys functions used by inject/extract.

Co-authored-by: Bryan Naegele <bryannaegele@users.noreply.github.com>

Co-authored-by: Fred Hebert <mononcqc@ferd.ca>

Since a new option to inject/extract was needed I've added 2 new functions `inject_from` and `extract_to` that take Contexts as the first argument. Otherwise there is a lot of overloading of types of arguments required.

instead of propagators always being a list that are run in order this PR requires a composite propagator be created if a multiple propagators are to be used.

To simplify the interface from opentelemetry module the user must pass a single propagator, using a composite propagator if they need multiple. But the confiugration `text_map_propagators` remains and will create a composite propagator for the user.

tsloughter · 2021-09-24T15:46:14Z

Ok, I think this is ready now.

Note that in configuration right now you can only set the propagators (text map propagators to be specific) and not injectors/extractors individually. So the functions in opentelemetry must be used if a user wants to use separate propagators for injecting and extracting.

bryannaegele

So the functions in opentelemetry must be used if a user wants to use separate propagators for injecting and extracting.

Just making sure I understand - you're referring to otel_propagator_baggage:inject(Context, Carrier, CarrierSetFun, Options) for instance, correct?

apps/opentelemetry_api/src/otel_propagator.erl

tsloughter · 2021-09-24T20:41:52Z

@bryannaegele no, users should not use propagator modules directly like that at all. In that comment I made I was referring just to globals, like:

opentelemetry:set_text_map_injector({otel_propagator_b3multi, []})

Which makes the global text map injector be b3multi but doesn't touch what the extractors are.

To not use the global propagators a user should do:

otel_propagator_text_map:inject({otel_propagator_baggage, []}, [{<<"existing-header">>, <<"I exist">>}]),

tsloughter requested a review from a team September 7, 2021 16:26

github-actions bot added language-erlang scope-api scope-sdk labels Sep 7, 2021

tsloughter force-pushed the composite branch 2 times, most recently from 620eb77 to 13ae5e0 Compare September 8, 2021 14:30

github-actions bot added the language-elixir label Sep 8, 2021

ferd reviewed Sep 9, 2021

View reviewed changes

bryannaegele reviewed Sep 12, 2021

View reviewed changes

tsloughter and others added 8 commits September 24, 2021 05:29

documenting modules and functions related to propagators

05c05c3

Update apps/opentelemetry/src/otel_tracer_default.erl

efe9fd8

Co-authored-by: Bryan Naegele <bryannaegele@users.noreply.github.com>

Update apps/opentelemetry_api/test/custom_propagator.erl

d59c450

Co-authored-by: Fred Hebert <mononcqc@ferd.ca>

make tracestate default to an empty list

db8f00a

Add a changelog file

bac4249

support manually specifying propagators in inject/extract

a7070a8

Since a new option to inject/extract was needed I've added 2 new functions `inject_from` and `extract_to` that take Contexts as the first argument. Otherwise there is a lot of overloading of types of arguments required.

rework propagators to use a composite propagator

f59d27c

instead of propagators always being a list that are run in order this PR requires a composite propagator be created if a multiple propagators are to be used.

tsloughter force-pushed the composite branch from 4b66d42 to b16ab6a Compare September 24, 2021 11:30

tsloughter added 2 commits September 24, 2021 06:05

cleanup old uses of b3 to b3multi

1c898d4

tsloughter force-pushed the composite branch 3 times, most recently from fe76169 to a6a1d9b Compare September 24, 2021 12:45

fix otel_configuration to set text_map_propagators option

f772491

tsloughter force-pushed the composite branch from a6a1d9b to f772491 Compare September 24, 2021 15:13

improve type specs in propagator modules

45784c3

bryannaegele reviewed Sep 24, 2021

View reviewed changes

apps/opentelemetry_api/src/otel_propagator.erl Show resolved Hide resolved

bryannaegele approved these changes Sep 24, 2021

View reviewed changes

tsloughter merged commit 4905c93 into open-telemetry:main Sep 24, 2021

tsloughter mentioned this pull request Sep 24, 2021

Updates to the propagators interface #269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update propagators with composite #273

Update propagators with composite #273

tsloughter commented Sep 7, 2021

codecov bot commented Sep 7, 2021 •

edited

Loading

ferd Sep 9, 2021

tsloughter Sep 9, 2021

tsloughter Sep 9, 2021

tsloughter Sep 9, 2021

ferd commented Sep 9, 2021

bryannaegele left a comment

tsloughter commented Sep 15, 2021

tsloughter commented Sep 16, 2021

tsloughter commented Sep 24, 2021

bryannaegele left a comment

tsloughter commented Sep 24, 2021

Update propagators with composite #273

Update propagators with composite #273

Conversation

tsloughter commented Sep 7, 2021

codecov bot commented Sep 7, 2021 • edited Loading

Codecov Report

ferd Sep 9, 2021

Choose a reason for hiding this comment

tsloughter Sep 9, 2021

Choose a reason for hiding this comment

tsloughter Sep 9, 2021

Choose a reason for hiding this comment

tsloughter Sep 9, 2021

Choose a reason for hiding this comment

ferd commented Sep 9, 2021

bryannaegele left a comment

Choose a reason for hiding this comment

tsloughter commented Sep 15, 2021

tsloughter commented Sep 16, 2021

tsloughter commented Sep 24, 2021

bryannaegele left a comment

Choose a reason for hiding this comment

tsloughter commented Sep 24, 2021

codecov bot commented Sep 7, 2021 •

edited

Loading