Skip to content

core,opentelemetry: Fix server metric labels on early close#12774

Merged
kannanjgithub merged 3 commits intogrpc:masterfrom
becomeStar:fix/otel-server-early-method-resolution
May 7, 2026
Merged

core,opentelemetry: Fix server metric labels on early close#12774
kannanjgithub merged 3 commits intogrpc:masterfrom
becomeStar:fix/otel-server-early-method-resolution

Conversation

@becomeStar
Copy link
Copy Markdown
Contributor

This addresses the server-side OpenTelemetry metric labeling bug from #12117 where a generated method can be recorded as grpc.method="other" if streamClosed() happens before serverCallStarted().

What changed

  • add an internal StatsTraceContext.ServerCallMethodListener hook so tracers can consume an already-resolved primary-registry MethodDescriptor
  • resolve the immutable internal primary registry on the transport path and seed method classification before the async MethodLookup path runs
  • keep fallback registry lookup on the existing async path
  • update the OpenTelemetry server tracer to use the early-resolved method classification for close metrics

Why this shape

  • avoids tracer-side HandlerRegistry lookup
  • uses only the immutable internal primary registry for early transport-path lookup
  • keeps fallback registry lookup on the existing async path

Tests

  • primary generated method: early close preserves the generated method name
  • primary non-generated method: early close still records other
  • fallback generated method: fallback lookup remains on the existing async path and does not introduce early transport-path classification
  • tracer-level regression: serverCallMethodResolved() + streamClosed() records the generated method name without waiting for serverCallStarted()

Notes

  • ServerCallMethodListener is an internal hook that carries the resolved MethodDescriptor; tracers consume the resolved result instead of performing registry lookup themselves
  • ServerImpl uses InternalHandlerRegistry explicitly for the primary registry to make it clear that the early transport- path lookup is limited to the immutable internal primary registry
  • this PR intentionally does not widen transport-path lookup to the fallback registry

Ref #12117

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.
@kannanjgithub
Copy link
Copy Markdown
Contributor

Unit test coverage is missing for non-generated method name case. If a method is not generated (or isSampledToLocalTracing is false), it should be recorded as "other". This applies to both the new serverCallMethodResolved() path and the standard serverCallStarted() path.

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.
@becomeStar
Copy link
Copy Markdown
Contributor Author

Thanks for the review.

Added the missing non-generated coverage in OpenTelemetryMetricsModuleTest.

The new tests cover both:

  • the new serverCallMethodResolved() path
  • the standard serverCallStarted() path

They verify that when isSampledToLocalTracing is false, the method is recorded as other for server metrics.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ServerCallMethodListener hook to deliver an already-resolved primary-registry MethodDescriptor before serverCallStarted().
  • Resolve the immutable primary registry method on the transport path in ServerImpl and 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.

@kannanjgithub
Copy link
Copy Markdown
Contributor

/gcbrun

@kannanjgithub kannanjgithub added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 7, 2026
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 7, 2026
@kannanjgithub kannanjgithub merged commit 0248e6f into grpc:master May 7, 2026
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants