Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.cloud.opentelemetry.detection.AttributeKeys;
import com.google.cloud.opentelemetry.detection.DetectedPlatform;
import com.google.cloud.opentelemetry.detection.GCPPlatformDetector;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.hash.HashFunction;
import com.google.common.hash.Hashing;
Expand Down Expand Up @@ -75,10 +76,12 @@ final class BuiltInMetricsProvider {
private static final String default_location = "global";

private OpenTelemetry openTelemetry;
private String projectId;
private boolean mismatchedProjectIdLogged;

private BuiltInMetricsProvider() {}

OpenTelemetry getOrCreateOpenTelemetry(
synchronized OpenTelemetry getOrCreateOpenTelemetry(
String projectId,
@Nullable Credentials credentials,
@Nullable String monitoringHost,
Expand All @@ -88,7 +91,7 @@ OpenTelemetry getOrCreateOpenTelemetry(
SdkMeterProviderBuilder sdkMeterProviderBuilder = SdkMeterProvider.builder();
BuiltInMetricsView.registerBuiltinMetrics(
SpannerCloudMonitoringExporter.create(
projectId, credentials, monitoringHost, universeDomain),
this::getProjectId, credentials, monitoringHost, universeDomain),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahul2393 I did not understand this solution. getOrCreateOpenTelemetry is called from GapicSpannerRPC while creating SpannerClient. At the time projectId shared here could be the projectId of GKE instance for example.

So in this case we will be initialising SpannerCloudMonitoringExporter with null projectId ? As by this time setProjectIdIfAbsent won't be called, it is called later during database init.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So flow is:

  1. SpannerClient init → OpenTelemetry/exporter may be created, project supplier returns null
  2. no export happens yet because no database project is known
  3. getDatabaseClient(DatabaseId) → database project is set once
  4. future metric exports use that database project

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are also passing the projectId in next line to create OpenTelemetry Resource

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also do

monitoredResourceBuilder.putLabels(PROJECT_ID_KEY.getKey(), projectId);

So the resource created during SDK initialization may contain the early/default project, but before sending
createServiceTimeSeries, we overwrite the monitored resource label with the database project from the exporter supplier.

sdkMeterProviderBuilder);
sdkMeterProviderBuilder.setResource(Resource.create(createResourceAttributes(projectId)));
SdkMeterProvider sdkMeterProvider = sdkMeterProviderBuilder.build();
Expand All @@ -106,6 +109,39 @@ OpenTelemetry getOrCreateOpenTelemetry(
}
}

synchronized void setProjectIdIfAbsent(String projectId) {
if (this.projectId == null) {
this.projectId = projectId;
} else if (!this.projectId.equals(projectId) && !mismatchedProjectIdLogged) {
mismatchedProjectIdLogged = true;
logger.log(
Level.WARNING,
"Built-in metrics are already initialized for project {0}. Metrics for project {1} will"
+ " be exported using the existing project.",
new Object[] {this.projectId, projectId});
}
}

@Nullable
synchronized OpenTelemetry getOpenTelemetry() {
return this.openTelemetry;
}

@VisibleForTesting
synchronized String getProjectId() {
return this.projectId;
}

@VisibleForTesting
synchronized void reset() {
if (this.openTelemetry instanceof OpenTelemetrySdk) {
((OpenTelemetrySdk) this.openTelemetry).getSdkMeterProvider().close();
}
this.openTelemetry = null;
this.projectId = null;
this.mismatchedProjectIdLogged = false;
}

// TODO: Remove when
// https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/issues/421
// has been fixed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,15 @@
import io.opentelemetry.sdk.metrics.data.AggregationTemporality;
import io.opentelemetry.sdk.metrics.data.MetricData;
import io.opentelemetry.sdk.metrics.export.MetricExporter;
import io.opentelemetry.sdk.resources.Resource;
import java.io.IOException;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Supplier;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

Expand All @@ -70,16 +69,24 @@ class SpannerCloudMonitoringExporter implements MetricExporter {
// https://cloud.google.com/monitoring/quotas#custom_metrics_quotas.
private static final int EXPORT_BATCH_SIZE_LIMIT = 200;
private final AtomicBoolean spannerExportFailureLogged = new AtomicBoolean(false);
private final AtomicBoolean lastExportSkippedData = new AtomicBoolean(false);
private final MetricServiceClient client;
private final String spannerProjectId;
private final Supplier<String> spannerProjectIdSupplier;

static SpannerCloudMonitoringExporter create(
String projectId,
@Nullable Credentials credentials,
@Nullable String monitoringHost,
String universeDomain)
throws IOException {
return create(() -> projectId, credentials, monitoringHost, universeDomain);
}

static SpannerCloudMonitoringExporter create(
Supplier<String> projectIdSupplier,
@Nullable Credentials credentials,
@Nullable String monitoringHost,
String universeDomain)
throws IOException {
MetricServiceSettings.Builder settingsBuilder = MetricServiceSettings.newBuilder();
CredentialsProvider credentialsProvider;
if (credentials == null || credentials instanceof NoCredentials) {
Expand Down Expand Up @@ -114,13 +121,18 @@ static SpannerCloudMonitoringExporter create(
settingsBuilder.createServiceTimeSeriesSettings().setSimpleTimeoutNoRetriesDuration(timeout);

return new SpannerCloudMonitoringExporter(
projectId, MetricServiceClient.create(settingsBuilder.build()));
projectIdSupplier, MetricServiceClient.create(settingsBuilder.build()));
}

@VisibleForTesting
SpannerCloudMonitoringExporter(String projectId, MetricServiceClient client) {
this(() -> projectId, client);
}

@VisibleForTesting
SpannerCloudMonitoringExporter(Supplier<String> projectIdSupplier, MetricServiceClient client) {
this.client = client;
this.spannerProjectId = projectId;
this.spannerProjectIdSupplier = projectIdSupplier;
}

@Override
Expand All @@ -140,37 +152,20 @@ MetricServiceClient getMetricServiceClient() {

/** Export client built in metrics */
private CompletableResultCode exportSpannerClientMetrics(Collection<MetricData> collection) {
// Filter spanner metrics. Only include metrics that contain a valid project.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can safely remove this code when using this strategy. Or technically you can, but this then changes the current behavior of clients that use multiple different projects, which I don't think is a side-effect that we want from this fix. Previously, metrics with mismatched project IDs would be filtered out and not exported. Now, they are all set to whatever project ID is used by the first DatabaseClient that is created and then exported. I think that the latter is a degradation from the previous behavior, where they were dropped, as now you risk getting metrics from a 'wrong' database into a different project.

An alternative to setting a fixed project ID that is used for all metrics, is to dynamically collect and then batch export the metrics per project ID (which in the vast majority of cases would be just one project). That would remove the requirement to try to set a project ID the first time a DatabaseClient is created, which would simplify the code a bit. See here for an example: https://github.com/googleapis/google-cloud-java/compare/spanner-export-metrics-per-project (Note: That sample has not been further refined, so it might need a bit of polishing before it is ready to use)

The up and downsides of the strategy in https://github.com/googleapis/google-cloud-java/compare/spanner-export-metrics-per-project are:

  • Upside: It correctly exports Spanner metrics to the correct project, even when a client creates multiple DatabaseClients with different project IDs.
  • Downside: It uses the default Project ID of the environment (so for example the GKE project ID) for core gRPC metrics that are not handled by the Spanner interceptor.

The downside mentioned above could partly be mitigated by combining it with the strategy in this pull request, and dynamically setting the project ID that is used for non-Spanner metrics to the project ID of the first DatabaseClient.

List<MetricData> spannerMetricData = collection.stream().collect(Collectors.toList());

// Log warnings for metrics that will be skipped.
boolean mustFilter = false;
if (spannerMetricData.stream()
.map(metricData -> metricData.getResource())
.anyMatch(this::shouldSkipPointDataDueToProjectId)) {
logger.log(
Level.WARNING, "Some metric data contain a different projectId. These will be skipped.");
mustFilter = true;
}

if (mustFilter) {
spannerMetricData =
spannerMetricData.stream()
.filter(this::shouldSkipMetricData)
.collect(Collectors.toList());
String spannerProjectId = spannerProjectIdSupplier.get();
if (Strings.isNullOrEmpty(spannerProjectId)) {
return CompletableResultCode.ofSuccess();
}
lastExportSkippedData.set(mustFilter);

// Skips exporting if there's none
if (spannerMetricData.isEmpty()) {
if (collection.isEmpty()) {
return CompletableResultCode.ofSuccess();
}

List<TimeSeries> spannerTimeSeries;
try {
spannerTimeSeries =
SpannerCloudMonitoringExporterUtils.convertToSpannerTimeSeries(
spannerMetricData, this.spannerProjectId);
collection, spannerProjectId);
} catch (Throwable e) {
logger.log(
Level.WARNING,
Expand Down Expand Up @@ -218,18 +213,6 @@ public void onSuccess(List<Empty> empty) {
return spannerExportCode;
}

private boolean shouldSkipMetricData(MetricData metricData) {
return shouldSkipPointDataDueToProjectId(metricData.getResource());
}

private boolean shouldSkipPointDataDueToProjectId(Resource resource) {
return !spannerProjectId.equals(SpannerCloudMonitoringExporterUtils.getProjectId(resource));
}

boolean lastExportSkippedData() {
return this.lastExportSkippedData.get();
}

private ApiFuture<List<Empty>> exportTimeSeriesInBatch(
ProjectName projectName, List<TimeSeries> timeSeries) {
List<ApiFuture<Empty>> batchResults = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@
import io.opentelemetry.sdk.metrics.data.MetricDataType;
import io.opentelemetry.sdk.metrics.data.PointData;
import io.opentelemetry.sdk.metrics.data.SumData;
import io.opentelemetry.sdk.resources.Resource;
import java.util.ArrayList;
Comment thread
rahul2393 marked this conversation as resolved.
import java.util.Collection;
import java.util.List;
Comment thread
rahul2393 marked this conversation as resolved.
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -75,12 +75,8 @@ class SpannerCloudMonitoringExporterUtils {

private SpannerCloudMonitoringExporterUtils() {}

static String getProjectId(Resource resource) {
return resource.getAttributes().get(PROJECT_ID_KEY);
}

static List<TimeSeries> convertToSpannerTimeSeries(
List<MetricData> collection, String projectId) {
Collection<MetricData> collection, String projectId) {
List<TimeSeries> allTimeSeries = new ArrayList<>();

for (MetricData metricData : collection) {
Expand All @@ -102,6 +98,7 @@ static List<TimeSeries> convertToSpannerTimeSeries(
monitoredResourceBuilder.putLabels(
key.getKey(), String.valueOf(resourceAttributes.get(key)));
}
monitoredResourceBuilder.putLabels(PROJECT_ID_KEY.getKey(), projectId);

metricData.getData().getPoints().stream()
.map(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ public DatabaseClient getDatabaseClient(DatabaseId db) {
if (clientId == null) {
clientId = nextDatabaseClientId(db);
}
getOptions().initializeBuiltInMetrics(db);
MultiplexedSessionDatabaseClient multiplexedSessionDatabaseClient =
new MultiplexedSessionDatabaseClient(SpannerImpl.this.getSessionClient(db));
DatabaseClientImpl dbClient =
Expand Down Expand Up @@ -337,6 +338,7 @@ public BatchClient getBatchClient(DatabaseId db) {
if (this.dbBatchClients.containsKey(db)) {
return this.dbBatchClients.get(db);
}
getOptions().initializeBuiltInMetrics(db);
BatchClientImpl batchClient = new BatchClientImpl(getSessionClient(db));
this.dbBatchClients.put(db, batchClient);
return batchClient;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2520,6 +2520,17 @@ public void enablegRPCMetrics(InstantiatingGrpcChannelProvider.Builder channelPr
}
}

void initializeBuiltInMetrics(DatabaseId databaseId) {
if (isEnableBuiltInMetrics() && !usesNoCredentials()) {
this.builtInMetricsProvider.setProjectIdIfAbsent(databaseId.getInstanceId().getProject());
this.builtInMetricsProvider.getOrCreateOpenTelemetry(
databaseId.getInstanceId().getProject(),
getCredentials(),
this.monitoringHost,
getUniverseDomain());
}
}

public ApiTracerFactory getApiTracerFactory(boolean isAdminClient, boolean isEmulatorEnabled) {
return createApiTracerFactory(isAdminClient, isEmulatorEnabled);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,31 @@
package com.google.cloud.spanner;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import com.google.auth.oauth2.AccessToken;
import com.google.auth.oauth2.OAuth2Credentials;
import java.util.Date;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class BuiltInOpenTelemetryMetricsProviderTest {

@Before
public void setUp() {
BuiltInMetricsProvider.INSTANCE.reset();
}

@After
public void tearDown() {
BuiltInMetricsProvider.INSTANCE.reset();
}

@Test
public void testGenerateClientHashWithSimpleUid() {
String clientUid = "testClient";
Expand Down Expand Up @@ -56,11 +72,46 @@ public void testGenerateClientHashWithSpecialCharacters() {
verifyHash(BuiltInMetricsProvider.generateClientHash(clientUid));
}

@Test
public void testApiTracerFactoryDoesNotSetBuiltInMetricsProject() {
SpannerOptions options = newTestOptions();

options.getApiTracerFactory(/* isAdminClient= */ false, /* isEmulatorEnabled= */ false);

assertNull(BuiltInMetricsProvider.INSTANCE.getProjectId());
}

@Test
public void testBuiltInOpenTelemetryDoesNotSetMetricsProject() {
SpannerOptions options = newTestOptions();

options.getBuiltInOpenTelemetry();

assertNull(BuiltInMetricsProvider.INSTANCE.getProjectId());
}

@Test
public void testInitializeBuiltInMetricsUsesDatabaseProject() {
SpannerOptions options = newTestOptions();

options.initializeBuiltInMetrics(DatabaseId.of("database-project", "i", "d"));

assertEquals("database-project", BuiltInMetricsProvider.INSTANCE.getProjectId());
}

private void verifyHash(String hash) {
// Check if the hash length is 6
assertEquals(hash.length(), 6);
// Check if the hash is in the range [000000, 0003ff]
long hashValue = Long.parseLong(hash, 16); // Convert hash from hex to decimal
assertTrue(hashValue >= 0 && hashValue <= 0x3FF);
}

private SpannerOptions newTestOptions() {
return SpannerOptions.newBuilder()
.setProjectId("host-project")
.setCredentials(
OAuth2Credentials.create(new AccessToken("test-token", new Date(Long.MAX_VALUE))))
.build();
}
}
Loading
Loading