fix: update grpc client side metrics detection to be graceful when not running on gcp (#3097)

BenWhitehead · web-flow · commit 10cd32d51aa0 · 2025-05-08T11:35:32.000-04:00
Refactor OpenTelemetryBootstrappingUtils to return a channel configurator function rather than mutating a passed in value.

Add ChannelConfigurator interface to make testing easier and to simplify composition with any existing channel configurator function present in the InstantiatingGrpcChannelProvider.
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java
@@ -60,6 +60,7 @@
 import com.google.cloud.spi.ServiceRpcFactory;
 import com.google.cloud.storage.GrpcUtils.ZeroCopyBidiStreamingCallable;
 import com.google.cloud.storage.Hasher.UncheckedChecksumMismatchException;
+import com.google.cloud.storage.OpenTelemetryBootstrappingUtils.ChannelConfigurator;
 import com.google.cloud.storage.RetryContext.RetryContextProvider;
 import com.google.cloud.storage.Retrying.DefaultRetrier;
 import com.google.cloud.storage.Storage.BlobWriteOption;
@@ -332,12 +333,14 @@ private Tuple<StorageSettings, Opts<UserProject>> resolveSettingsAndOpts() throw
     }
 
     if (enableGrpcClientMetrics) {
-      OpenTelemetryBootstrappingUtils.enableGrpcMetrics(
-          channelProviderBuilder,
-          endpoint,
-          this.getProjectId(),
-          this.getUniverseDomain(),
-          !grpcClientMetricsManuallyEnabled);
+      ChannelConfigurator channelConfigurator =
+          OpenTelemetryBootstrappingUtils.enableGrpcMetrics(
+              ChannelConfigurator.lift(channelProviderBuilder.getChannelConfigurator()),
+              endpoint,
+              this.getProjectId(),
+              this.getUniverseDomain(),
+              !grpcClientMetricsManuallyEnabled);
+      channelProviderBuilder.setChannelConfigurator(channelConfigurator);
     }
 
     builder.setTransportChannelProvider(channelProviderBuilder.build());
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/OpenTelemetryBootstrappingUtils.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/OpenTelemetryBootstrappingUtils.java
@@ -17,7 +17,6 @@
 package com.google.cloud.storage;
 
 import com.google.api.core.ApiFunction;
-import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
 import com.google.api.gax.rpc.PermissionDeniedException;
 import com.google.api.gax.rpc.UnavailableException;
 import com.google.cloud.opentelemetry.metric.GoogleCloudMetricExporter;
