Name resolution delay - #12893
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
…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
…ientTransportTest
…finished call timing
…endingCall, PendingStream, and OTel modules
…e resolution delay tests
|
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
left a comment
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.
| */ | ||
| synchronized void updateDelay(@Nullable String newType, @Nullable String newReason) { | ||
| if (getRealStream() != null) { | ||
| if (getRealStream() != null || delayEnded) { |
There was a problem hiding this comment.
Branch getRealStream() == null && delayEnded == true is missing coverage. To cover it, a unit test only needs to invoke endDelay() directly on a pending stream while getRealStream() == null and then call updateDelay(...):
// Example in DelayedClientTransportTest.java
PendingStream pendingStream = ...;
pendingStream.endDelay(); // sets delayEnded = true while getRealStream() is still null
pendingStream.updateDelay("connecting", "new reason"); // triggers missing branch > returns
There was a problem hiding this comment.
Lets not do this, I didn't realize this change requires changing the class access modifier for PendingStream.
| // Delay type changed (e.g., from RLS lookup to connecting). End the previous delay. | ||
| if (!Objects.equals(activeDelayType, newType)) { | ||
| // Delay type changed (e.g., from RLS lookup to connecting). End the previous delay. | ||
| if (activeDelayType != null) { |
There was a problem hiding this comment.
The is reported missing coverage for the branch when activeDelayType is null but it cannot be meaningfully tested because determineQueuingDelayType always returns non null value and there are other complications if we make this unconditional.
| @Override | ||
| public synchronized void recordCallDelayStart(String delayType, String delayReason) { | ||
| if (!GrpcOpenTelemetry.isDelayObservabilityEnabled() | ||
| || (activeCallDelayStopwatch != null && Objects.equals(activeCallDelayType, delayType))) { |
There was a problem hiding this comment.
When activeCallDelayStopwatch != null is true, invoking recordCallDelayStart with a different delayType while an existing call delay is already active to transition between delay segments is not exercised.
We can cover this branch by adding a transition call with a different delay type (e.g. "connecting") to clientCallDelayDuration_duplicateStart_ignoresSecondStart in OpenTelemetryMetricsModuleTest.java:
@Test
public void clientCallDelayDuration_duplicateStart_ignoresSecondStart() {
OpenTelemetryMetricsResource resource = GrpcOpenTelemetry.createMetricInstruments(
openTelemetryTesting.getOpenTelemetry().getMeterProvider().get("grpc-java"),
ImmutableMap.of("grpc.client.call.delay.duration", true),
false);
OpenTelemetryMetricsModule module = new OpenTelemetryMetricsModule(
new FakeClock().getStopwatchSupplier(), resource, emptyList(), emptyList());
OpenTelemetryMetricsModule.CallAttemptsTracerFactory factory =
new OpenTelemetryMetricsModule.CallAttemptsTracerFactory(
module, "target:///", CallOptions.DEFAULT, method.getFullMethodName(),
emptyList(), io.opentelemetry.context.Context.root());
factory.recordCallDelayStart("resolving", "first start");
// Duplicate call with same delay type is ignored
factory.recordCallDelayStart("resolving", "second start");
// Transition to a different delay type while active (exercises Objects.equals == false)
factory.recordCallDelayStart("connecting", "transition to connecting");
factory.recordCallDelayEnd();
assertThat(openTelemetryTesting.getMetrics())
.anySatisfy(
metric - assertThat(metric)
.hasName("grpc.client.call.delay.duration"));
}| public synchronized void recordCallDelayEnd() { | ||
| Stopwatch delayStopwatch = activeCallDelayStopwatch; | ||
| String delayType = activeCallDelayType; | ||
| if (delayStopwatch != null && delayType != null) { |
There was a problem hiding this comment.
In the test for the null delay type branch, assert that no metric was recorded for null (e.g. assertThat(openTelemetryTesting.getMetrics()).extracting("name").doesNotContain("grpc.client.call.delay.duration") or verify that only points with valid non-null grpc.delay_type attributes exist).
This finishes the remaining work in #12807