OpenTelemetryMetricsModule : Ensure method name is recorded instead of "other" in OpenTelemetry metrics. - #12325
Sangamesh1997 wants to merge 1 commit into
Conversation
…r" in OpenTelemetry metrics.
| this.fullMethodName = fullMethodName; | ||
| this.streamPlugins = checkNotNull(streamPlugins, "streamPlugins"); | ||
| this.stopwatch = module.stopwatchSupplier.get().start(); | ||
| isGeneratedMethod = fullMethodName != null; |
There was a problem hiding this comment.
This overrides early the reasoning specified in serverCallStarted but only till serverCallStarted is called. There will be higher cardinality metrics only in the small number of cases where the stream is closed before the server call is started.
@ejona86
There was a problem hiding this comment.
An attacker has control of whether the stream is closed before the server call is started.
I'm not sure there's anything to do to fix that issue, except maybe by making some processing occur in a certain order in the case of cancellation.
There was a problem hiding this comment.
This != null check is also garbage. It is guaranteed to be non-null (see CallAttemptsTracerFactory's constructor), so this is the same as isGeneratedMethod = true
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
We need to either error in the side of too many "other"s or not error at all. We can't change this to erring by taking on extra memory usage.
3 participants
Fixes: #12117
current change sets isGenerated = true whenever fullMethodName != null in ServerTracer. This ensures OpenTelemetry metrics don’t fall back to "other" in early-cancel cases.