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

Allow users to choose propagator #320

Closed
carlosalberto opened this issue Apr 14, 2020 · 9 comments
Closed

Allow users to choose propagator #320

carlosalberto opened this issue Apr 14, 2020 · 9 comments
Assignees
Milestone

Comments

@carlosalberto
Copy link
Contributor

B3 and Jaeger propagators were recently added to OTel Java (living in the new trace_propagators artifact), and I would imagine being able to specify them when running the agent, e.g. -Dota.propagator=B3.

FWIW: Don't imagine users providing their own jar with their own custom propagator.

@trask Any opinion on this? ;)

@carlosalberto carlosalberto changed the title Allow users to choose propagators Allow users to choose propagator Apr 14, 2020
@trask
Copy link
Member

trask commented Apr 14, 2020

Do you know if propagators will have a standard SDK configuration, e.g. something similar to the BatchSpanProcessor configuration in open-telemetry/opentelemetry-java#1080? It's nice to follow standard SDK configuration where we can (maybe with our own prefix?). But either way, 👍 on supporting this use case.

@trask trask added the good first issue Good for newcomers label Apr 23, 2020
@prydin
Copy link
Contributor

prydin commented Apr 24, 2020

@carlosalberto I was looking for the trace_propagators artifact, but couldn't find it. Can you give me a pointer?

@trask
Copy link
Member

trask commented Apr 24, 2020

I see it in the repo https://github.com/open-telemetry/opentelemetry-java/tree/master/contrib/trace_propagators, but looks like it's not in maven central yet. I think it will be in maven central by end of next week when SDK 0.4.0 is released, so we should be able to come back to this soon (I'm hesitant to depend on SDK snapshot versions).

@prydin
Copy link
Contributor

prydin commented Apr 24, 2020

OK. I guess I'll pick another issue for now... :)

@trask trask added the contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome label Apr 25, 2020
@trask trask added this to the v0.3.0 milestone Apr 28, 2020
@carlosalberto
Copy link
Contributor Author

@trask So I will resume my work after open-telemetry/opentelemetry-java#1161 is either merged or rejected - but meanwhile wanted to ask about the design overall, as I'm not really familiar with the auto-instrumentation codebase.

So I'm simply exposing a few constants that match to pre-defined propagators (b3, tracecontext, etc). But the question is where to invoke this code. Initially I had added a PropagatorsInitializator under agent-tooling/src/main/java/io/opentelemetry/auto/tooling/, which will be called from agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/Agent.java. It feels like an overkill for this specific code, but I'm fine to put it there (as other initialization blocks live there).

If we keep the global Propagators in OpenTelemetry (the aforementioned PR being rejected), then we also need to do the propagators initialization in the same class loader where TracerInstaller gets called. Does this sound right?

@trask
Copy link
Member

trask commented Apr 30, 2020

Hey @carlosalberto, I think TracerInstaller is the best place for configuration of the embedded SDK (barring unforeseen issues).

@carlosalberto
Copy link
Contributor Author

@trask One of the 'problems' is however the fact Propagators can exist for other concerns (CorrelationContext, for example). But lets go with TracerInstaller for now, so we only consume the trace propagators, and iterate on top of that (as open-telemetry/opentelemetry-java#1161 wasn't merged as part of 0.4.0).

(Also, we don't have Propagators other than for traces/spans, so we are cool for now).

@trask trask removed good first issue Good for newcomers contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome labels May 6, 2020
@trask
Copy link
Member

trask commented May 6, 2020

Oh yeah, as @jkwatson mentioned on gitter today, the TracerInstaller class probably needs to be renamed (it does metrics exporter setup also 😆).

@carlosalberto
Copy link
Contributor Author

Closed via #389

(We can iterate on further/updated support when more propagators are added in the API/contrib section)

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

3 participants