Conversation
opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java
Outdated
Show resolved
Hide resolved
opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricsModule.java
Outdated
Show resolved
Hide resolved
| streamPlugins = Collections.unmodifiableList(streamPluginsMutable); | ||
| } | ||
| return new ServerTracer(OpenTelemetryMetricsModule.this, fullMethodName, streamPlugins); | ||
| Context context = contextPropagators.getTextMapPropagator().extract( |
There was a problem hiding this comment.
This is being done by the OpenTelemetryTracingModule and is being propagated via filterContext(), specifically so it can be used by the metrics module. If we want the full context, propagate that instead. I don't understand the goal here (and there's no description about why we're doing this in the PR description or a comment from what I can tell). And I'm doubly confused by using this context directly half the time and copying BAGGAGE_KEY into it other times.
opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| void recordFinishedAttempt() { | ||
| Context otelContext = otelContextWithBaggage(); | ||
| Context otelContext = otelContextWithBaggage(BAGGAGE_KEY.get()); |
There was a problem hiding this comment.
There is no guaranteed Context as this point. I think this is only working because you're using in-process, which calls the StatsTraceContext on the calling thread. If the test used Netty instead, I suspect the baggage would no longer be found.
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
I think we're iterating further away from something appropriate. We shouldn't add the baggage to our attributes.
Take a look at #12671 |
2 participants
No description provided.