feat(BuiltInMetrics): Add built in metrics library - #15871
robertvoinescu-work wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Google.Cloud.BuiltInMetrics package, which enables and exports Google Cloud client built-in metrics telemetry via OpenTelemetry, specifically configuring and testing Spanner-related metrics. Feedback on the changes highlights two key issues: first, a critical issue where static configuration of the OAuth2 access token at startup will cause export failures after one hour, which can be resolved by dynamically injecting the token using a custom DelegatingHandler; second, a scoping issue where calling AddView globally without specifying the meter name could cause unexpected side-effects on other metrics, which should be resolved by scoping the views to the Spanner meter.
1a50487 to
69c472f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Google.Cloud.BuiltInMetrics package, which provides OpenTelemetry exporter configuration and setup for Google Cloud client built-in metrics, with an initial implementation for Google Cloud Spanner. The feedback suggests several improvements: resolving a potential authorization issue by calling GetAccessTokenForRequestAsync directly on the scoped credential rather than its underlying credential, adding a null check in AuthHeaderHandler, exposing the AddGoogleCloudBuiltInMetrics and AddGoogleCloudBuiltInMetricsAsync extension methods as public, and catching InvalidArgument RPC exceptions in integration tests to handle initial metric ingestion delays more robustly.
69c472f to
bbc5d66
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new Google.Cloud.BuiltInMetrics package, which configures and exports Google Cloud client built-in metrics (such as Spanner telemetry) to Google Cloud Monitoring using OpenTelemetry and OTLP. It includes the core implementation, unit tests, integration tests, and updates to repository configuration files. The feedback suggests adding a safety check in AuthHeaderHandler to ensure the retrieved access token is not null or empty before setting the authorization header, preventing potential exceptions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Google.Cloud.BuiltInMetrics project to support OpenTelemetry collection and exporting of Google Cloud client built-in metrics, along with corresponding unit and integration tests. Key feedback includes resolving a compilation issue on the net462 target framework by avoiding Environment.ProcessId, exposing the internal extension methods AddGoogleCloudBuiltInMetrics and AddGoogleCloudBuiltInMetricsAsync as public to allow integration with existing builders, and replacing the unnecessary Google.Api.Gax.Grpc dependency with Google.Api.Gax in both the project configuration and apis.json metadata.
8085440 to
6b52859
Compare
| /// <summary> | ||
| /// Defines service-specific metrics configuration for OpenTelemetry collection. | ||
| /// </summary> | ||
| internal interface IServiceMetricsConfiguration |
There was a problem hiding this comment.
The idea is we can eventually extend for any other service that requires built in metrics simply by implementing this and adding a reference in BuiltInMetrics.ConfigureOtlpExporter
6b52859 to
57e3e4a
Compare
1 participant
b/404948213
Test Validation
