Skip to content

Commit 712d57e

Browse files
ehsannasehsannas
authored
fix: Drop the setTracingEnabled flag from Options (@BetaApi change) (#1869)
* 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.
1 parent d466ef0 commit 712d57e

File tree

12 files changed

+136
-191
lines changed

12 files changed

+136
-191
lines changed

‎google-cloud-firestore/clirr-ignored-differences.xml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,18 @@
314314
<method>com.google.cloud.firestore.Query getQuery()</method>
315315
</difference>
316316

317+
<!-- Removed an OpenTelemetry Tracing Beta Api -->
318+
<difference>
319+
<differenceType>7002</differenceType>
320+
<className>com/google/cloud/firestore/FirestoreOpenTelemetryOptions</className>
321+
<method>boolean isTracingEnabled()</method>
322+
</difference>
323+
<difference>
324+
<differenceType>7002</differenceType>
325+
<className>com/google/cloud/firestore/FirestoreOpenTelemetryOptions$Builder</className>
326+
<method>com.google.cloud.firestore.FirestoreOpenTelemetryOptions$Builder setTracingEnabled(boolean)</method>
327+
</difference>
328+
317329
<difference>
318330
<className>com/google/cloud/firestore/StreamableQuery</className>
319331
<differenceType>7009</differenceType>

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,14 +384,15 @@ public void onError(Throwable throwable) {
384384
.currentSpan()
385385
.addEvent(
386386
METHOD_NAME_RUN_AGGREGATION_QUERY + ": Retryable Error",
387-
Collections.singletonMap("error.message", throwable.getMessage()));
387+
Collections.singletonMap("error.message", throwable.toString()));
388+
388389
runQuery(responseDeliverer, attempt + 1);
389390
} else {
390391
getTraceUtil()
391392
.currentSpan()
392393
.addEvent(
393394
METHOD_NAME_RUN_AGGREGATION_QUERY + ": Error",
394-
Collections.singletonMap("error.message", throwable.getMessage()));
395+
Collections.singletonMap("error.message", throwable.toString()));
395396
responseDeliverer.deliverError(throwable);
396397
}
397398
}

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOpenTelemetryOptions.java‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -27,28 +27,15 @@
2727
*/
2828
@BetaApi
2929
public class FirestoreOpenTelemetryOptions {
30-
private final boolean tracingEnabled;
3130
private final boolean exportBuiltinMetricsToGoogleCloudMonitoring;
3231
private final @Nullable OpenTelemetry openTelemetry;
3332

3433
FirestoreOpenTelemetryOptions(Builder builder) {
35-
this.tracingEnabled = builder.tracingEnabled;
3634
this.exportBuiltinMetricsToGoogleCloudMonitoring =
3735
builder.exportBuiltinMetricsToGoogleCloudMonitoring;
3836
this.openTelemetry = builder.openTelemetry;
3937
}
4038

41-
/**
42-
* @deprecated This method will be removed in the next minor version update. Please use a no-op
43-
* TracerProvider or set the environment variable `FIRESTORE_ENABLE_TRACING=OFF` to disable
44-
* tracing. If the GlobalOpenTelemetry or the OpenTelemetry instance passed to Firestore
45-
* contain a valid TracerProvider, the Firestore client will generate spans by utilizing it.
46-
*/
47-
@Deprecated
48-
public boolean isTracingEnabled() {
49-
return tracingEnabled;
50-
}
51-
5239
public boolean exportBuiltinMetricsToGoogleCloudMonitoring() {
5340
return exportBuiltinMetricsToGoogleCloudMonitoring;
5441
}
@@ -68,20 +55,16 @@ public static FirestoreOpenTelemetryOptions.Builder newBuilder() {
6855
}
6956

7057
public static class Builder {
71-
72-
private boolean tracingEnabled;
7358
private boolean exportBuiltinMetricsToGoogleCloudMonitoring;
7459
@Nullable private OpenTelemetry openTelemetry;
7560

7661
private Builder() {
77-
tracingEnabled = false;
7862
// TODO(metrics): default this to true when feature is ready
7963
exportBuiltinMetricsToGoogleCloudMonitoring = false;
8064
openTelemetry = null;
8165
}
8266

8367
private Builder(FirestoreOpenTelemetryOptions options) {
84-
this.tracingEnabled = options.tracingEnabled;
8568
this.exportBuiltinMetricsToGoogleCloudMonitoring =
8669
options.exportBuiltinMetricsToGoogleCloudMonitoring;
8770
this.openTelemetry = options.openTelemetry;
@@ -92,23 +75,6 @@ public FirestoreOpenTelemetryOptions build() {
9275
return new FirestoreOpenTelemetryOptions(this);
9376
}
9477

95-
/**
96-
* Sets whether tracing should be enabled.
97-
*
98-
* @param tracingEnabled Whether tracing should be enabled.
99-
* @deprecated This method will be removed in the next minor version update. Please use a no-op
100-
* TracerProvider or set the environment variable `FIRESTORE_ENABLE_TRACING=OFF` to disable
101-
* tracing. If the GlobalOpenTelemetry or the OpenTelemetry instance passed to Firestore
102-
* contains a valid TracerProvider, the Firestore client will generate spans by utilizing
103-
* it.
104-
*/
105-
@Deprecated
106-
@Nonnull
107-
public FirestoreOpenTelemetryOptions.Builder setTracingEnabled(boolean tracingEnabled) {
108-
this.tracingEnabled = tracingEnabled;
109-
return this;
110-
}
111-
11278
// TODO(metrics): make this public when feature is ready.
11379
/**
11480
* Sets whether built-in metrics should be exported to Google Cloud Monitoring

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/StreamableQuery.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,7 @@ public void onError(Throwable throwable) {
378378
if (isRetryableWithCursor() && shouldRetry(cursor, throwable)) {
379379
currentSpan.addEvent(
380380
TelemetryConstants.METHOD_NAME_RUN_QUERY + ": Retryable Error",
381-
Collections.singletonMap("error.message", throwable.getMessage()));
381+
Collections.singletonMap("error.message", throwable.toString()));
382382

383383
startAfter(cursor)
384384
.internalStream(
@@ -391,7 +391,7 @@ public void onError(Throwable throwable) {
391391
} else {
392392
currentSpan.addEvent(
393393
TelemetryConstants.METHOD_NAME_RUN_QUERY + ": Error",
394-
Collections.singletonMap("error.message", throwable.getMessage()));
394+
Collections.singletonMap("error.message", throwable.toString()));
395395
streamResponseObserver.onError(throwable);
396396
}
397397
}

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/DisabledTraceUtil.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.api.core.ApiFunction;
2020
import com.google.api.core.ApiFuture;
21+
import com.google.api.core.InternalApi;
2122
import io.grpc.ManagedChannelBuilder;
2223
import java.util.Map;
2324
import javax.annotation.Nonnull;
@@ -27,6 +28,7 @@
2728
* A no-op implementation of {@link MetricsUtil} that does not collect or export any metrics and has
2829
* near-zero overhead.
2930
*/
31+
@InternalApi
3032
public class DisabledTraceUtil implements TraceUtil {
3133

3234
static class Span implements TraceUtil.Span {

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/EnabledTraceUtil.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.api.core.ApiFuture;
2121
import com.google.api.core.ApiFutureCallback;
2222
import com.google.api.core.ApiFutures;
23+
import com.google.api.core.InternalApi;
2324
import com.google.cloud.firestore.FirestoreOptions;
2425
import com.google.common.base.Throwables;
2526
import io.grpc.ManagedChannelBuilder;
@@ -31,6 +32,7 @@
3132
import io.opentelemetry.api.trace.SpanKind;
3233
import io.opentelemetry.api.trace.StatusCode;
3334
import io.opentelemetry.api.trace.Tracer;
35+
import io.opentelemetry.api.trace.TracerProvider;
3436
import io.opentelemetry.instrumentation.grpc.v1_6.GrpcTelemetry;
3537
import java.util.Map;
3638
import javax.annotation.Nonnull;
@@ -40,6 +42,7 @@
4042
* A utility class that uses OpenTelemetry for trace collection. `FirestoreOpenTelemetryOptions` in
4143
* `FirestoreOptions` can be used to configure its behavior.
4244
*/
45+
@InternalApi
4346
public class EnabledTraceUtil implements TraceUtil {
4447
private final Tracer tracer;
4548
private final OpenTelemetry openTelemetry;
@@ -48,8 +51,7 @@ public class EnabledTraceUtil implements TraceUtil {
4851
EnabledTraceUtil(FirestoreOptions firestoreOptions) {
4952
OpenTelemetry openTelemetry = firestoreOptions.getOpenTelemetryOptions().getOpenTelemetry();
5053

51-
// If tracing is enabled, but an OpenTelemetry instance is not provided, fall back
52-
// to using GlobalOpenTelemetry.
54+
// If an OpenTelemetry instance is not provided, fall back to using GlobalOpenTelemetry.
5355
if (openTelemetry == null) {
5456
openTelemetry = GlobalOpenTelemetry.get();
5557
}
@@ -84,6 +86,11 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder managedChannelBuilder)
8486
@Override
8587
@Nullable
8688
public ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> getChannelConfigurator() {
89+
// Note: using `==` rather than `.equals` since OpenTelemetry has only 1 static instance of
90+
// `TracerProvider.noop`.
91+
if (openTelemetry.getTracerProvider() == TracerProvider.noop()) {
92+
return null;
93+
}
8794
return new OpenTelemetryGrpcChannelConfigurator();
8895
}
8996

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/telemetry/TraceUtil.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.api.core.ApiFunction;
2020
import com.google.api.core.ApiFuture;
21+
import com.google.api.core.InternalApi;
2122
import com.google.cloud.firestore.FirestoreOptions;
2223
import io.grpc.ManagedChannelBuilder;
2324
import java.util.Map;
@@ -50,8 +51,9 @@ public interface TraceUtil {
5051
* TraceUtil.
5152
* @return An instance of the TraceUtil class.
5253
*/
54+
@InternalApi
5355
static TraceUtil getInstance(@Nonnull FirestoreOptions firestoreOptions) {
54-
boolean createEnabledInstance = firestoreOptions.getOpenTelemetryOptions().isTracingEnabled();
56+
boolean createEnabledInstance = true;
5557

5658
// The environment variable can override options to enable/disable telemetry collection.
5759
String enableTracingEnvVar = System.getenv(ENABLE_TRACING_ENV_VAR);

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/OpenTelemetryOptionsTest.java‎

Lines changed: 36 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,12 @@
1818

1919
import static com.google.common.truth.Truth.assertThat;
2020

21-
import com.google.cloud.firestore.telemetry.DisabledTraceUtil;
2221
import com.google.cloud.firestore.telemetry.EnabledTraceUtil;
2322
import io.opentelemetry.api.GlobalOpenTelemetry;
24-
import io.opentelemetry.api.OpenTelemetry;
23+
import io.opentelemetry.api.trace.TracerProvider;
2524
import io.opentelemetry.sdk.OpenTelemetrySdk;
25+
import io.opentelemetry.sdk.resources.Resource;
26+
import io.opentelemetry.sdk.trace.SdkTracerProvider;
2627
import javax.annotation.Nullable;
2728
import org.junit.After;
2829
import org.junit.Before;
@@ -49,112 +50,67 @@ FirestoreOptions.Builder getBaseOptions() {
4950
}
5051

5152
@Test
52-
public void defaultOptionsDisablesTelemetryCollection() {
53+
public void defaultOptionsUsesEnabledTraceUtilWithNoopOpenTelemetry() {
5354
FirestoreOptions firestoreOptions = getBaseOptions().build();
5455
firestore = firestoreOptions.getService();
55-
assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isFalse();
5656
assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry()).isNull();
5757
assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
58-
assertThat(firestore.getOptions().getTraceUtil() instanceof DisabledTraceUtil).isTrue();
59-
}
60-
61-
@Test
62-
public void canEnableTelemetryCollectionWithoutOpenTelemetryInstance() {
63-
FirestoreOptions firestoreOptions =
64-
getBaseOptions()
65-
.setOpenTelemetryOptions(
66-
FirestoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build())
67-
.build();
68-
firestore = firestoreOptions.getService();
69-
assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isTrue();
70-
assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry()).isNull();
71-
assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
72-
assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
73-
}
74-
75-
@Test
76-
public void canEnableTelemetryCollectionWithOpenTelemetryInstance() {
77-
OpenTelemetry openTelemetry = GlobalOpenTelemetry.get();
78-
FirestoreOptions firestoreOptions =
79-
getBaseOptions()
80-
.setOpenTelemetryOptions(
81-
FirestoreOpenTelemetryOptions.newBuilder()
82-
.setTracingEnabled(true)
83-
.setOpenTelemetry(openTelemetry)
84-
.build())
85-
.build();
86-
firestore = firestoreOptions.getService();
87-
assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isTrue();
88-
assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry())
89-
.isEqualTo(openTelemetry);
90-
assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
9158
assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
59+
EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
60+
// Assert that a Noop tracer provider is used by default.
61+
assertThat(enabledTraceUtil.getOpenTelemetry().getTracerProvider())
62+
.isSameInstanceAs(TracerProvider.noop());
9263
}
9364

9465
@Test
95-
public void canDisableTelemetryCollectionWhileOpenTelemetryInstanceIsNotNull() {
96-
OpenTelemetry openTelemetry = GlobalOpenTelemetry.get();
97-
FirestoreOptions firestoreOptions =
98-
getBaseOptions()
99-
.setOpenTelemetryOptions(
100-
FirestoreOpenTelemetryOptions.newBuilder()
101-
.setTracingEnabled(false)
102-
.setOpenTelemetry(openTelemetry)
103-
.build())
104-
.build();
105-
firestore = firestoreOptions.getService();
106-
assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isFalse();
107-
assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry())
108-
.isEqualTo(openTelemetry);
109-
assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
110-
assertThat(firestore.getOptions().getTraceUtil() instanceof DisabledTraceUtil).isTrue();
111-
}
66+
public void existenceOfGlobalOpenTelemetryEnablesTracingWithTheGlobalTracerProvider() {
67+
// Make a custom TracerProvider.
68+
Resource resource =
69+
Resource.getDefault().merge(Resource.builder().put("service.name", "test").build());
70+
SdkTracerProvider myTracerProvider = SdkTracerProvider.builder().setResource(resource).build();
11271

113-
@Test
114-
public void existenceOfGlobalOpenTelemetryDoesNotEnableTracing() {
115-
// Register a global OpenTelemetry SDK.
116-
OpenTelemetrySdk.builder().buildAndRegisterGlobal();
72+
// Register a GlobalOpenTelemetry with the custom tracer provider.
73+
OpenTelemetrySdk.builder().setTracerProvider(myTracerProvider).buildAndRegisterGlobal();
11774

118-
// Make sure Firestore does not use GlobalOpenTelemetry by default.
75+
// Do NOT pass an OpenTelemetry instance to Firestore.
11976
FirestoreOptions firestoreOptions = getBaseOptions().build();
12077
firestore = firestoreOptions.getService();
121-
assertThat(firestore.getOptions().getOpenTelemetryOptions().isTracingEnabled()).isFalse();
78+
79+
// An OpenTelemetry instance has not been set in options.
12280
assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry()).isNull();
81+
82+
// Assert that tracing is enabled and is using the custom tracer provider from the
83+
// GlobalOpenTelemetry.
12384
assertThat(firestore.getOptions().getTraceUtil()).isNotNull();
124-
assertThat(firestore.getOptions().getTraceUtil() instanceof DisabledTraceUtil).isTrue();
85+
assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
86+
EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
87+
assertThat(enabledTraceUtil.getOpenTelemetry().getTracerProvider().equals(myTracerProvider));
12588
}
12689

12790
@Test
12891
public void canPassOpenTelemetrySdkInstanceToFirestore() {
129-
OpenTelemetrySdk myOpenTelemetrySdk = OpenTelemetrySdk.builder().build();
92+
// Make a custom TracerProvider and make an OpenTelemetry instance with it.
93+
Resource resource =
94+
Resource.getDefault().merge(Resource.builder().put("service.name", "test").build());
95+
SdkTracerProvider myTracerProvider = SdkTracerProvider.builder().setResource(resource).build();
96+
OpenTelemetrySdk myOpenTelemetrySdk =
97+
OpenTelemetrySdk.builder().setTracerProvider(myTracerProvider).build();
98+
99+
// Pass it to Firestore.
130100
FirestoreOptions firestoreOptions =
131101
getBaseOptions()
132102
.setOpenTelemetryOptions(
133103
FirestoreOpenTelemetryOptions.newBuilder()
134-
.setTracingEnabled(true)
135104
.setOpenTelemetry(myOpenTelemetrySdk)
136105
.build())
137106
.build();
138107
firestore = firestoreOptions.getService();
108+
assertThat(firestore.getOptions().getOpenTelemetryOptions().getOpenTelemetry())
109+
.isEqualTo(myOpenTelemetrySdk);
110+
assertThat(firestore.getOptions().getTraceUtil() instanceof EnabledTraceUtil).isTrue();
139111
EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
140112
assertThat(enabledTraceUtil).isNotNull();
141113
assertThat(enabledTraceUtil.getOpenTelemetry()).isEqualTo(myOpenTelemetrySdk);
142-
}
143-
144-
@Test
145-
public void usesGlobalOpenTelemetryIfOpenTelemetryNotProvidedInOptions() {
146-
// Register a global OpenTelemetry SDK.
147-
OpenTelemetrySdk.builder().buildAndRegisterGlobal();
148-
149-
// Do _not_ pass it to FirestoreOptions.
150-
FirestoreOptions firestoreOptions =
151-
getBaseOptions()
152-
.setOpenTelemetryOptions(
153-
FirestoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build())
154-
.build();
155-
firestore = firestoreOptions.getService();
156-
EnabledTraceUtil enabledTraceUtil = (EnabledTraceUtil) firestore.getOptions().getTraceUtil();
157-
assertThat(enabledTraceUtil).isNotNull();
158-
assertThat(enabledTraceUtil.getOpenTelemetry()).isEqualTo(GlobalOpenTelemetry.get());
114+
assertThat(enabledTraceUtil.getOpenTelemetry().getTracerProvider().equals(myTracerProvider));
159115
}
160116
}

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITE2ETracingTest.java‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -319,20 +319,13 @@ public void before() throws Exception {
319319
// Initialize the Firestore DB w/ the OTel SDK. Ideally we'd do this is the @BeforeAll method
320320
// but because gRPC traces need to be deterministically force-flushed, firestore.shutdown()
321321
// must be called in @After for each test.
322-
FirestoreOptions.Builder optionsBuilder;
323-
if (isUsingGlobalOpenTelemetrySDK()) {
324-
optionsBuilder =
325-
FirestoreOptions.newBuilder()
326-
.setOpenTelemetryOptions(
327-
FirestoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build());
328-
} else {
322+
FirestoreOptions.Builder optionsBuilder = FirestoreOptions.newBuilder();
323+
if (!isUsingGlobalOpenTelemetrySDK()) {
329324
optionsBuilder =
330-
FirestoreOptions.newBuilder()
331-
.setOpenTelemetryOptions(
332-
FirestoreOpenTelemetryOptions.newBuilder()
333-
.setOpenTelemetry(openTelemetrySdk)
334-
.setTracingEnabled(true)
335-
.build());
325+
optionsBuilder.setOpenTelemetryOptions(
326+
FirestoreOpenTelemetryOptions.newBuilder()
327+
.setOpenTelemetry(openTelemetrySdk)
328+
.build());
336329
}
337330

338331
String namedDb = System.getProperty("FIRESTORE_NAMED_DATABASE");

0 commit comments

Comments
 (0)