core,opentelemetry: Fix server metric labels on early close#12774
Conversation
Resolve generated-method classification before serverCallStarted() so close metrics do not fall back to "other" when streamClosed() happens first. Keep fallback registry lookup on the existing async path and avoid tracer-side HandlerRegistry access to match the maintainer constraints from issue grpc#12117. Add regressions for primary generated, primary non-generated, and fallback-generated server paths, plus the tracer-level early-resolution contract.
|
Unit test coverage is missing for non-generated method name case. If a method is not generated (or |
Add unit tests for non-generated server method metrics. Verify that both serverCallMethodResolved() and serverCallStarted() record the method name as "other" when isSampledToLocalTracing is false.
|
Thanks for the review. Added the missing non-generated coverage in OpenTelemetryMetricsModuleTest. The new tests cover both:
They verify that when |
There was a problem hiding this comment.
Pull request overview
Fixes a server-side OpenTelemetry metrics labeling race where grpc.method could be recorded as "other" when streamClosed() occurs before serverCallStarted(), by propagating an early-resolved primary-registry MethodDescriptor to server tracers.
Changes:
- Add an internal
StatsTraceContext.ServerCallMethodListenerhook to deliver an already-resolved primary-registryMethodDescriptorbeforeserverCallStarted(). - Resolve the immutable primary registry method on the transport path in
ServerImpland notify tracers early; keep fallback registry resolution on the existing async path. - Update the OpenTelemetry server tracer and add regression tests covering early-close labeling for primary vs fallback and generated vs non-generated methods.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/io/grpc/internal/StatsTraceContext.java |
Introduces ServerCallMethodListener and a notification method for early primary-registry method resolution. |
core/src/main/java/io/grpc/internal/ServerImpl.java |
Performs early primary-registry lookup on the transport path and notifies StatsTraceContext prior to async method lookup. |
opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricsModule.java |
Implements the new listener in the server tracer to seed method classification for close metrics. |
core/src/test/java/io/grpc/internal/ServerImplTest.java |
Adds transport-path/async-path tests validating labeling behavior across primary/fallback registries. |
opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java |
Adds tracer-level regression tests for method resolution preceding early stream close. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gcbrun |
This addresses the server-side OpenTelemetry metric labeling bug from #12117 where a generated method can be recorded as
grpc.method="other"ifstreamClosed()happens beforeserverCallStarted().What changed
StatsTraceContext.ServerCallMethodListenerhook so tracers can consume an already-resolved primary-registryMethodDescriptorMethodLookuppath runsWhy this shape
HandlerRegistrylookupTests
otherserverCallMethodResolved()+streamClosed()records the generated method name without waiting forserverCallStarted()Notes
ServerCallMethodListeneris an internal hook that carries the resolvedMethodDescriptor; tracers consume the resolved result instead of performing registry lookup themselvesServerImplusesInternalHandlerRegistryexplicitly for the primary registry to make it clear that the early transport- path lookup is limited to the immutable internal primary registryRef #12117