fix: Drop the setTracingEnabled flag from Options (@BetaApi change) (#1869)

ehsannas · web-flow · commit 712d57e05da8 · 2024-11-06T10:19:46.000-08:00
* fix: Add the deprecation notice for tracing enable/disable option.

* feat: Drop the `setTracingEnabled` flag in `OpenTelemetryOptions` (Beta api change).

* throwable.getMessage may return null. Use toString which handles it.

* add clirr-ignore-diffs.

* Address feedback.

* fix typo in clirr-ignored-differences.xml.
diff --git a/google-cloud-firestore/clirr-ignored-differences.xml b/google-cloud-firestore/clirr-ignored-differences.xml
@@ -314,6 +314,18 @@
     <method>com.google.cloud.firestore.Query getQuery()</method>
   </difference>
 
+  <!-- Removed an OpenTelemetry Tracing Beta Api -->
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/firestore/FirestoreOpenTelemetryOptions</className>
+    <method>boolean isTracingEnabled()</method>
+  </difference>
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/firestore/FirestoreOpenTelemetryOptions$Builder</className>
+    <method>com.google.cloud.firestore.FirestoreOpenTelemetryOptions$Builder setTracingEnabled(boolean)</method>
+  </difference>
+
   <difference>
     <className>com/google/cloud/firestore/StreamableQuery</className>
     <differenceType>7009</differenceType>
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java
@@ -384,14 +384,15 @@ public void onError(Throwable throwable) {
             .currentSpan()
             .addEvent(
                 METHOD_NAME_RUN_AGGREGATION_QUERY + ": Retryable Error",
-                Collections.singletonMap("error.message", throwable.getMessage()));
+                Collections.singletonMap("error.message", throwable.toString()));
+
         runQuery(responseDeliverer, attempt + 1);
       } else {
         getTraceUtil()
             .currentSpan()
             .addEvent(
                 METHOD_NAME_RUN_AGGREGATION_QUERY + ": Error",
-                Collections.singletonMap("error.message", throwable.getMessage()));
+                Collections.singletonMap("error.message", throwable.toString()));
         responseDeliverer.deliverError(throwable);
       }
     }
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOpenTelemetryOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOpenTelemetryOptions.java
@@ -27,28 +27,15 @@
  */
 @BetaApi
 public class FirestoreOpenTelemetryOptions {
-  private final boolean tracingEnabled;
   private final boolean exportBuiltinMetricsToGoogleCloudMonitoring;
   private final @Nullable OpenTelemetry openTelemetry;
 
   FirestoreOpenTelemetryOptions(Builder builder) {
-    this.tracingEnabled = builder.tracingEnabled;
     this.exportBuiltinMetricsToGoogleCloudMonitoring =
         builder.exportBuiltinMetricsToGoogleCloudMonitoring;
     this.openTelemetry = builder.openTelemetry;
   }
 
-  /**
-   * @deprecated This method will be removed in the next minor version update. Please use a no-op
-   *     TracerProvider or set the environment variable `FIRESTORE_ENABLE_TRACING=OFF` to disable
-   *     tracing. If the GlobalOpenTelemetry or the OpenTelemetry instance passed to Firestore
-   *     contain a valid TracerProvider, the Firestore client will generate spans by utilizing it.
-   */
-  @Deprecated
-  public boolean isTracingEnabled() {
-    return tracingEnabled;
-  }
-
   public boolean exportBuiltinMetricsToGoogleCloudMonitoring() {
     return exportBuiltinMetricsToGoogleCloudMonitoring;
   }
@@ -68,20 +55,16 @@ public static FirestoreOpenTelemetryOptions.Builder newBuilder() {
   }
 
   public static class Builder {
-
-    private boolean tracingEnabled;
     private boolean exportBuiltinMetricsToGoogleCloudMonitoring;
     @Nullable private OpenTelemetry openTelemetry;
 
     private Builder() {
-      tracingEnabled = false;
       // TODO(metrics): default this to true when feature is ready
       exportBuiltinMetricsToGoogleCloudMonitoring = false;
       openTelemetry = null;
     }
 
     private Builder(FirestoreOpenTelemetryOptions options) {
-      this.tracingEnabled = options.tracingEnabled;
       this.exportBuiltinMetricsToGoogleCloudMonitoring =
           options.exportBuiltinMetricsToGoogleCloudMonitoring;
       this.openTelemetry = options.openTelemetry;
@@ -92,23 +75,6 @@ public FirestoreOpenTelemetryOptions build() {
       return new FirestoreOpenTelemetryOptions(this);
     }
 
-    /**
-     * Sets whether tracing should be enabled.
-     *
-     * @param tracingEnabled Whether tracing should be enabled.
-     * @deprecated This method will be removed in the next minor version update. Please use a no-op
-     *     TracerProvider or set the environment variable `FIRESTORE_ENABLE_TRACING=OFF` to disable
-     *     tracing. If the GlobalOpenTelemetry or the OpenTelemetry instance passed to Firestore
-     *     contains a valid TracerProvider, the Firestore client will generate spans by utilizing
-     *     it.
-     */
-    @Deprecated
-    @Nonnull
-    public FirestoreOpenTelemetryOptions.Builder setTracingEnabled(boolean tracingEnabled) {
-      this.tracingEnabled = tracingEnabled;
-      return this;
-    }
-
     // TODO(metrics): make this public when feature is ready.
     /**
      * Sets whether built-in metrics should be exported to Google Cloud Monitoring
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/StreamableQuery.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/StreamableQuery.java
@@ -378,7 +378,7 @@ public void onError(Throwable throwable) {
             if (isRetryableWithCursor() && shouldRetry(cursor, throwable)) {
               currentSpan.addEvent(
                   TelemetryConstants.METHOD_NAME_RUN_QUERY + ": Retryable Error",
-                  Collections.singletonMap("error.message", throwable.getMessage()));
+                  Collections.singletonMap("error.message", throwable.toString()));
 
               startAfter(cursor)
                   .internalStream(
@@ -391,7 +391,7 @@ public void onError(Throwable throwable) {
             } else {
               currentSpan.addEvent(
                   TelemetryConstants.METHOD_NAME_RUN_QUERY + ": Error",
-                  Collections.singletonMap("error.message", throwable.getMessage()));
+                  Collections.singletonMap("error.message", throwable.toString()));
               streamResponseObserver.onError(throwable);
             }
           }
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/DisabledTraceUtil.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/DisabledTraceUtil.java
@@ -18,6 +18,7 @@
 
 import com.google.api.core.ApiFunction;
 import com.google.api.core.ApiFuture;
+import com.google.api.core.InternalApi;
 import io.grpc.ManagedChannelBuilder;
 import java.util.Map;
 import javax.annotation.Nonnull;
@@ -27,6 +28,7 @@
  * A no-op implementation of {@link MetricsUtil} that does not collect or export any metrics and has
  * near-zero overhead.
  */
+@InternalApi
 public class DisabledTraceUtil implements TraceUtil {
 
   static class Span implements TraceUtil.Span {
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/EnabledTraceUtil.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/EnabledTraceUtil.java
@@ -20,6 +20,7 @@
 import com.google.api.core.ApiFuture;
 import com.google.api.core.ApiFutureCallback;
 import com.google.api.core.ApiFutures;
+import com.google.api.core.InternalApi;
 import com.google.cloud.firestore.FirestoreOptions;
 import com.google.common.base.Throwables;
 import io.grpc.ManagedChannelBuilder;
@@ -31,6 +32,7 @@
 import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.api.trace.StatusCode;
 import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.TracerProvider;
 import io.opentelemetry.instrumentation.grpc.v1_6.GrpcTelemetry;
 import java.util.Map;
 import javax.annotation.Nonnull;
@@ -40,6 +42,7 @@
  * A utility class that uses OpenTelemetry for trace collection. `FirestoreOpenTelemetryOptions` in
  * `FirestoreOptions` can be used to configure its behavior.
  */
+@InternalApi
 public class EnabledTraceUtil implements TraceUtil {
   private final Tracer tracer;
   private final OpenTelemetry openTelemetry;
@@ -48,8 +51,7 @@ public class EnabledTraceUtil implements TraceUtil {
   EnabledTraceUtil(FirestoreOptions firestoreOptions) {
     OpenTelemetry openTelemetry = firestoreOptions.getOpenTelemetryOptions().getOpenTelemetry();
 
-    // If tracing is enabled, but an OpenTelemetry instance is not provided, fall back
-    // to using GlobalOpenTelemetry.
+    // If an OpenTelemetry instance is not provided, fall back to using GlobalOpenTelemetry.
     if (openTelemetry == null) {
       openTelemetry = GlobalOpenTelemetry.get();
     }
@@ -84,6 +86,11 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder managedChannelBuilder)
   @Override
   @Nullable
   public ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> getChannelConfigurator() {
+    // Note: using `==` rather than `.equals` since OpenTelemetry has only 1 static instance of
+    // `TracerProvider.noop`.
+    if (openTelemetry.getTracerProvider() == TracerProvider.noop()) {
+      return null;
+    }
     return new OpenTelemetryGrpcChannelConfigurator();
   }
 
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/TraceUtil.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/TraceUtil.java
@@ -18,6 +18,7 @@
 
 import com.google.api.core.ApiFunction;
 import com.google.api.core.ApiFuture;
+import com.google.api.core.InternalApi;
 import com.google.cloud.firestore.FirestoreOptions;
 import io.grpc.ManagedChannelBuilder;
 import java.util.Map;
@@ -50,8 +51,9 @@ public interface TraceUtil {
    *     TraceUtil.
    * @return An instance of the TraceUtil class.
    */
+  @InternalApi
   static TraceUtil getInstance(@Nonnull FirestoreOptions firestoreOptions) {
-    boolean createEnabledInstance = firestoreOptions.getOpenTelemetryOptions().isTracingEnabled();
+    boolean createEnabledInstance = true;
 
     // The environment variable can override options to enable/disable telemetry collection.
     String enableTracingEnvVar = System.getenv(ENABLE_TRACING_ENV_VAR);
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/OpenTelemetryOptionsTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/OpenTelemetryOptionsTest.java
@@ -18,11 +18,12 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
-import com.google.cloud.firestore.telemetry.DisabledTraceUtil;
 import com.google.cloud.firestore.telemetry.EnabledTraceUtil;
 import io.opentelemetry.api.GlobalOpenTelemetry;
-import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.trace.TracerProvider;
 import io.opentelemetry.sdk.OpenTelemetrySdk;
+import io.opentelemetry.sdk.resources.Resource;
+import io.opentelemetry.sdk.trace.SdkTracerProvider;
 import javax.annotation.Nullable;
 import org.junit.After;
 import org.junit.Before;
@@ -49,112 +50,67 @@ FirestoreOptions.Builder getBaseOptions() {
   }
 
   @Test
-  public void defaultOptionsDisablesTelemetryCollection() {
+  public void defaultOptionsUsesEnabledTraceUtilWithNoopOpenTelemetry() {
     FirestoreOptions firestoreOptions = getBaseOptions().build();
     firestore = firestoreOptions.getService();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isFalse();
     assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry()).isNull();
     assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
-    assertThat(firestore.getOptions().getTraceUtil() instanceof DisabledTraceUtil).isTrue();
-  }
-
-  @Test
-  public void canEnableTelemetryCollectionWithoutOpenTelemetryInstance() {
-    FirestoreOptions firestoreOptions =
-        getBaseOptions()
-            .setOpenTelemetryOptions(
-                FirestoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build())
-            .build();
-    firestore = firestoreOptions.getService();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isTrue();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry()).isNull();
-    assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
-    assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
-  }
-
-  @Test
-  public void canEnableTelemetryCollectionWithOpenTelemetryInstance() {
-    OpenTelemetry openTelemetry = GlobalOpenTelemetry.get();
-    FirestoreOptions firestoreOptions =
-        getBaseOptions()
-            .setOpenTelemetryOptions(
-                FirestoreOpenTelemetryOptions.newBuilder()
-                    .setTracingEnabled(true)
-                    .setOpenTelemetry(openTelemetry)
-                    .build())
-            .build();
-    firestore = firestoreOptions.getService();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isTrue();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry())
-        .isEqualTo(openTelemetry);
-    assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
     assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
+    EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
+    // Assert that a Noop tracer provider is used by default.
+    assertThat(enabledTraceUtil.getOpenTelemetry().getTracerProvider())
+        .isSameInstanceAs(TracerProvider.noop());
   }
 
   @Test
-  public void canDisableTelemetryCollectionWhileOpenTelemetryInstanceIsNotNull() {
-    OpenTelemetry openTelemetry = GlobalOpenTelemetry.get();
-    FirestoreOptions firestoreOptions =
-        getBaseOptions()
-            .setOpenTelemetryOptions(
-                FirestoreOpenTelemetryOptions.newBuilder()
-                    .setTracingEnabled(false)
-                    .setOpenTelemetry(openTelemetry)
-                    .build())
-            .build();
-    firestore = firestoreOptions.getService();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isFalse();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry())
-        .isEqualTo(openTelemetry);
-    assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
-    assertThat(firestore.getOptions().getTraceUtil() instanceof DisabledTraceUtil).isTrue();
-  }
+  public void existenceOfGlobalOpenTelemetryEnablesTracingWithTheGlobalTracerProvider() {
+    // Make a custom TracerProvider.
+    Resource resource =
+        Resource.getDefault().merge(Resource.builder().put("service.name", "test").build());
+    SdkTracerProvider myTracerProvider = SdkTracerProvider.builder().setResource(resource).build();
 
-  @Test
-  public void existenceOfGlobalOpenTelemetryDoesNotEnableTracing() {
-    // Register a global OpenTelemetry SDK.
-    OpenTelemetrySdk.builder().buildAndRegisterGlobal();
+    // Register a GlobalOpenTelemetry with the custom tracer provider.
+    OpenTelemetrySdk.builder().setTracerProvider(myTracerProvider).buildAndRegisterGlobal();
 
-    // Make sure Firestore does not use GlobalOpenTelemetry by default.
+    // Do NOT pass an OpenTelemetry instance to Firestore.
     FirestoreOptions firestoreOptions = getBaseOptions().build();
     firestore = firestoreOptions.getService();
-    assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isFalse();
+
+    // An OpenTelemetry instance has not been set in options.
     assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry()).isNull();
+
+    // Assert that tracing is enabled and is using the custom tracer provider from the
+    // GlobalOpenTelemetry.
     assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
-    assertThat(firestore.getOptions().getTraceUtil() instanceof DisabledTraceUtil).isTrue();
+    assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
+    EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
+    assertThat(enabledTraceUtil.getOpenTelemetry().getTracerProvider().equals(myTracerProvider));
   }
 
   @Test
   public void canPassOpenTelemetrySdkInstanceToFirestore() {
-    OpenTelemetrySdk myOpenTelemetrySdk = OpenTelemetrySdk.builder().build();
+    // Make a custom TracerProvider and make an OpenTelemetry instance with it.
+    Resource resource =
+        Resource.getDefault().merge(Resource.builder().put("service.name", "test").build());
+    SdkTracerProvider myTracerProvider = SdkTracerProvider.builder().setResource(resource).build();
+    OpenTelemetrySdk myOpenTelemetrySdk =
+        OpenTelemetrySdk.builder().setTracerProvider(myTracerProvider).build();
+
+    // Pass it to Firestore.
     FirestoreOptions firestoreOptions =
         getBaseOptions()
             .setOpenTelemetryOptions(
                 FirestoreOpenTelemetryOptions.newBuilder()
-                    .setTracingEnabled(true)
                     .setOpenTelemetry(myOpenTelemetrySdk)
                     .build())
             .build();
     firestore = firestoreOptions.getService();
+    assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry())
+        .isEqualTo(myOpenTelemetrySdk);
+    assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
     EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
     assertThat(enabledTraceUtil).isNotNull();
     assertThat(enabledTraceUtil.getOpenTelemetry()).isEqualTo(myOpenTelemetrySdk);
-  }
-
-  @Test
-  public void usesGlobalOpenTelemetryIfOpenTelemetryNotProvidedInOptions() {
-    // Register a global OpenTelemetry SDK.
-    OpenTelemetrySdk.builder().buildAndRegisterGlobal();
-
-    // Do _not_ pass it to FirestoreOptions.
-    FirestoreOptions firestoreOptions =
-        getBaseOptions()
-            .setOpenTelemetryOptions(
-                FirestoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build())
-            .build();
-    firestore = firestoreOptions.getService();
-    EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
-    assertThat(enabledTraceUtil).isNotNull();
-    assertThat(enabledTraceUtil.getOpenTelemetry()).isEqualTo(GlobalOpenTelemetry.get());
+    assertThat(enabledTraceUtil.getOpenTelemetry().getTracerProvider().equals(myTracerProvider));
   }
 }
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITE2ETracingTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITE2ETracingTest.java
@@ -319,20 +319,13 @@ public void before() throws Exception {
     // Initialize the Firestore DB w/ the OTel SDK. Ideally we'd do this is the @BeforeAll method
     // but because gRPC traces need to be deterministically force-flushed, firestore.shutdown()
     // must be called in @After for each test.
-    FirestoreOptions.Builder optionsBuilder;
-    if (isUsingGlobalOpenTelemetrySDK()) {
-      optionsBuilder =
-          FirestoreOptions.newBuilder()
-              .setOpenTelemetryOptions(
-                  FirestoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build());
-    } else {
+    FirestoreOptions.Builder optionsBuilder = FirestoreOptions.newBuilder();
+    if (!isUsingGlobalOpenTelemetrySDK()) {
       optionsBuilder =
-          FirestoreOptions.newBuilder()
-              .setOpenTelemetryOptions(
-                  FirestoreOpenTelemetryOptions.newBuilder()
-                      .setOpenTelemetry(openTelemetrySdk)
-                      .setTracingEnabled(true)
-                      .build());
+          optionsBuilder.setOpenTelemetryOptions(
+              FirestoreOpenTelemetryOptions.newBuilder()
+                  .setOpenTelemetry(openTelemetrySdk)
+                  .build());
     }
 
     String namedDb = System.getProperty("FIRESTORE_NAMED_DATABASE");
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITTracingTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITTracingTest.java
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/telemetry/EnabledTraceUtilTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/telemetry/EnabledTraceUtilTest.java
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/telemetry/TraceUtilTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/telemetry/TraceUtilTest.java
