-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat: Add Grpc Telemetry client interceptor for trace context propagation #3162
base: main
Are you sure you want to change the base?
feat: Add Grpc Telemetry client interceptor for trace context propagation #3162
Conversation
google-cloud-spanner/pom.xml
Outdated
<dependency> | ||
<groupId>io.opentelemetry.instrumentation</groupId> | ||
<artifactId>opentelemetry-grpc-1.6</artifactId> | ||
<version>2.1.0-alpha</version> |
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.
Why are we adding alpha package as a dependency, don't we have stable version of it ?
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.
There's no stable version available for this package. https://mvnrepository.com/artifact/io.opentelemetry.instrumentation/opentelemetry-grpc-1.6
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 think we should add non stable version in our library. Would you please check with the OT team or gRPC team and see what are the plans for making this stable ?
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.
Sure. Will check and update here.
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.
Copied the code from this library specific to trace context propagation. It's a small change and works. PTAL at your convenience.
…o grpc-telemetry-client-interceptor
…o grpc-telemetry-client-interceptor
...-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerInterceptorProvider.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/TraceContextInterceptor.java
Outdated
Show resolved
Hide resolved
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.context.propagation.ContextPropagators; | ||
import io.opentelemetry.context.propagation.TextMapSetter; | ||
|
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.
If this class is copied from somewhere, then we should add a comment that makes a reference to the original.
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.
Done. Added the link to the original class.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/TraceContextInterceptor.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/TraceContextInterceptor.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/TraceContextInterceptor.java
Outdated
Show resolved
Hide resolved
@Override | ||
public void start(Listener<RespT> responseListener, Metadata headers) { | ||
Context parentContext = Context.current(); | ||
|
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.
nit: remove the empty lines in this method. It is only 3 lines long (once the anonymous class has been removed), so it's easy to read without additional empty lines.
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.
Done.
@@ -158,6 +166,9 @@ public static Object[] data() { | |||
|
|||
@Before | |||
public void startServer() throws IOException { | |||
// Enable OpenTelemetry tracing. | |||
SpannerOptions.enableOpenTelemetryTraces(); |
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.
You must call SpannerOptions.resetActiveTracingFramework()
before calling this.
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.
Done. For this to work, had to make SpannerOptions.resetActiveTracingFramework()
public.
private final ContextPropagators propagators; | ||
|
||
TraceContextInterceptor(OpenTelemetry openTelemetry) { | ||
this.propagators = openTelemetry.getPropagators(); |
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.
This approach means that users are required to create and register the correct propagators with OpenTelemetry
for this to work. That has both an advantage and disadvantage:
- It won't work automatically out of the box without the user adding some additional (simple) configuration.
- It gives users a simple opt-out of the feature.
Is the above intentional? Or do we want this to always be enabled without any additional configuration from the user when OpenTelemetry is being used?
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.
Yes, this is intentional. In the documentation, it will be mentioned that user need to set the W3TraceContextPropagator(this provide methods to inject the traceparent
header from SpanContext) for server side tracing to work.
@@ -51,6 +52,7 @@ public static SpannerInterceptorProvider createDefault(OpenTelemetry openTelemet | |||
defaultInterceptorList.add( | |||
new LoggingInterceptor(Logger.getLogger(GapicSpannerRpc.class.getName()), Level.FINER)); | |||
defaultInterceptorList.add(new HeaderInterceptor(new SpannerRpcMetrics(openTelemetry))); | |||
defaultInterceptorList.add(new TraceContextInterceptor(openTelemetry)); |
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.
Is it possible to make the addition of this interceptor conditional on OpenTelemetry actually being used for this Spanner instance?
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.
For End to end tracing feature,
traceparent
header is required on the Spanner layer in all the requests. This change help in passingtraceparent
header inside Spanner requests if Context propagators are setup in OpenTelemetrySdk.