Name resolution delay - #12893
Open
AgraVator wants to merge 60 commits into
Open
Name resolution delay#12893AgraVator wants to merge 60 commits into
AgraVator wants to merge 60 commits into
Conversation
This commit implements the plumbing required to propagate delay reason tokens from load balancing policies up to the transport layer and tracers, as specified in the LB policy delay design.
…dence invariants - Refactor ClientStreamTracer to expose delayTypeStarted(String) and delayReasonAttached(String) - Enhance PickResult with separate delayType and delayReason diagnostic fields - Implement Mark Roth's hybrid telemetry cadence model in DelayedClientTransport.PendingStream - Support channel fallback delay states (client_channel_init, subchannel_state_mismatch, wait_for_ready_failed) - Simplify leaf and container LB policies to emit canonical unified connecting metric labels
# Conflicts: # core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java
… in OpenTelemetry modules
…ay and Attempt-Level Delay
…for name resolution in ManagedChannelImpl
AgraVator
marked this pull request as draft
July 6, 2026 14:32
…name-resolution-delay branch
…ame-resolution-delay branch
…elay # Conflicts: # core/src/main/java/io/grpc/internal/DelayedClientTransport.java # core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java # core/src/test/java/io/grpc/internal/DelayedClientTransportTest.java # opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java # opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricsModule.java # opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricsResource.java # opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryTracingModule.java # opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java # opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryTracingModuleTest.java # rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java # rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java # util/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java # util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java # xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java # xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java # xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java # xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java
… and expand unit/stress tests
…nelImpl callback pattern
…y observability (gRFC A66) - Add nameResolutionDelay, lbPolicyDelay, and baselineNoDelay end-to-end tests to GrpcOpenTelemetryTest - Update LoadBalancer.PickResult.withError to set delayType="connecting" and delayReason=error.getDescription() - Fix missing static import checkstyle violation in CdsLoadBalancer2Test
…lay, remove debug prints, restore stress tests and add unit tests for patch coverage
…hStreamTracerFactory tests in LoadBalancerTest
…est and ManagedChannelImplTest
Contributor
|
Can you check the missing code coverages from the report and fix them? |
…s in delay observability
…lientTransport and OpenTelemetryTracingModule
…mptDelay, and duplicate cancel handling
…plugins in delay observability
kannanjgithub
requested changes
Aug 13, 2026
kannanjgithub
left a comment
kannanjgithub
left a comment
Contributor
There was a problem hiding this comment.
There are still some missing branches in codecov report. I have commented about missing coverages for OpenTelemetryMetricsModule.
Also address still existing coverages in the report except for cases where it cannot be done without changing class visibility or other reasons.
…edback, and prune redundant tests - Maintain private encapsulation for realChannel, RealChannel, and PendingCall in ManagedChannelImpl. - Synchronize delay lifecycle checks and tracer notifications to establish happens-before boundaries. - Add defensive lifecycle guards in OpenTelemetryMetricsModule to handle abrupt call endings and reject starts after callEnded. - Address codecov and reviewer feedback: - Test delay type transitions while active for both call and attempt delay metrics. - Test that null delay types record no metric in OpenTelemetryMetricsModule. - Test that null delay types fall back safely to empty strings in OpenTelemetryTracingModule. - Cover syncContext cancellation and tracer abort branches in ManagedChannelImpl using public API. - Prune redundant duplicate label tests, reflection helpers, and multi-threaded barrier stress tests.
… fast paths - In ManagedChannelImpl.PendingCall, replace AtomicBoolean delayEnded with boolean field guarded by monitor to avoid per-call heap allocation and CAS overhead. - In OpenTelemetryMetricsModule and OpenTelemetryTracingModule: - Check experiment flag before acquiring monitor on delay start. - Implement lock-free fast exit in record*DelayEnd when no delay is active. - Declare active delay stopwatches/spans as volatile for safe lock-free null reads. - Guard activeDelayType strictly with monitor lock. - In OpenTelemetryMetricsModuleTest, assert grpc.client.call.delay.duration metric recording on delay type transitions.
… volatile, and align with upstream
kannanjgithub
requested changes
Sep 9, 2026
…olve review comments - Unify call- and attempt-level child delay tracing span names to "Delay" - Align "Delay state transition" span event to record only grpc.delay_reason - Remove redundant local variables in OpenTelemetryTracingModule and OpenTelemetryMetricsModule - Eliminate redundant fully qualified Attributes references - Clean up synchronization and remove redundant volatile qualifiers - Update tests to follow repository style and verify updated span names and attributes
kannanjgithub
requested changes
Sep 10, 2026
… coverage - Update grpc.client.call.delay.duration and grpc.client.attempt.delay.duration descriptions to match gRFC A121 - Add assertions for metric descriptions, units, and required labels (grpc.target, grpc.method, grpc.delay_type) - Add test verifying null-safe behavior when delay observability flag is enabled but metrics are not opted in
kannanjgithub
approved these changes
Sep 10, 2026
AgraVator
commented
Sep 10, 2026
Comment on lines
+228
to
+230
| activeCallDelaySpan.addEvent( | ||
| "Delay state transition", | ||
| Attributes.of(AttributeKey.stringKey("grpc.delay_reason"), delayReason)); |
Contributor
Author
There was a problem hiding this comment.
Being discussed at grpc/proposal#556 (comment)
… alignment and concurrency fixes - Unify ClientStreamTracer and ClientStreamTracer.Factory delay APIs to recordDelayStart(type, reason), recordDelayReasonChanged(type, reason), and recordDelayEnd(type); update ForwardingClientStreamTracer classes. - Remove GRPC_EXPERIMENTAL_ENABLE_DELAY_OBSERVABILITY environment flag gating; register call and attempt delay duration histograms as standard optional metrics on GrpcOpenTelemetry. - Emit 'Delay triggered' span events with grpc.delay.type and grpc.delay.reason attributes on call and attempt spans. - Queue wait_for_ready calls arriving after initial name resolution failure (when no defaultServiceConfig is set) in pendingCalls with the resolution failure reason; release queued calls when defaultServiceConfig is applied. - Ensure all tracer callbacks run outside internal locks; add wait barrier in PendingStream.endDelay() and streamCreated guards in OTel ClientTracer to prevent stream creation/closure races. - Prefix child delay reasons with numeric priority index in PriorityLoadBalancer and unwrap FixedResultPicker in equals/hashCode. - Pass child connecting pickers through WeightedRandomPicker in WeightedTargetLoadBalancer when overall state is CONNECTING/IDLE.
…ay gaps and remove bogus tests
Two sites left the channel without a picker while carrying no A121 annotation,
so an RPC queued there fell back to the channel's generic "waiting for picker":
- GracefulSwitchLoadBalancer publishes its pending placeholder as-is when the
outgoing policy is not READY, i.e. before the incoming policy has reported
anything. Annotated as "waiting for the new load balancing policy to report
a picker".
- AutoConfiguredLoadBalancerFactory publishes a bare CONNECTING picker between
delegate.shutdown() and the new delegate producing one, on every policy
change. Annotated as "waiting for the load balancing policy to be applied".
Test quality pass. Removed tests that asserted nothing:
- lbPolicyDelay_endToEndClientServerSimulation and
baselineNoDelay_endToEndClientServerSimulation hand-drove a tracer and then
asserted the value they had just fed it; the latter asserted the absence of
delay metrics and would pass if the feature regressed entirely.
- nameResolutionDelay_endToEndClientServerSimulation stood up a real server,
channel and RPC but asserted on a separately constructed tracer factory, so
the RPC contributed nothing. Rewritten without the dead scaffolding and
renamed to describe what it actually verifies.
- delayMethodsForwarded (core and util) duplicated allMethodsForwarded, which
already reflects over every public method.
- clientAttemptDelayDuration_recorded and clientCallDelayDuration_recorded are
strict subsets of delayHistograms_bucketBoundariesAndUnit.
- clientMetrics_targetAttributeFilter_returnsFilteredOrOther is trivially true
with a null filter and covers code this change does not touch.
PriorityLoadBalancerTest.priorityPicker_equalsAndHashCodeAndToString compared a
picker to itself: updateOverallState suppresses a picker equal to the current
one, so the second publish never reached the helper and the captor returned the
prior instance. Interleave a distinct reason so the equality contract is really
exercised, and assert the instances differ.
Assert the delay type and reason at the emission sites that had none: pick_first
(address list updated, attempting to connect, requesting connection, health
check state), grpclb, the RLS fallback and child-without-picker paths,
cluster_impl, lazy, weighted_target, the DelayedClientTransport legacy-picker
fallback, and PickResult.copyWithSubchannel/copyWithStreamTracerFactory. The
last is invisible to every other test because PickResult.equals deliberately
excludes the delay attributes.
All new tests live in existing test classes.
…chines
The delay span is deliberately built outside the tracer's monitor, so a
concurrent call end, stream close or delay-type rollover can race its
publication. The production code detects that window ("stale") and ends the
orphaned span on the creating thread, because no other thread can observe it.
Nothing exercised that path: the OpenTelemetry modules had no multi-threaded
coverage at all, even though they hold the most intricate synchronization in
this change (the delay epoch counter, span creation outside the lock, and the
double-checked call-ended tests).
Count span starts and ends through a real SDK SpanProcessor and require them to
balance. That invariant holds under every possible interleaving, so the
assertion is deterministic even though the execution is not, and it is exactly
the property the stale handling exists to guarantee: a span that is started must
always be ended.
Three races are driven, 300 iterations each, through a CyclicBarrier:
recordDelayStart against callEnded, recordDelayStart against streamClosed, and
two threads driving delay type transitions at the same time.
Verified by mutation: disabling either stale-cleanup branch fails all three
tests. Verified for flakiness by 10 consecutive runs, 9000 racing iterations
total, with no failures. This also brings the two stale-cleanup branches under
coverage.
…ests The new tests called picker.pickSubchannel(null), copying an idiom from production code: PriorityLoadBalancer.fixedPickResult does pass null, because a FixedResultPicker ignores its argument. The xds tests do not follow that convention. Before this branch they used mock(PickSubchannelArgs.class) and never passed null, and PriorityLoadBalancerTest and GracefulSwitchLoadBalancerTest already had such call sites, so the new tests were inconsistent with the files they were added to. Pass a mock instead, which also avoids a NullPointerException if one of these pickers ever starts reading its arguments. Also shorten the SpanBalanceProcessor javadoc, which described the tests rather than the class, and move that rationale onto the test that depends on it. No behavior change.
The guards that protect an already terminated `resolving` delay were not
exercised. `callCancelled()` marks the delay finished synchronously but only
schedules the call's removal from the pending queue, so a test that cancels
from its own thread has the removal drained before it can deliver a resolver
event, and the release pass then sweeps an empty queue.
`callDelay_cancelledWhileQueued_laterResolutionFailureIsNotReported` documented
exactly that scenario but did not produce it, so it asserted that nothing
changed while nothing had been delivered. It now queues the resolver failure
and the cancellation from inside the SynchronizationContext, which keeps the
cancelled call in the queue when the failure arrives.
Two cases are added:
- a successful resolution reaching a cancelled call, which reprocesses it and
must not end a delay the tracer already terminated. The success path runs
one hop behind the failure path, since the result goes through onResult()
before onResult2() applies it, so the cancellation is nested accordingly.
- a call created just as resolution completes, which observes the initial
config selector but is handed to a real call before it is ever queued, and
so must not report a delay at all.
Verified by mutation: removing either guard in endDelayIfNeeded() fails the new
tests. The delayFinished check in notifyNameResolutionFailed() stays covered but
is not independently observable, as the isDelayFinished() check inside the
notification loop already stops the fan-out.
Resolves one conflict in ManagedChannelImpl.updateConfigSelector(). Upstream grpc#12832 resets the config selector when the channel enters IDLE, by calling realChannel.updateConfigSelector(INITIAL_PENDING_SELECTOR) from enterIdleMode(), and guards the method so that this reset does not reprocess the queued calls. This branch had rewritten the same method to release the pending calls through releasePendingCalls() and no longer has the prevConfig check that the guard was added to, so the two changes could not be combined textually. The reset is now handled by an early return that keeps upstream's behaviour: the new config selector is stored, but nothing is released. It also clears initialConfigResolved and lastResolutionError, because entering IDLE shuts the name resolver down and the channel really is back to the state it was in before its first resolution. Leaving initialConfigResolved set would make a resolution failure after the idle period skip the default service config in onConfigError(), and a stale lastResolutionError would be reported as the delay reason for a call queued after the channel went idle. Everything else merged cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This finishes the remaining work in #12807