fix(spanner): derive built-in metrics project from database client#13262
fix(spanner): derive built-in metrics project from database client#13262rahul2393 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the built-in metrics initialization and export logic to use a project ID supplier, ensuring that metrics are correctly associated with the active database project. Key changes include updating SpannerCloudMonitoringExporter to override resource project IDs during export and integrating metric initialization into SpannerImpl and SpannerOptions. Reviewer feedback identifies dead code in the exporter, suggests optimizing convertToSpannerTimeSeries to accept a Collection to avoid redundant list copies, and points out a test case that needs a non-empty collection to properly validate export skipping behavior.
There was a problem hiding this comment.
Code Review
This pull request refactors the initialization and export of built-in metrics in the Spanner client by introducing a project ID supplier and triggering initialization upon client access. The logic for filtering metrics based on project ID mismatches in resource attributes has been removed, with the project ID now applied directly to all exported metrics. Review feedback suggests enhancing performance by using Collection instead of List in convertToSpannerTimeSeries to avoid redundant object allocations. Furthermore, the reviewer recommended re-evaluating the lastExportSkippedData flag, as the removal of filtering logic has rendered its current implementation potentially obsolete or incorrect.
| BuiltInMetricsView.registerBuiltinMetrics( | ||
| SpannerCloudMonitoringExporter.create( | ||
| projectId, credentials, monitoringHost, universeDomain), | ||
| this::getProjectId, credentials, monitoringHost, universeDomain), |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
So flow is:
SpannerClientinit → OpenTelemetry/exporter may be created, project supplier returnsnull- no export happens yet because no database project is known
getDatabaseClient(DatabaseId)→ database project is set once- future metric exports use that database project
| BuiltInMetricsView.registerBuiltinMetrics( | ||
| SpannerCloudMonitoringExporter.create( | ||
| projectId, credentials, monitoringHost, universeDomain), | ||
| this::getProjectId, credentials, monitoringHost, universeDomain), |
There was a problem hiding this comment.
We are also passing the projectId in next line to create OpenTelemetry Resource
There was a problem hiding this comment.
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.
Fixes #13240
Fix built-in metrics export project selection for shared VPC / host-project environments.
Built-in metrics can initialize before any
DatabaseClient, causing the Cloud Monitoring exporter to useSpannerOptions.getProjectId()instead of the Spanner database project. This makescreateServiceTimeSeriestarget the wrong project.Changes:
DatabaseIdused bygetDatabaseClient()orgetBatchClient()project_idduring export because OTelResourceis immutable