Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurator improvements #438

Closed
mwear opened this issue Oct 14, 2020 · 7 comments
Closed

Configurator improvements #438

mwear opened this issue Oct 14, 2020 · 7 comments
Milestone

Comments

@mwear
Copy link
Member

mwear commented Oct 14, 2020

The configurator was introduced to simplify SDK setup, which used to be fairly cumbersome. It was a step in the right direction, but could use some attention and improvements before GA. This issue is to discuss and track desired changes.

@mwear
Copy link
Member Author

mwear commented Oct 27, 2020

Currently a user has to do the following to set up custom propagation (B3 in this case):

require 'opentelemetry-sdk'
require 'opentelemetry-propagator-b3'

OpenTelemetry::SDK.configure do |c|
  c.http_extractors = [OpenTelemetry::Propagator::B3::Single.rack_extractor, OpenTelemetry::Propagator::B3::Multi.rack_extractor]
  c.http_injectors = [OpenTelemetry::Propagator::B3::Single.rack_injector]
  c.text_map_extractors = [OpenTelemetry::Propagator::B3::Single.text_map_extractor, OpenTelemetry::Propagator::B3::Multi.text_map_extractor]
  c.text_map_injectors = [OpenTelemetry::Propagator::B3::Single.text_map_injector]
end

on gitter @genebean suggested having:

something like c.enable_b3 that does all the things

That specific approach might be challenging due to b3 being in a separate package, but we should be able to come up with something reasonable for supported propagation formats.

@mwlang
Copy link
Contributor

mwlang commented Oct 27, 2020

I'm starting to think we may benefit from having a separate Config class that holds config state and is a Singleton Object (not singleton pattern). If we did something like this, other dependency gems that add additional functionality could reopen the Config class and add a method like enable_b3 to "do all the things". Configure would still do it's thing, but also set states on the Config class along the way so everything stays sync'd.

@mwear
Copy link
Member Author

mwear commented Oct 28, 2020

We should keep that option on the table, but for configuring propagation generally, I think it'd be sufficient to have a propagators option that takes a comma delimited string of well known propagator names. For example,c.propagators = 'tracecontext,baggage'. This would match the behavior needed for the OPENTELEMETRY_PROPAGATORS environment variable as well.

We can build this on top of the individual *_injectors and *_extractors accessors and leave them as is for more sophisticated scenarios.

@mwear
Copy link
Member Author

mwear commented Oct 28, 2020

@mwlang what do you think about this as an approach for #412? If the b3 gem is present we could expose three b3 configurations: b3, b3multi, and b3single. b3 would setup extractors for b3 single and b3 multi, but only one injector for b3 single. This is due to the spec requirements listed here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#b3-requirements. b3single and b3multi would wire up only one set of injectors and extractors (either the single or multi variants).

@mwlang
Copy link
Contributor

mwlang commented Nov 17, 2020

@mwear I missed this comment from days ago. I definitely agree with adding a propagators option and I will wire it up as you described for b3. Thanks for linking the b3-requirements section. One thing I wanted to raise here before I got too far along, unless there's a strong desire to build a solution that's easily extensible, I am planning to centralize the logic for validating and wiring up propagators in one central place in the configurator.

Does that work?

@fbogsany fbogsany added this to the Tracing v1.0 milestone Feb 18, 2021
@fhwang
Copy link
Contributor

fhwang commented Mar 19, 2021

See also #321.

@mwear
Copy link
Member Author

mwear commented Jul 13, 2021

Closing as the SIG is happy with the configurator and improvements it has received since this was opened. Feel free to open issues for any future improvements that come up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants