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

Configuring propagators by environment variable #680

Closed
mwear opened this issue Jun 30, 2020 · 10 comments · Fixed by #995
Closed

Configuring propagators by environment variable #680

mwear opened this issue Jun 30, 2020 · 10 comments · Fixed by #995
Assignees
Labels
area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory

Comments

@mwear
Copy link
Member

mwear commented Jun 30, 2020

Background

This is issue is similar to #679, but deals with propagator configuration and has some additional considerations.

#666 allows configuration of propagators via and environment variable where the value is a comma delimited string. In order for this to work, there must a way to instantiate or look up a propagator using a string. Since this is an extension point, users will need a way to register custom propagators. Having a propagator registry is one way to accomplish this. There are likely other strategies that will work. Please discuss in the comments if you have other ideas.

Environment Variable Name

This is the currently proposed name and format

Name Description Notes Default
OTEL_PROPAGATOR Propagators to use as a comma separated list E.g. tracecontext, b3, b3,tracecontext tracecontext

Note: We will need to define a canonical name for each propagator. Is lower snakecase a reasonable choice for built in propagators, or should we choose something else?

Open Questions

What should be done in case of failure?

  • What no names match the configured value?
  • For the comma delimited case
    • What if some names match, but some do not?
    • Should whitespace be allowed, removed?
@Oberon00
Copy link
Member

Oberon00 commented Jul 7, 2020

Note that a major open point here is that we currently don't really support more than one propagator at a time, see #496.

I also think that it should be possible to configure extractors and injectors separately (it's likely you want to be able to read different formats but still write only W3C, for example). This is also not well-supported by the propagator API currently, however.

@Oberon00 Oberon00 added area:sdk Related to the SDK spec:trace Related to the specification/trace directory spec:context Related to the specification/context directory and removed spec:trace Related to the specification/trace directory labels Jul 7, 2020
@carlosalberto
Copy link
Contributor

I also think that it should be possible to configure extractors and injectors separately (it's likely you want to be able to read different formats but still write only W3C, for example).

Not all languages support separated injectors/extractors (Java doesn't, for example). But the case you mention on reading different formats but write only W3C could be supported in a MultiTracePropagator (through a flag specified at builder time, e.g. builder.setWriteFirstOnly()), as @jkwatson already mentioned somewhere.

@carlosalberto
Copy link
Contributor

Also, observe that this should support canonical formats for other signals, such as CorrelationContext ;)

@Oberon00
Copy link
Member

Oberon00 commented Jul 7, 2020

True, the MultiTracePropagator could simply have separate lists of propagators for Inject/Extract.

@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 10, 2020
@carlosalberto carlosalberto self-assigned this Jul 13, 2020
@carlosalberto
Copy link
Contributor

I'm interested in this issue, so I will take this on me.

@carlosalberto carlosalberto added the priority:p2 Medium priority level label Jul 24, 2020
@tigrannajaryan
Copy link
Member

Do we still consider this to be a must-have for GA?

@carlosalberto carlosalberto added priority:p1 Highest priority level and removed priority:p2 Medium priority level labels Sep 10, 2020
@carlosalberto
Copy link
Contributor

Changing this to P1 after talking to the Java Instrumentation Maintainers.

@trask
Copy link
Member

trask commented Sep 11, 2020

can you assign to me? thx

@trask
Copy link
Member

trask commented Sep 14, 2020

Since this is an extension point, users will need a way to register custom propagators.

@carlosalberto I don't think this is the issue that I thought it was. We don't need custom propagators in Java Instrumentation for GA.

@iNikem
Copy link
Contributor

iNikem commented Sep 14, 2020

But this PR is not only about custom propagators, right? It includes built-in ones.

@arminru arminru linked a pull request Sep 24, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants