Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions google-cloud-firestore/clirr-ignored-differences.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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();
}
Expand Down Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Loading
Loading