-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(spanner): derive built-in metrics project from database client #13262
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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, | ||
|
|
@@ -88,7 +91,7 @@ OpenTelemetry getOrCreateOpenTelemetry( | |
| SdkMeterProviderBuilder sdkMeterProviderBuilder = SdkMeterProvider.builder(); | ||
| BuiltInMetricsView.registerBuiltinMetrics( | ||
| SpannerCloudMonitoringExporter.create( | ||
| projectId, credentials, monitoringHost, universeDomain), | ||
| this::getProjectId, credentials, monitoringHost, universeDomain), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| sdkMeterProviderBuilder); | ||
| sdkMeterProviderBuilder.setResource(Resource.create(createResourceAttributes(projectId))); | ||
| SdkMeterProvider sdkMeterProvider = sdkMeterProviderBuilder.build(); | ||
|
|
@@ -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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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) { | ||
|
|
@@ -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 | ||
|
|
@@ -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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 The up and downsides of the strategy in https://github.com/googleapis/google-cloud-java/compare/spanner-export-metrics-per-project are:
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, | ||
|
|
@@ -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<>(); | ||
|
|
||
There was a problem hiding this comment.
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
projectIdshared 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
setProjectIdIfAbsentwon't be called, it is called later during database init.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So flow is:
SpannerClientinit → OpenTelemetry/exporter may be created, project supplier returnsnullgetDatabaseClient(DatabaseId)→ database project is set once