fix(spanner): derive built-in metrics project from database client #13262
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,13 +42,15 @@ | |
| import io.opentelemetry.sdk.metrics.data.AggregationTemporality; | ||
| import io.opentelemetry.sdk.metrics.data.MetricData; | ||
| import io.opentelemetry.sdk.metrics.export.MetricExporter; | ||
| import java.io.IOException; | ||
| import java.time.Duration; | ||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.function.Supplier; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import java.util.stream.Collectors; | ||
|
|
@@ -69,13 +71,12 @@ class SpannerCloudMonitoringExporter implements MetricExporter { | |
| // This the quota limit from Cloud Monitoring. More details in | ||
| // https://cloud.google.com/monitoring/quotas#custom_metrics_quotas. | ||
| private static final int EXPORT_BATCH_SIZE_LIMIT = 200; | ||
| private final Set<String> spannerExportFailureLoggedProjects = ConcurrentHashMap.newKeySet(); | ||
| private final MetricServiceClient client; | ||
| private final Supplier<String> fallbackProjectIdSupplier; | ||
|
|
||
| static SpannerCloudMonitoringExporter create( | ||
| Supplier<String> fallbackProjectIdSupplier, | ||
| @Nullable Credentials credentials, | ||
| @Nullable String monitoringHost, | ||
| String universeDomain) | ||
|
|
@@ -114,13 +115,19 @@ static SpannerCloudMonitoringExporter create( | |
| settingsBuilder.createServiceTimeSeriesSettings().setSimpleTimeoutNoRetriesDuration(timeout); | ||
|
|
||
| return new SpannerCloudMonitoringExporter( | ||
| fallbackProjectIdSupplier, MetricServiceClient.create(settingsBuilder.build())); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| SpannerCloudMonitoringExporter(MetricServiceClient client) { | ||
| this(() -> null, client); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| SpannerCloudMonitoringExporter( | ||
| Supplier<String> fallbackProjectIdSupplier, MetricServiceClient client) { | ||
| this.client = client; | ||
| this.fallbackProjectIdSupplier = fallbackProjectIdSupplier; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -140,37 +147,16 @@ MetricServiceClient getMetricServiceClient() { | |
|
|
||
| /** Export client built in metrics */ | ||
| private CompletableResultCode exportSpannerClientMetrics(Collection<MetricData> collection) { | ||
|
Contributor
There was a problem hiding this comment. 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. |
||
| // Skips exporting if there's none | ||
| if (collection.isEmpty()) { | ||
| return CompletableResultCode.ofSuccess(); | ||
| } | ||
|
|
||
| List<TimeSeries> spannerTimeSeries; | ||
| try { | ||
| spannerTimeSeries = | ||
| SpannerCloudMonitoringExporterUtils.convertToSpannerTimeSeries( | ||
| collection, fallbackProjectIdSupplier.get()); | ||
| } catch (Throwable e) { | ||
| logger.log( | ||
| Level.WARNING, | ||
|
|
@@ -179,37 +165,60 @@ private CompletableResultCode exportSpannerClientMetrics(Collection<MetricData> | |
| return CompletableResultCode.ofFailure(); | ||
| } | ||
|
|
||
| if (spannerTimeSeries.isEmpty()) { | ||
| return CompletableResultCode.ofSuccess(); | ||
| } | ||
|
|
||
| Map<String, List<TimeSeries>> timeSeriesByProject = | ||
| spannerTimeSeries.stream() | ||
| .collect( | ||
| Collectors.groupingBy( | ||
| timeSeries -> | ||
| timeSeries | ||
| .getResource() | ||
| .getLabelsMap() | ||
| .get(BuiltInMetricsConstant.PROJECT_ID_KEY.getKey()))); | ||
|
|
||
| List<ApiFuture<List<Empty>>> futures = new ArrayList<>(); | ||
| for (Map.Entry<String, List<TimeSeries>> entry : timeSeriesByProject.entrySet()) { | ||
| ProjectName projectName = ProjectName.of(entry.getKey()); | ||
| ApiFuture<List<Empty>> future = exportTimeSeriesInBatch(projectName, entry.getValue()); | ||
| ApiFutures.addCallback( | ||
| future, | ||
| new ApiFutureCallback<List<Empty>>() { | ||
| @Override | ||
| public void onFailure(Throwable throwable) { | ||
| logExportFailure(throwable, projectName); | ||
| } | ||
|
|
||
| @Override | ||
| public void onSuccess(List<Empty> ignored) { | ||
| spannerExportFailureLoggedProjects.remove(projectName.getProject()); | ||
| } | ||
| }, | ||
| MoreExecutors.directExecutor()); | ||
| futures.add(future); | ||
| } | ||
|
|
||
| ApiFuture<List<List<Empty>>> groupedFuture = ApiFutures.allAsList(futures); | ||
| ApiFuture<List<Empty>> futureList = | ||
| ApiFutures.transform( | ||
| groupedFuture, | ||
| groupedResults -> | ||
| groupedResults.stream().flatMap(List::stream).collect(Collectors.toList()), | ||
| MoreExecutors.directExecutor()); | ||
|
|
||
| CompletableResultCode spannerExportCode = new CompletableResultCode(); | ||
| ApiFutures.addCallback( | ||
| futureList, | ||
| new ApiFutureCallback<List<Empty>>() { | ||
| @Override | ||
| public void onFailure(Throwable throwable) { | ||
| spannerExportCode.fail(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onSuccess(List<Empty> empty) { | ||
| spannerExportCode.succeed(); | ||
| } | ||
| }, | ||
|
|
@@ -218,16 +227,22 @@ public void onSuccess(List<Empty> empty) { | |
| return spannerExportCode; | ||
| } | ||
|
|
||
| private void logExportFailure(Throwable throwable, ProjectName projectName) { | ||
| if (spannerExportFailureLoggedProjects.add(projectName.getProject())) { | ||
| String msg = "createServiceTimeSeries request failed for spanner metrics."; | ||
| if (throwable instanceof PermissionDeniedException) { | ||
| msg += | ||
| String.format( | ||
| " Need monitoring metric writer permission on project=%s. Follow" | ||
| + " https://cloud.google.com/spanner/docs/view-manage-client-side-metrics" | ||
| + "#access-client-side-metrics" | ||
| + " to set up permissions", | ||
| projectName.getProject()); | ||
| } else { | ||
| msg += String.format(" project=%s.", projectName.getProject()); | ||
| } | ||
| logger.log(Level.WARNING, msg, throwable); | ||
| } | ||
| } | ||
|
|
||
| private ApiFuture<List<Empty>> exportTimeSeriesInBatch( | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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