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

Clarify the name used for Lightstep's (ot-tracer-*) propagator #1391

Closed
bogdandrutu opened this issue Jan 29, 2021 · 12 comments · Fixed by #1402
Closed

Clarify the name used for Lightstep's (ot-tracer-*) propagator #1391

bogdandrutu opened this issue Jan 29, 2021 · 12 comments · Fixed by #1402
Labels
area:api Cross language API specification issue priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory

Comments

@bogdandrutu
Copy link
Member

Currently in some repos this propagator is called OpenTracingPropogator in others is called OtPropagator. Couple suggestions:

  • Use consistent names, probably LightstepPropagator, since this is not the official OpenTracing propagator (or maybe I am wrong, but if that is the case then we need to probably add it to the ot-shim).
@bogdandrutu bogdandrutu added the spec:trace Related to the specification/trace directory label Jan 29, 2021
@bogdandrutu
Copy link
Member Author

/cc @yurishkuro @tedsuo @carlosalberto as experts in OpenTracing.

@yurishkuro
Copy link
Member

  1. I am curious who is asking for this propagator and for what purpose
  2. Agree that it should not be using the name OpenTracing
  3. Is there even a spec for that format?

@austinlparker
Copy link
Member

Conventionally the ot-tracer-* headers appeared in OpenTracing as part of the basictracer packages (see https://github.com/opentracing/basictracer-python/blob/master/basictracer/text_propagator.py, https://github.com/opentracing/basictracer-go/blob/master/propagation_ot.go, etc) and is conventionally referred to as 'OpenTracing' headers in 3rd party use (such as in Veneur - https://github.com/stripe/veneur/blob/master/trace/opentracing.go)

@codeboten
Copy link
Contributor

I am curious who is asking for this propagator and for what purpose

Users wanting to deploy new services using OpenTelemetry who have previously instrumented w/ OpenTracing, but aren't interested in updating their existing services.

Agree that it should not be using the name OpenTracing

Happy to change it based on the conclusion of the discussion here.

@yurishkuro
Copy link
Member

I recommend

// OTTracePropagator implements propagation format used by the "basic tracer" implementations from the OpenTracing project.
class OTTracePropagator

@austinlparker
Copy link
Member

sgtm

@jmacd
Copy link
Contributor

jmacd commented Jan 29, 2021

I agree this form is called "basic tracer" from OpenTelemetry.

but if that is the case then we need to probably add it to the ot-shim

@yurishkuro and @austinlparker what do you think?

For the record, Lightstep has a distinct single-header base64-encoded-protobuf representation of the OpenTracing "basic tracer" information, but that form is not the subject of this PR.

@austinlparker
Copy link
Member

What does adding it to ot-shim entail?

@jmacd
Copy link
Contributor

jmacd commented Jan 29, 2021

I think that means requiring it to be implemented as part of an on-spec OpenTracing bridge?

@yurishkuro
Copy link
Member

I don't think it needs to be in the shim or required. Basic tracer repos had explicit warning "sample implementation, not for production use". To my knowledge, they all had no trace export format, just in-memory collection. But it's fine to have the propagator in contrib for people who ignored that warning and used basic tracer in prod.

@carlosalberto
Copy link
Contributor

Agreed, it doesn't need to become part of the shim.

@andrewhsu andrewhsu added priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs area:api Cross language API specification issue labels Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants