-
Notifications
You must be signed in to change notification settings - Fork 827
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
Implement OpenTracing Shim specifying custom propagators #3059
Implement OpenTracing Shim specifying custom propagators #3059
Conversation
Signed-off-by: Sergei Malafeev <[email protected]>
Signed-off-by: Sergei Malafeev <[email protected]>
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/Propagators.java
Outdated
Show resolved
Hide resolved
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/Propagators.java
Outdated
Show resolved
Hide resolved
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/Propagators.java
Outdated
Show resolved
Hide resolved
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/Propagators.java
Outdated
Show resolved
Hide resolved
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/Propagators.java
Outdated
Show resolved
Hide resolved
opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanShimTest.java
Outdated
Show resolved
Hide resolved
Left a few small comments. Overall LGTM. Thanks! |
Signed-off-by: Sergei Malafeev <[email protected]>
Signed-off-by: Sergei Malafeev <[email protected]>
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingPropagators.java
Outdated
Show resolved
Hide resolved
...acing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingPropagatorsBuilder.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sergei Malafeev <[email protected]>
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingPropagators.java
Outdated
Show resolved
Hide resolved
opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingPropagators.java
Show resolved
Hide resolved
...acing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingPropagatorsBuilder.java
Show resolved
Hide resolved
...acing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingPropagatorsBuilder.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sergei Malafeev <[email protected]>
new TelemetryInfo( | ||
getTracer(GlobalOpenTelemetry.getTracerProvider()), | ||
GlobalOpenTelemetry.getPropagators())); | ||
return createTracerShim(getTracer(GlobalOpenTelemetry.getTracerProvider())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any way here to actually use custom propagators. As far as I can see, we only every use the standard stuff in GlobalOpenTelemetry
or the OpenTelemetry
instance passed in directly. Are we missing a creational method here that will allow passing in custom OpenTracingPropagators
somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
Signed-off-by: Sergei Malafeev <[email protected]>
* | ||
* @return a {@code io.opentracing.Tracer}. | ||
*/ | ||
public static io.opentracing.Tracer createTracerShim( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 creational methods make me think we should be using a builder. any thoughts about that? It could definitely be done as a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even 4.
maybe leave only two by removing createTracerShim(OpenTelemetry openTelemetry)
and createTracerShim()
?
@carlosalberto ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Let's do that in a follow up. @malafeev would you mind creating an issue for that, so we don't forget, before merging this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created #3084
Updates #3049
spec: https:/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/opentracing.md#create-an-opentracing-tracer-shim