From 5e0626dcbd71ba2889a4f94ea4c1c11caeed30f3 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Wed, 12 Apr 2023 16:11:35 -0700 Subject: [PATCH 1/2] ns delay --- .../main/java/io/grpc/ClientStreamTracer.java | 5 +++ .../io/grpc/census/CensusTracingModule.java | 4 ++ .../io/grpc/census/CensusModulesTest.java | 5 ++- .../io/grpc/internal/ManagedChannelImpl.java | 4 +- .../grpc/internal/ManagedChannelImplTest.java | 45 +++++++++++++++++++ 5 files changed, 61 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/io/grpc/ClientStreamTracer.java b/api/src/main/java/io/grpc/ClientStreamTracer.java index 8b3520a8dcd..64fcd0e9c62 100644 --- a/api/src/main/java/io/grpc/ClientStreamTracer.java +++ b/api/src/main/java/io/grpc/ClientStreamTracer.java @@ -27,6 +27,11 @@ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/2861") @ThreadSafe public abstract class ClientStreamTracer extends StreamTracer { + /** + * The call was delayed due to waiting for name resolution result. + */ + public static final CallOptions.Key isResolutionDelay = + CallOptions.Key.createWithDefault("Name resolution delay", false); /** * The stream is being created on a ready transport. diff --git a/census/src/main/java/io/grpc/census/CensusTracingModule.java b/census/src/main/java/io/grpc/census/CensusTracingModule.java index dfe437780ba..8078588b476 100644 --- a/census/src/main/java/io/grpc/census/CensusTracingModule.java +++ b/census/src/main/java/io/grpc/census/CensusTracingModule.java @@ -17,6 +17,7 @@ package io.grpc.census; import static com.google.common.base.Preconditions.checkNotNull; +import static io.grpc.ClientStreamTracer.isResolutionDelay; import static io.grpc.census.internal.ObservabilityCensusConstants.CLIENT_TRACE_SPAN_CONTEXT_KEY; import com.google.common.annotations.VisibleForTesting; @@ -269,6 +270,9 @@ public ClientStreamTracer newClientStreamTracer( "previous-rpc-attempts", AttributeValue.longAttributeValue(info.getPreviousAttempts())); attemptSpan.putAttribute( "transparent-retry", AttributeValue.booleanAttributeValue(info.isTransparentRetry())); + if (info.getCallOptions().getOption(isResolutionDelay)) { + span.addAnnotation("Delayed name resolution complete"); + } return new ClientTracer(attemptSpan, span, tracingHeader, isSampledToLocalTracing); } diff --git a/census/src/test/java/io/grpc/census/CensusModulesTest.java b/census/src/test/java/io/grpc/census/CensusModulesTest.java index 2447b2c01ff..ba19e3041f7 100644 --- a/census/src/test/java/io/grpc/census/CensusModulesTest.java +++ b/census/src/test/java/io/grpc/census/CensusModulesTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static io.grpc.ClientStreamTracer.isResolutionDelay; import static io.grpc.census.CensusStatsModule.CallAttemptsTracerFactory.RETRIES_PER_CALL; import static io.grpc.census.CensusStatsModule.CallAttemptsTracerFactory.RETRY_DELAY_PER_CALL; import static io.grpc.census.CensusStatsModule.CallAttemptsTracerFactory.TRANSPARENT_RETRIES_PER_CALL; @@ -132,7 +133,8 @@ public class CensusModulesTest { private static final CallOptions CALL_OPTIONS = CallOptions.DEFAULT.withOption(CUSTOM_OPTION, "customvalue"); private static final ClientStreamTracer.StreamInfo STREAM_INFO = - ClientStreamTracer.StreamInfo.newBuilder().build(); + ClientStreamTracer.StreamInfo.newBuilder() + .setCallOptions(CallOptions.DEFAULT.withOption(isResolutionDelay, true)).build(); private static class StringInputStream extends InputStream { final String string; @@ -768,6 +770,7 @@ public void clientBasicTracingDefaultSpan() { .putAttribute("previous-rpc-attempts", AttributeValue.longAttributeValue(0)); inOrder.verify(spyAttemptSpan) .putAttribute("transparent-retry", AttributeValue.booleanAttributeValue(false)); + inOrder.verify(spyClientSpan).addAnnotation("Delayed name resolution complete"); inOrder.verify(spyAttemptSpan).addAnnotation("Delayed LB pick complete"); inOrder.verify(spyAttemptSpan, times(2)).addMessageEvent(messageEventCaptor.capture()); List events = messageEventCaptor.getAllValues(); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 4ab29d25734..ad82880900a 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static io.grpc.ClientStreamTracer.isResolutionDelay; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; @@ -1085,7 +1086,8 @@ void reprocess() { ClientCall realCall; Context previous = context.attach(); try { - realCall = newClientCall(method, callOptions); + CallOptions delayResolutionOption = callOptions.withOption(isResolutionDelay, true); + realCall = newClientCall(method, delayResolutionOption); } finally { context.detach(previous); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 3491eab2e65..7cea9401e5f 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static io.grpc.ClientStreamTracer.isResolutionDelay; import static io.grpc.ConnectivityState.CONNECTING; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; @@ -1071,6 +1072,50 @@ public void loadBalancerThrowsInHandleResolvedAddresses() { verifyPanicMode(ex); } + @Test + public void delayedNameResolution() { + ClientStream mockStream = mock(ClientStream.class); + final ClientStreamTracer tracer = new ClientStreamTracer() {}; + ClientStreamTracer.Factory factory = new ClientStreamTracer.Factory() { + @Override + public ClientStreamTracer newClientStreamTracer(StreamInfo info, Metadata headers) { + return tracer; + } + }; + FakeNameResolverFactory nsFactory = new FakeNameResolverFactory.Builder(expectedUri) + .setResolvedAtStart(false).build(); + channelBuilder.nameResolverFactory(nsFactory); + createChannel(); + + CallOptions callOptions = CallOptions.DEFAULT.withStreamTracerFactory(factory); + ClientCall call = channel.newCall(method, callOptions); + call.start(mockCallListener, new Metadata()); + + nsFactory.allResolved(); + Subchannel subchannel = + createSubchannelSafely(helper, addressGroup, Attributes.EMPTY, subchannelStateListener); + requestConnectionSafely(helper, subchannel); + MockClientTransportInfo transportInfo = transports.poll(); + transportInfo.listener.transportReady(); + ClientTransport mockTransport = transportInfo.transport; + when(mockTransport.newStream( + any(MethodDescriptor.class), any(Metadata.class), any(CallOptions.class), + ArgumentMatchers.any())) + .thenReturn(mockStream); + when(mockPicker.pickSubchannel(any(PickSubchannelArgs.class))).thenReturn( + PickResult.withSubchannel(subchannel)); + + updateBalancingStateSafely(helper, READY, mockPicker); + assertEquals(2, executor.runDueTasks()); + + verify(mockPicker).pickSubchannel(any(PickSubchannelArgs.class)); + verify(mockTransport).newStream( + same(method), any(Metadata.class), callOptionsCaptor.capture(), + tracersCaptor.capture()); + assertThat(Arrays.asList(tracersCaptor.getValue()).contains(tracer)).isTrue(); + assertThat(callOptionsCaptor.getValue().getOption(isResolutionDelay)).isTrue(); + } + @Test public void nameResolvedAfterChannelShutdown() { // Delay the success of name resolution until allResolved() is called. From c9ebfefd82d6702f406c21979d914d70500449cc Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Thu, 13 Apr 2023 11:14:58 -0700 Subject: [PATCH 2/2] rename --- api/src/main/java/io/grpc/ClientStreamTracer.java | 5 +++-- census/src/main/java/io/grpc/census/CensusTracingModule.java | 4 ++-- census/src/test/java/io/grpc/census/CensusModulesTest.java | 4 ++-- core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 4 ++-- .../test/java/io/grpc/internal/ManagedChannelImplTest.java | 4 ++-- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/io/grpc/ClientStreamTracer.java b/api/src/main/java/io/grpc/ClientStreamTracer.java index 64fcd0e9c62..5d55ae119f9 100644 --- a/api/src/main/java/io/grpc/ClientStreamTracer.java +++ b/api/src/main/java/io/grpc/ClientStreamTracer.java @@ -30,8 +30,9 @@ public abstract class ClientStreamTracer extends StreamTracer { /** * The call was delayed due to waiting for name resolution result. */ - public static final CallOptions.Key isResolutionDelay = - CallOptions.Key.createWithDefault("Name resolution delay", false); + public static final CallOptions.Key NAME_RESOLUTION_DELAYED = + CallOptions.Key.createWithDefault("io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED", + false); /** * The stream is being created on a ready transport. diff --git a/census/src/main/java/io/grpc/census/CensusTracingModule.java b/census/src/main/java/io/grpc/census/CensusTracingModule.java index 8078588b476..4afa08bc716 100644 --- a/census/src/main/java/io/grpc/census/CensusTracingModule.java +++ b/census/src/main/java/io/grpc/census/CensusTracingModule.java @@ -17,7 +17,7 @@ package io.grpc.census; import static com.google.common.base.Preconditions.checkNotNull; -import static io.grpc.ClientStreamTracer.isResolutionDelay; +import static io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED; import static io.grpc.census.internal.ObservabilityCensusConstants.CLIENT_TRACE_SPAN_CONTEXT_KEY; import com.google.common.annotations.VisibleForTesting; @@ -270,7 +270,7 @@ public ClientStreamTracer newClientStreamTracer( "previous-rpc-attempts", AttributeValue.longAttributeValue(info.getPreviousAttempts())); attemptSpan.putAttribute( "transparent-retry", AttributeValue.booleanAttributeValue(info.isTransparentRetry())); - if (info.getCallOptions().getOption(isResolutionDelay)) { + if (info.getCallOptions().getOption(NAME_RESOLUTION_DELAYED)) { span.addAnnotation("Delayed name resolution complete"); } return new ClientTracer(attemptSpan, span, tracingHeader, isSampledToLocalTracing); diff --git a/census/src/test/java/io/grpc/census/CensusModulesTest.java b/census/src/test/java/io/grpc/census/CensusModulesTest.java index ba19e3041f7..12c71d7269a 100644 --- a/census/src/test/java/io/grpc/census/CensusModulesTest.java +++ b/census/src/test/java/io/grpc/census/CensusModulesTest.java @@ -18,7 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; -import static io.grpc.ClientStreamTracer.isResolutionDelay; +import static io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED; import static io.grpc.census.CensusStatsModule.CallAttemptsTracerFactory.RETRIES_PER_CALL; import static io.grpc.census.CensusStatsModule.CallAttemptsTracerFactory.RETRY_DELAY_PER_CALL; import static io.grpc.census.CensusStatsModule.CallAttemptsTracerFactory.TRANSPARENT_RETRIES_PER_CALL; @@ -134,7 +134,7 @@ public class CensusModulesTest { CallOptions.DEFAULT.withOption(CUSTOM_OPTION, "customvalue"); private static final ClientStreamTracer.StreamInfo STREAM_INFO = ClientStreamTracer.StreamInfo.newBuilder() - .setCallOptions(CallOptions.DEFAULT.withOption(isResolutionDelay, true)).build(); + .setCallOptions(CallOptions.DEFAULT.withOption(NAME_RESOLUTION_DELAYED, true)).build(); private static class StringInputStream extends InputStream { final String string; diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index ad82880900a..696a3388556 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -19,7 +19,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; -import static io.grpc.ClientStreamTracer.isResolutionDelay; +import static io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; @@ -1086,7 +1086,7 @@ void reprocess() { ClientCall realCall; Context previous = context.attach(); try { - CallOptions delayResolutionOption = callOptions.withOption(isResolutionDelay, true); + CallOptions delayResolutionOption = callOptions.withOption(NAME_RESOLUTION_DELAYED, true); realCall = newClientCall(method, delayResolutionOption); } finally { context.detach(previous); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 7cea9401e5f..055b648e106 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -19,7 +19,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; -import static io.grpc.ClientStreamTracer.isResolutionDelay; +import static io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED; import static io.grpc.ConnectivityState.CONNECTING; import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; @@ -1113,7 +1113,7 @@ public ClientStreamTracer newClientStreamTracer(StreamInfo info, Metadata header same(method), any(Metadata.class), callOptionsCaptor.capture(), tracersCaptor.capture()); assertThat(Arrays.asList(tracersCaptor.getValue()).contains(tracer)).isTrue(); - assertThat(callOptionsCaptor.getValue().getOption(isResolutionDelay)).isTrue(); + assertThat(callOptionsCaptor.getValue().getOption(NAME_RESOLUTION_DELAYED)).isTrue(); } @Test