From 8efc70444db9198f9412bfcb2c74f6660d21fbe0 Mon Sep 17 00:00:00 2001 From: Min Zhu Date: Fri, 13 Sep 2024 11:34:46 -0400 Subject: [PATCH] feat: expose property in GrpcTransportChannel if it uses direct path. (#3170) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should fix #3134. Spanner needs to know if `GrpcSpannerStub`’s transportChannel uses DirectPath for tracing purposes. `canUseDirectPath()` from `stubSettings.getTransportChannelProvider()` does not provide accurate information because credentials are set afterwards when creating the transportChannel for `GrpcSpannerStub` (`GrpcSpannerStub.create(stubSettings)`) Proposed change: Expose a property in `GrpcTransportChannel` and pass this information down from channel provider. Spanner can access from `ClientContext` as shown below ``` SpannerStubSettings stubSettings = options .getSpannerStubSettings() .toBuilder() .setTransportChannelProvider(channelProvider) .setCredentialsProvider(credentialsProvider) .setStreamWatchdogProvider(watchdogProvider) .setTracerFactory(options.getApiTracerFactory()) .build(); ClientContext clientContext = ClientContext.create(stubSettings); // create GrpcSpannerStub from clientContext instead. // this is equivelant to GrpcSpannerStub.create(stubSettings) this.spannerStub = GrpcSpannerStub.create(clientContext); // access this property from transport channel. ((GrpcTransportChannel) clientContext.getTransportChannel()).isDirectPath(); ``` --- .../gax-grpc/clirr-ignored-differences.xml | 10 ++++ .../api/gax/grpc/GrpcTransportChannel.java | 6 ++- .../InstantiatingGrpcChannelProvider.java | 9 ++-- .../gax/grpc/GrpcTransportChannelTest.java | 48 +++++++++++++++++++ .../InstantiatingGrpcChannelProviderTest.java | 20 ++++++-- gax-java/gax/clirr-ignored-differences.xml | 5 -- 6 files changed, 85 insertions(+), 13 deletions(-) create mode 100644 gax-java/gax-grpc/clirr-ignored-differences.xml create mode 100644 gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcTransportChannelTest.java diff --git a/gax-java/gax-grpc/clirr-ignored-differences.xml b/gax-java/gax-grpc/clirr-ignored-differences.xml new file mode 100644 index 0000000000..8b595b0a85 --- /dev/null +++ b/gax-java/gax-grpc/clirr-ignored-differences.xml @@ -0,0 +1,10 @@ + + + + + + 7013 + com/google/api/gax/grpc/GrpcTransportChannel + boolean isDirectPath() + + diff --git a/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportChannel.java b/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportChannel.java index 9e63bb2a63..2fa0908f17 100644 --- a/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportChannel.java +++ b/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportChannel.java @@ -60,6 +60,8 @@ public GrpcCallContext getEmptyCallContext() { /** The channel in use. */ abstract ManagedChannel getManagedChannel(); + public abstract boolean isDirectPath(); + public Channel getChannel() { return getManagedChannel(); } @@ -100,7 +102,7 @@ public void close() { } public static Builder newBuilder() { - return new AutoValue_GrpcTransportChannel.Builder(); + return new AutoValue_GrpcTransportChannel.Builder().setDirectPath(false); } public static GrpcTransportChannel create(ManagedChannel channel) { @@ -111,6 +113,8 @@ public static GrpcTransportChannel create(ManagedChannel channel) { public abstract static class Builder { public abstract Builder setManagedChannel(ManagedChannel value); + abstract Builder setDirectPath(boolean value); + public abstract GrpcTransportChannel build(); } } diff --git a/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java index c49d5265f7..5d601e8fa6 100644 --- a/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java +++ b/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java @@ -258,9 +258,12 @@ public TransportChannel getTransportChannel() throws IOException { } private TransportChannel createChannel() throws IOException { - return GrpcTransportChannel.create( - ChannelPool.create( - channelPoolSettings, InstantiatingGrpcChannelProvider.this::createSingleChannel)); + return GrpcTransportChannel.newBuilder() + .setManagedChannel( + ChannelPool.create( + channelPoolSettings, InstantiatingGrpcChannelProvider.this::createSingleChannel)) + .setDirectPath(this.canUseDirectPath()) + .build(); } private boolean isDirectPathEnabled() { diff --git a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcTransportChannelTest.java b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcTransportChannelTest.java new file mode 100644 index 0000000000..337f054c42 --- /dev/null +++ b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcTransportChannelTest.java @@ -0,0 +1,48 @@ +/* + * Copyright 2024 Google LLC + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google LLC nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package com.google.api.gax.grpc; + +import static org.junit.jupiter.api.Assertions.*; + +import io.grpc.ManagedChannel; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +class GrpcTransportChannelTest { + + @Test + void testBuilderDefaults() { + ManagedChannel channel = Mockito.mock(ManagedChannel.class); + GrpcTransportChannel grpcTransportChannel = + GrpcTransportChannel.newBuilder().setManagedChannel(channel).build(); + assertFalse(grpcTransportChannel.isDirectPath()); + } +} diff --git a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java index 4de1b8ee11..a4bcb65a1a 100644 --- a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java +++ b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java @@ -706,7 +706,7 @@ void testLogDirectPathMisconfigNotOnGCE() throws Exception { } @Test - public void canUseDirectPath_happyPath() { + public void canUseDirectPath_happyPath() throws IOException { System.setProperty("os.name", "Linux"); EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class); Mockito.when( @@ -718,14 +718,20 @@ public void canUseDirectPath_happyPath() { .setAttemptDirectPath(true) .setCredentials(computeEngineCredentials) .setEndpoint(DEFAULT_ENDPOINT) - .setEnvProvider(envProvider); + .setEnvProvider(envProvider) + .setHeaderProvider(Mockito.mock(HeaderProvider.class)); InstantiatingGrpcChannelProvider provider = new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016); Truth.assertThat(provider.canUseDirectPath()).isTrue(); + + // verify this info is passed correctly to transport channel + TransportChannel transportChannel = provider.getTransportChannel(); + Truth.assertThat(((GrpcTransportChannel) transportChannel).isDirectPath()).isTrue(); + transportChannel.shutdownNow(); } @Test - public void canUseDirectPath_directPathEnvVarDisabled() { + public void canUseDirectPath_directPathEnvVarDisabled() throws IOException { System.setProperty("os.name", "Linux"); EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class); Mockito.when( @@ -737,10 +743,16 @@ public void canUseDirectPath_directPathEnvVarDisabled() { .setAttemptDirectPath(true) .setCredentials(computeEngineCredentials) .setEndpoint(DEFAULT_ENDPOINT) - .setEnvProvider(envProvider); + .setEnvProvider(envProvider) + .setHeaderProvider(Mockito.mock(HeaderProvider.class)); InstantiatingGrpcChannelProvider provider = new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016); Truth.assertThat(provider.canUseDirectPath()).isFalse(); + + // verify this info is passed correctly to transport channel + TransportChannel transportChannel = provider.getTransportChannel(); + Truth.assertThat(((GrpcTransportChannel) transportChannel).isDirectPath()).isFalse(); + transportChannel.shutdownNow(); } @Test diff --git a/gax-java/gax/clirr-ignored-differences.xml b/gax-java/gax/clirr-ignored-differences.xml index cbaed47eb5..af5c5f26a1 100644 --- a/gax-java/gax/clirr-ignored-differences.xml +++ b/gax-java/gax/clirr-ignored-differences.xml @@ -66,11 +66,6 @@ com/google/api/gax/*/* *org.threeten.bp.Duration *() - - 7002 - com/google/api/gax/grpc/InstantiatingGrpcChannelProvider$Builder - * * - 7012