add OpenTelemetryTracingModule #11477
Conversation
|
|
||
| private String generateErrorStatusDescription(io.grpc.Status status) { | ||
| if (status.getDescription() != null) { | ||
| return status.getCode() + ". " + status.getDescription(); |
There was a problem hiding this comment.
nit: We generally use : between the code and description.
TIL Java now lets you do string concatenation with a non-string left operand.
opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryTracingModuleTest.java
Outdated
Show resolved
Hide resolved
| public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor(); | ||
| private Tracer tracerRule; | ||
| @Mock | ||
| private Tracer mockTracer; |
There was a problem hiding this comment.
FWIW, I would have really preferred not using mocks all though here, as we don't know if we're using OTel properly. But since these are interfaces, at least we aren't doing horrible things to other people's objects, but we won't get default methods.
There was a problem hiding this comment.
Good point. I mostly use real otel from OpenTelemetryRule.
But they hardcoded w3c propagator in the rule.
https://github.com/open-telemetry/opentelemetry-java/blob/97b3fa42f10d43a4f27ecdf2d809bca95fed4de3/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/junit4/OpenTelemetryRule.java#L87
So whenever I wanted to test our grpcTraceBinContextPropagator I used mock.
There was a problem hiding this comment.
Maybe we should contribute to the OpenTelemetryRule package to use custom propagator.
| private static final Logger logger = Logger.getLogger(OpenTelemetryTracingModule.class.getName()); | ||
|
|
||
| @VisibleForTesting | ||
| static final String OTEL_TRACING_SCOPE_NAME = "grpc-opentelemetry-tracing"; |
There was a problem hiding this comment.
Do we want to include the language in the scope name to different scope across languages?
For instance, we used grpc-java for Metrics.
| Span clientSpan = otelTracer.spanBuilder( | ||
| generateTraceSpanName(false, method.getFullMethodName())) | ||
| .startSpan(); |
There was a problem hiding this comment.
How can we specify the parent span for this if one exists?
There was a problem hiding this comment.
Great question! Opentelemetry sdk by default pulls in the current span on the io.opentelemetry.context as it's parent span.
https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java#L185-L186
Users will just makeCurrent() of the parent span using opentelemetry context API.
Interceptor are run in channel thread.
|
@ejona86 @DNVindhya I'm merging this. Feel free to comment and I'll address in future PRs. |
What is missing.
Contextpropagation, i.e. inStreamTracer.filterContext(), OpenTelemetry Context does not automatically propagate to another thread. This needs a separate PR to override grpcContextStorage.inboundMessageReadin deframer will supplyoptionalWireSize==optionalUncompressedSizein compression situation.#11409