@@ -59,6 +58,8 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
+import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 final class OpenTelemetryBootstrappingUtils {
   private static final Collection<String> METRICS_TO_ENABLE =
@@ -88,15 +89,41 @@ final class OpenTelemetryBootstrappingUtils {
 
   static final Logger log = Logger.getLogger(OpenTelemetryBootstrappingUtils.class.getName());
 
-  static void enableGrpcMetrics(
-      InstantiatingGrpcChannelProvider.Builder channelProviderBuilder,
+  @NonNull
+  static ChannelConfigurator enableGrpcMetrics(
+      @Nullable ChannelConfigurator channelConfigurator,
       String endpoint,
-      String projectId,
+      @Nullable String projectId,
       String universeDomain,
       boolean shouldSuppressExceptions) {
+    GCPResourceProvider resourceProvider = new GCPResourceProvider();
+    Attributes detectedAttributes = resourceProvider.getAttributes();
+
+    @Nullable String detectedProjectId =
+        detectedAttributes.get(AttributeKey.stringKey("cloud.account.id"));
+    if (projectId == null && detectedProjectId == null) {
+      log.warning(
+          "Unable to determine the Project ID in order to report metrics. No gRPC client metrics will be reported.");
+      return channelConfigurator != null ? channelConfigurator : ChannelConfigurator.identity();
+    }
+
+    String projectIdToUse = detectedProjectId == null ? projectId : detectedProjectId;
+    if (!projectIdToUse.equals(projectId)) {
+      log.warning(
+          "The Project ID configured for gRPC client metrics is "
+              + projectIdToUse
+              + ", but the Project ID of the storage client is "
+              + projectId
+              + ". Make sure that the service account in use has the required metric writing role "
+              + "(roles/monitoring.metricWriter) in the project "
+              + projectIdToUse
+              + ", or metrics will not be written.");
+    }
+
     String metricServiceEndpoint = getCloudMonitoringEndpoint(endpoint, universeDomain);
     SdkMeterProvider provider =
-        createMeterProvider(metricServiceEndpoint, projectId, shouldSuppressExceptions);
+        createMeterProvider(
+            metricServiceEndpoint, projectIdToUse, detectedAttributes, shouldSuppressExceptions);
 
     OpenTelemetrySdk openTelemetrySdk =
         OpenTelemetrySdk.builder().setMeterProvider(provider).build();
@@ -106,16 +133,48 @@ static void enableGrpcMetrics(
             .addOptionalLabel("grpc.lb.locality")
             .enableMetrics(METRICS_TO_ENABLE)
             .build();
-    ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
-        channelProviderBuilder.getChannelConfigurator();
-    channelProviderBuilder.setChannelConfigurator(
+    ChannelConfigurator otelConfigurator =
         b -> {
           grpcOpenTelemetry.configureChannelBuilder(b);
-          if (channelConfigurator != null) {
-            return channelConfigurator.apply(b);
-          }
           return b;
-        });
+        };
+    return otelConfigurator.andThen(channelConfigurator);
+  }
+
+  @SuppressWarnings("rawtypes") // ManagedChannelBuilder
+  @FunctionalInterface
+  interface ChannelConfigurator extends ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> {
+    @NonNull
+    default ChannelConfigurator andThen(@Nullable ChannelConfigurator then) {
+      if (then == null) {
+        return this;
+      }
+      return b -> then.apply(this.apply(b));
+    }
+
+    static ChannelConfigurator identity() {
+      return IdentityChannelConfigurator.INSTANCE;
+    }
+
+    static ChannelConfigurator lift(
+        @Nullable ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> f) {
+      if (f == null) {
+        return identity();
+      }
+      return f::apply;
+    }
+  }
+
+  @SuppressWarnings("rawtypes") // ManagedChannelBuilder
+  private static final class IdentityChannelConfigurator implements ChannelConfigurator {
+    private static final IdentityChannelConfigurator INSTANCE = new IdentityChannelConfigurator();
+
+    private IdentityChannelConfigurator() {}
+
+    @Override
+    public ManagedChannelBuilder apply(ManagedChannelBuilder input) {
+      return input;
+    }
   }
 
   @VisibleForTesting
@@ -147,24 +206,10 @@ static String getCloudMonitoringEndpoint(String endpoint, String universeDomain)
 
   @VisibleForTesting
   static SdkMeterProvider createMeterProvider(
-      String metricServiceEndpoint, String projectId, boolean shouldSuppressExceptions) {
-    GCPResourceProvider resourceProvider = new GCPResourceProvider();
-    Attributes detectedAttributes = resourceProvider.getAttributes();
-
-    String detectedProjectId = detectedAttributes.get(AttributeKey.stringKey("cloud.account.id"));
-    String projectIdToUse = detectedProjectId == null ? projectId : detectedProjectId;
-
-    if (!projectIdToUse.equals(projectId)) {
-      log.warning(
-          "The Project ID configured for metrics is "
-              + projectIdToUse
-              + ", but the Project ID of the storage client is "
-              + projectId
-              + ". Make sure that the service account in use has the required metric writing role "
-              + "(roles/monitoring.metricWriter) in the project "
-              + projectIdToUse
-              + ", or metrics will not be written.");
-    }
+      String metricServiceEndpoint,
+      String projectIdToUse,
+      Attributes detectedAttributes,
+      boolean shouldSuppressExceptions) {
 
     MonitoredResourceDescription monitoredResourceDescription =
         new MonitoredResourceDescription(
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcMetricsTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcMetricsTest.java
@@ -21,6 +21,8 @@
 import com.google.cloud.storage.it.runner.StorageITRunner;
 import com.google.cloud.storage.it.runner.annotations.Backend;
 import com.google.cloud.storage.it.runner.annotations.SingleBackend;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.contrib.gcp.resource.GCPResourceProvider;
 import io.opentelemetry.sdk.metrics.SdkMeterProvider;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -36,9 +38,14 @@ public void testGrpcMetrics() {
                 "storage.googleapis.com:443", "storage.googleapis.com"))
         .isEqualTo("monitoring.googleapis.com:443");
 
+    GCPResourceProvider resourceProvider = new GCPResourceProvider();
+    Attributes detectedAttributes = resourceProvider.getAttributes();
     SdkMeterProvider provider =
         OpenTelemetryBootstrappingUtils.createMeterProvider(
-            "monitoring.googleapis.com:443", grpcStorageOptions.getProjectId(), false);
+            "monitoring.googleapis.com:443",
+            grpcStorageOptions.getProjectId(),
+            detectedAttributes,
+            false);
 
     /*
      * SDKMeterProvider doesn't expose the relevant fields we want to test, but they are present in
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryBootstrappingUtilsTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryBootstrappingUtilsTest.java
@@ -0,0 +1,97 @@
+/*
+ * Copyright 2025 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.cloud.storage;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assume.assumeFalse;
+import static org.mockito.Mockito.mock;
+
+import com.google.cloud.storage.OpenTelemetryBootstrappingUtils.ChannelConfigurator;
+import io.grpc.ManagedChannelBuilder;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.junit.Test;
+
+public final class OpenTelemetryBootstrappingUtilsTest {
+
+  @Test
+  public void noErrorIfNotRunningOnGcp() {
+    assumeFalse("Skipping because running on GCP", TestUtils.isOnComputeEngine());
+
+    ChannelConfigurator cc = ChannelConfigurator.identity();
+
+    String endpoint = "storage.googleapis.com:443";
+    String projectId = null;
+    String universeDomain = null;
+    ChannelConfigurator actual =
+        OpenTelemetryBootstrappingUtils.enableGrpcMetrics(
+            cc, endpoint, projectId, universeDomain, true);
+
+    assertThat(actual).isSameInstanceAs(cc);
+  }
+
+  @SuppressWarnings("rawtypes") // ManagedChannelBuilder
+  @Test
+  public void channelConfigurator_andThen() {
+    ManagedChannelBuilder b1 = mock(ManagedChannelBuilder.class, "b1");
+    ManagedChannelBuilder b2 = mock(ManagedChannelBuilder.class, "b2");
+    ManagedChannelBuilder b3 = mock(ManagedChannelBuilder.class, "b2");
+
+    ChannelConfigurator cc1 =
+        b -> {
+          assertThat(b).isSameInstanceAs(b1);
+          return b2;
+        };
+    ChannelConfigurator cc2 =
+        b -> {
+          assertThat(b).isSameInstanceAs(b2);
+          return b3;
+        };
+
+    ChannelConfigurator cc3 = cc1.andThen(cc2);
+
+    ManagedChannelBuilder apply = cc3.apply(b1);
+    assertThat(apply).isSameInstanceAs(b3);
+  }
+
+  @Test
+  public void channelConfigurator_lift_nullToIdentity() {
+    ChannelConfigurator actual = ChannelConfigurator.lift(null);
+    assertThat(actual).isSameInstanceAs(ChannelConfigurator.identity());
+  }
+
+  @SuppressWarnings("rawtypes") // ManagedChannelBuilder
+  @Test
+  public void channelConfigurator_lift_plumbingWorks() {
+    ManagedChannelBuilder b1 = mock(ManagedChannelBuilder.class, "b1");
+    AtomicBoolean called = new AtomicBoolean(false);
+    ChannelConfigurator lifted =
+        ChannelConfigurator.lift(
+            b -> {
+              called.compareAndSet(false, true);
+              return b;
+            });
+    ManagedChannelBuilder actual = lifted.apply(b1);
+    assertThat(actual).isSameInstanceAs(b1);
+    assertThat(called.get()).isTrue();
+  }
+
+  @Test
+  public void channelConfigurator_andThen_nullsafe() {
+    ChannelConfigurator actual = ChannelConfigurator.identity().andThen(null);
+    assertThat(actual).isSameInstanceAs(ChannelConfigurator.identity());
+  }
+}
