Repository navigation
fix: pre-merge review findings (OTel env labels, exporter notice, Close locking, recorder key) - #1195
Merged
Conversation
…se locking, recorder key - Never promote resource attributes to metric labels (WithFilteredResourceAttributes(NoAttributes)). The SDK merges OTEL_SERVICE_NAME / OTEL_RESOURCE_ATTRIBUTES into the resource and the exporter copied service.* onto every series; the API rejected the write and metrics were silently dead in any OTel-instrumented environment. - Pin the exporter to v0.60.0: v0.61.0+ prints a deprecation notice on the standard logger on first use, for every user, once metrics default on. - Close releases d.lock and metricsMu before the (network) flush. - Key recorders by project:region:name, not the full ConnName, so a DNS name and a connection name for one instance share one series. - Bound metadata-server lookups to 2s; make concurrent Close safe; use a schemaless OTel resource.
… e2e in CI; proxy version label - Metric credentials carry the Dialer's universe domain (DetectDefault UniverseDomain + option.WithAuthCredentials); caller token providers are wrapped in auth.Credentials with the configured universe. Without this every export failed the client's universe check outside googleapis.com, with an IAM hint. - The once-per-process warning now names the real fix for a disabled Monitoring API (SERVICE_DISABLED) and for a universe mismatch. - CI runs the e2e tests in package mode so e2e_metrics_test.go executes (it never ran: the scripts listed the three DB test files explicitly). - connector_version is the Auth Proxy's version when embedded in it. - Close the failed SQL Data stream before falling back; README closes the pool before the Dialer; bad-credentials e2e injects via AuthCredentials.
…ctions drains; small review items - The CI service account lives in an auto-provisioned project without the Monitoring API enabled, so its exports failed SERVICE_DISABLED. The e2e test now passes WithQuotaProject(QUOTA_PROJECT) (and the same quota project for the read-back), billing the calls to the e2e project; the skip message names the extra role needed. - TestMetricsE2EExport asserts open_connections is 0 after Close (mutation-checked: dropping the decrement fails the test). - Exemplars off on the meter provider; unit "1" on the three counters; connectionInfoCache's unused bool return removed (restores main's signature); README notes on billing project and hermetic tests.
…n the hint; e2e runs with the production default and fails in CI - metricCredentials passed the Dialer's effective universe (googleapis.com when WithUniverseDomain is unset) to DetectDefault, which overrides the universe_domain in the credential file, so a TPC credential lost its universe and its tokens were aimed at monitoring.googleapis.com. Only an explicit WithUniverseDomain is passed now; the file's value wins otherwise. Tests cover TPC JSON and TPC ADC without WithUniverseDomain. - The SERVICE_DISABLED hint names the project the API reports (the billed consumer), not the instance project. - TestMain turns the built-in default off only when no real instance is configured, so the database e2e tests run with metrics on as users do; TestMetricsE2EExport fails rather than skips when CI=true.
timeSeries.list, unlike the write, requires billing on the consumer project, and the e2e quota project has none; CI's export succeeded but the read-back was rejected and the test skipped. The read-back is billed to the instance's project now, and a read-back permission error also fails (not skips) in CI.
kgala2
approved these changes
Oct 28, 2026
panavenue
merged commit Oct 28, 2026
af545da
into
feature/client-side-metrics
13 of 15 checks passed
panavenue
added a commit
that referenced
this pull request
Oct 29, 2026
… default (BETA) (#1123) * System metrics setup (#1041) * feat: dynamically detect client telemetry attributes and record dialstatus (#1125) * feat: dynamically detect client telemetry attributes and record dial status - Dynamically detect ClientRegion, ComputePlatform, and DatabaseEngineType in telemetry Config - Format ResourceID as [project_name:instance_name] - Upgrade cached MetricRecorder engine type once connection info DBVersion is retrieved - Protect detection helper functions with panic recovery safeguards * style: format dialer.go with gofmt * fix from comment * feat: add support for connection_durations and open_connection_count … (#1129) * feat: add support for connection_durations and open_connection_count metrics * fix from comment * feat: disable built-in metrics by default and add WithBuiltInMetrics opt-in (#1175) Built-in client metrics (cloudsql.googleapis.com/client/connector/*) are now public and publicly writable, so the connector must not emit them unless the caller explicitly opts in. - Replace WithDisableBuiltInMetrics() with WithBuiltInMetrics(). The option was never released on main, so no deprecation path is needed. - Default enableBuiltInMetrics to false. - Only create the Cloud Monitoring client when metrics are enabled, so callers who do not opt in pay no ADC-detection or gRPC cost. - Add unit tests covering the default-off and opt-in paths. * fix: restore userAgent and applicationName on the Dialer; trim connector_version (#1178) The rebase of feature/client-side-metrics onto main dropped the userAgent and applicationName assignments from the Dialer literal. As a result the InstanceClient resource reported application_name="" (even when WithApplicationName was set) and connector_type="go" even when running inside the Auth Proxy. Also trim versionString when used as a metric label: version.txt ends with a newline, which was being exported as connector_version="1.25.3\n". Found by running cmd/metric_prototype against a real instance and inspecting the resulting time series. Adds a regression test. * fix: use the Dialer's credentials for the built-in metrics client, and flush metrics on Close (#1176) * fix: use the Dialer's credentials for the built-in metrics client The metric client was created with no options before credentials were resolved, so the export always used Application Default Credentials and ignored WithTokenSource, WithCredentials, WithCredentialsFile/JSON, the quota project, and the universe domain. - Create the client after credentials are resolved, with the same user agent, quota project, and universe domain as the other API clients. - For Dialer-detected credentials (ADC, credentials JSON/file), re-detect with the monitoring.write scope; for caller-supplied token sources, use them as-is. - Add tests for token provider selection and an end-to-end export to an in-memory Monitoring server. * fix: flush built-in metrics on Dialer.Close and when a recorder is replaced (#1177) * fix: flush built-in metrics on Dialer.Close and when a recorder is replaced Dialer.Close never stopped the metrics exporters, so anything recorded since the last 60s periodic export was lost when the process exited (the common case on Cloud Run). The UNKNOWN-engine recorder created on each first Dial was also never shut down when replaced by the engine-specific one, leaking a periodic reader goroutine per instance. - Add Shutdown to tel.MetricRecorder (provider shutdown; no-op for the null recorder). - Dialer.Close shuts down all recorders under a 3s timeout and closes the metric client, matching the AlloyDB Go Connector. - metricRecorder flushes and shuts down the recorder it replaces. - Add tests for both paths against an in-memory Monitoring server. * fix: do not close the shared metric client when a recorder shuts down The Cloud Monitoring exporter closes the MetricClient it was given on Shutdown. All recorders of a Dialer share one client, so shutting down the replaced UNKNOWN-engine recorder broke exports for its replacement, and on Close only the first recorder's flush could succeed. Wrap the exporter so the periodic reader's final export still runs but the shared client stays open; the Dialer closes it once in Close. Found by running cmd/metric_prototype against a real instance. Tests now assert the replacement recorder can still export and that Close flushes every recorder. * chore: rename sexp to sharedExp and document why the shared-client exporter and metric client placement are needed * chore: fix copyright year in internal/tel (files added in 2026) (#1179) * fix: use lowercase snake_case for all enum-like metric label values (#1180) * fix: use lowercase snake_case for all enum-like metric label values Several label values leaked internal Go constants or env-var detection results in uppercase (PUBLIC, GCE, CLOUD_RUN, MYSQL, UNKNOWN), while the deliberately chosen values (built_in, iam, success, refresh_ahead), the published cloudsql-metrics.yaml descriptors, and the AlloyDB connector all use lowercase snake_case. Cloud Monitoring filters are case sensitive, so this must be settled before the metrics leave BETA. - Add named constants for every value in internal/tel and use them in dialer.go and tests instead of string literals. - instance_ip_type: public, private, psc, auto, sql_data (normalized via IPTypeValue at record time; internal constants unchanged). An unset value now exports as "unknown" rather than "". - compute_platform: gce, gke, cloud_run, app_engine, cloud_functions. - database_engine_type: mysql, postgresql, sqlserver. - connector_type: go, auth_proxy (was cloud-sql-proxy; matches AlloyDB). - client_region lowercased; all unknowns are "unknown". - Add a contract test that pins every value and enforces snake_case. Verified against a real instance: Cloud Monitoring shows compute_platform=gce, database_engine_type=postgresql, instance_ip_type=public. * chore: add per-constant doc comments for exported label values (revive) * feat: add WithComputePlatform option with a closed set of values; add ai_studio (#1181) Applications deployed by Google AI Studio run the Auth Proxy as a Cloud Run sidecar, so runtime detection reports compute_platform=cloud_run and AIS traffic cannot be told apart from other Cloud Run traffic. Add a public ComputePlatform type and WithComputePlatform option so an embedding client (the proxy) can set the label explicitly. The set of values is closed: only the ComputePlatform* constants are accepted, and any other value (including "unknown", wrong case, or free text) makes NewDialer return a ConfigError. This keeps the cardinality of the InstanceClient resource bounded. - internal/tel: PlatformAIStudio = "ai_studio"; IsValidComputePlatform is the single source of truth for the allowed set. - options.go: ComputePlatform type, ComputePlatform* constants, WithComputePlatform with validation. - dialer.go: resolve the platform once in NewDialer (explicit option wins over detection) and store it on the Dialer. - Tests: all valid values accepted; invalid values rejected with ConfigError; default still auto-detects; override reaches the exported monitored resource even when the environment looks like Cloud Run. * fix: report the resolved IP type in instance_ip_type (#1182) AutoIP and SQLData are selection strategies, not IP types: AutoIP picks public then private, SQLData falls back to private, PSC, then public. The instance_ip_type label was set from the requested strategy, so a dial made with AutoIP was exported as "auto" regardless of which network path actually carried the connection. - Add ConnectionInfo.ResolveAddrs, which returns the concrete IP type selected alongside the addresses. Addrs is now a thin wrapper and is unchanged for callers. - In connectInstanceIP, set attrs.IPType from the resolved type before any metric is recorded, so connect_latencies, open_connections, open_connection_count, closed_connection_count and connection_durations all carry the real type. - Tests: table test for ResolveAddrs covering every strategy and fallback; dialer-level test that an AutoIP dial against an instance with only a private IP exports instance_ip_type=private and never "auto". * feat: enable built-in metrics by default; bool option; quiet, backed-off export failures; README (#1185) * feat: make WithBuiltInMetrics take a bool; make export failures quiet and backed off API: WithBuiltInMetrics() becomes WithBuiltInMetrics(enabled bool). The option is not yet released, so this is free now and avoids a second "disable" option (and a no-op "enable") if the default is ever flipped to on in a future minor release; the same option expresses both. Fail-open export: the OpenTelemetry periodic reader passes export errors to the global error handler, whose default prints to stderr on every failed export, i.e. once a minute forever for a user merely missing roles/monitoring.metricWriter. Neither the AlloyDB nor the Spanner Go client overrides this. sharedClientExporter.Export now: - never returns an error to the reader, so nothing reaches the global handler or the application's stderr; - reports failures through the connector's debug logger (a no-op unless the caller opted into debug logging), deduplicating identical consecutive errors and logging recovery once; - suspends export for PermanentExportErrorBackoff (10m) after PermissionDenied/Unauthenticated, with an IAM hint in the log line, then retries so a runtime IAM fix is picked up without restart. Verified against the real Monitoring API with a project the identity cannot write to: one debug line with the hint, nothing on stderr. * feat: warn once per process on the first permanent metrics export error A user who opted into built-in metrics but lacks roles/monitoring.metricWriter should find out without having to attach a debug logger, but a library must not flood the application's log. On the first PermissionDenied/Unauthenticated export error, write exactly one line (sync.Once, process-wide, across all recorders) to the standard logger with the IAM hint; everything after that stays on the debug logger as before. Transient errors never touch the standard logger. Verified against the real Monitoring API: three dials against a project the identity cannot write to produce one line on stderr. * feat: name the project, principal, and disable option in metrics export errors Review feedback on #1185: make the permission error actionable. - Project: taken from the recorder's ResourceContainer. - Principal: best effort, resolved once in NewDialer when metrics are enabled. client_email from service-account JSON credentials (WithCredentialsJSON/File or GOOGLE_APPLICATION_CREDENTIALS), or the default service account from the metadata server on GCE/GKE/Cloud Run. Caller-supplied token sources are opaque; the message then says "the identity used by the connector". - Disable hint: chosen from the connector type. Go API users are told to pass WithBuiltInMetrics(false); Auth Proxy users are told to remove --enable-builtin-metrics (the flag name the proxy must adopt). Both the once-per-process warning and the debug line carry the hint. * feat: enable built-in metrics by default Team decision (Oct 20): ship built-in client metrics on by default rather than opt-in, so that all Auth Proxy and connector users are covered and AIS does not need deployment changes. WithBuiltInMetrics(false) turns them off; option and flag names are unchanged. - builtInMetricsDefault = true seeds dialerConfig.enableBuiltInMetrics. - Proxy disable hint becomes "--enable-builtin-metrics=false". - Tests: TestMain sets the default to false for the test process so the suite does not create real Cloud Monitoring exporters; tests that exercise metrics opt in explicitly, and TestBuiltInMetricsEnabledByDefault pins the production default. Fail-open behavior from the previous commits applies to every user now: connectivity is never affected, a missing metricWriter role produces one line per process on the standard logger with remediation, and exports back off. * feat: exponential backoff (10m doubling to 1h) for permanent metrics export errors With metrics on by default, clients whose identity lacks roles/monitoring.metricWriter are the common case until the IAM auto-grant lands. Replace the fixed 10-minute suspension with exponential backoff: 10m after the first PermissionDenied/ Unauthenticated, doubling on each consecutive permanent error, capped at 1h, reset by a successful export. A client without permission therefore settles at one rejected CreateServiceTimeSeries per instance per hour, versus 60/hour for a working client, while an IAM fix is still picked up within at most an hour without a restart. * fix: classify NotFound and FailedPrecondition export errors as permanent Per the Monitoring API error contract, NotFound (project deleted or unrecognized region) and FailedPrecondition (project/billing state) do not resolve on retry, so they now get the same exponential backoff as PermissionDenied and Unauthenticated. InvalidArgument stays transient on purpose: it covers out-of-order/too-frequent points and partial-batch failures, which the next export of these cumulative metrics corrects, and backing off would suspend the healthy series in the batch too. ResourceExhausted stays transient; a once-per-minute retry is the right cadence for quota. Add a table test pinning every code's classification, including wrapped and joined errors. * docs: document built-in client metrics (on by default, IAM, opt-out, labels) in README * feat: back off transient metrics export errors too (2m doubling to 16m, jittered) Transient errors (Unavailable, DeadlineExceeded, ResourceExhausted, Internal, InvalidArgument, ...) were retried on every 60s tick. During a Monitoring API regression that keeps the whole connector fleet writing at 100% of normal rate while the service tries to recover. Add a second, shorter backoff tier for transient errors: 2m after the first failure, doubling to a 16m cap, reset on success. The cap bounds the gap in a customer's dashboard after an outage to roughly 16 minutes. Both tiers now apply +/-20% jitter so clients that failed together do not retry on the same tick, and switching error class (e.g. an outage followed by a permission error) restarts the schedule at the new tier's initial value. Verified against an unreachable endpoint (real DeadlineExceeded/ Unavailable): 6 attempts in 50 ticks with gaps 8/13/20/26/22s for a 6s->24s scaled schedule. * test: e2e verification of built-in metrics and fail-open behavior; panic guard on the record path (#1186) * test: e2e verification of built-in metrics and fail-open behavior; panic guard on the record path Replaces the manual prototype (cmd/metric_prototype) with automated coverage that runs in the existing integration-test job: - TestMetricsE2EExport: dial the e2e instance with metrics enabled and a unique application_name, Close (flush), then poll Cloud Monitoring for this process's client_uid and assert all five metrics arrive with the documented label values. Skips with the exact gcloud commands when the e2e identity lacks roles/monitoring.metricWriter or monitoring.viewer. - TestMetricsE2EFailOpenUnreachableEndpoint: metrics pointed at an unroutable endpoint (real DeadlineExceeded/Unavailable). Dials succeed, dial p50 matches a metrics-off dialer, Close returns within the shutdown timeout, nothing on the standard logger. - TestMetricsE2EFailOpenBadCredentials: an invalid token yields a real Unauthenticated. Dials succeed, exactly one standard-logger warning with remediation, export suspended (1-3 attempts in the window). All three pass against a real instance and the real Monitoring API; they skip under -short like the other e2e tests. Also add a deferred recover() to every MetricRecorder.Record* method so a panic inside the metrics SDK can never reach the dial path, with a test that fails when the guard is removed. Test hooks move from internal/tel/export_test.go to testhooks.go so the external e2e package can use them; export_test.go in the root package exposes the few unexported accessors the e2e tests need. * fix: make the metrics panic guard observable; use atomic counter in e2e test Review feedback on #1186. A bare recover() would make a bug in the metrics pipeline invisible. recoverPanic now counts recovered panics in tel.RecoveredPanics, debug-logs once per recorder, and writes one line per process to the standard logger with a link to file an issue. The dial path is still never affected. The e2e bad-credentials test uses atomic.Int32 instead of a mutex for its call counter. * fix: generate client_uid per Dialer, not per process (#1188) Two Dialers in one process each own their own cumulative counters and export loop; sharing one client_uid made them write conflicting points to the same Cloud Monitoring series. Restore the per-Dialer UUID that main has and that the AlloyDB, Spanner and Bigtable clients use. This also restores the pre-existing OpenCensus dialer_id semantics. * fix: record metrics on the SQL Data path; span before resolve; idempotent Close (#1189) * fix: generate client_uid per Dialer, not per process Two Dialers in one process each own their own cumulative counters and export loop; sharing one client_uid made them write conflicting points to the same Cloud Monitoring series. Restore the per-Dialer UUID that main has and that the AlloyDB, Spanner and Bigtable clients use. This also restores the pre-existing OpenCensus dialer_id semantics. * fix: record metrics on the SQL Data path, start the dial span before resolve, make Close idempotent - SQL Data (SFEv3) connections now record open/close/latency/duration with instance_ip_type=sql_data; cooldown rejections and dial failures are recorded as failed attempts; the direct-IP fallback is timed on its own. - The dial span and RecordDialError run before name resolution again, so DNS/resolve failures reach the existing OpenCensus dial_failure_count. - instrumentedConn.Close reports once even if called twice. * feat: add ParseComputePlatform and ComputePlatforms (#1187) Go "enums" (a named type plus typed constants) are not closed: a string conversion such as ComputePlatform(s) compiles for any s and is only rejected later by NewDialer. Clients that take the platform as a string, notably the Auth Proxy's --compute-platform flag, should not have to copy the list of valid values. - ComputePlatforms() returns all valid values in a stable order (a copy), for help text. - ParseComputePlatform(s) trims whitespace, matches case-insensitively, and returns an error listing the valid values otherwise. - WithComputePlatform stays strict (exact constants only) and its error now lists the valid values too. All three read from one unexported slice. - TestComputePlatformsMatchesInternalSet fails if the public list and internal/tel's allowed label values drift apart. - README mentions the helpers. * perf: keep metadata lookups and recorder flushes off the dial path; close-safe recorders; parallel Close (#1190) - Detect compute platform, client region and principal once in NewDialer, and only when the metric client exists. metricRecorder no longer calls the metadata server under metricsMu. - Shut down a replaced (engine-unknown) recorder in a goroutine tracked by metricsWG; Close waits for it before closing the shared MetricClient. - metricRecorder returns NullMetricRecorder after Close so a racing Dial cannot leak an export goroutine on a closed client. - Close shuts recorders down concurrently under the shared timeout. * fix: explicit histogram buckets; scope-aware hint; metrics default off outside googleapis.com (#1191) * fix: explicit histogram buckets and units; scope-aware remediation hint; metrics off by default outside googleapis.com - connect_latencies (ms) and connection_durations (s) use explicit bucket boundaries instead of the OTel defaults, which lumped all durations under 5 s together and overflowed at ~2.8 h. connect_latencies declares its unit. - The once-per-process warning distinguishes an access token lacking the monitoring.write scope from a missing IAM role and says which to fix. - Built-in metrics default to off when the universe domain is not googleapis.com; WithBuiltInMetrics(true) still enables them. - Drop never-emitted Attributes fields; revert example copyright bumps. * test: opting out of built-in metrics is silent end to end With WithBuiltInMetrics(false), a dial, close and Dialer.Close must produce no export, no standard-logger output and no metrics debug line. The new test found that every recorder lookup still logged "disabling built-in metrics" and stored a throwaway NullMetricRecorder; the lookup now short-circuits when metrics are off. * test: pin that Record* with a canceled or expired context still counts The OpenTelemetry SDK's synchronous instruments only use the context for exemplar trace linkage and never consult ctx.Err(), so failures recorded with the (often already canceled) Dial context are not dropped. Pin that property so an SDK change would be caught. * test: TestDisabledMetricsAreSilent must not depend on ADC setupDialer replaces its default options when dialerOptions is set, so the test dropped WithTokenSource and NewDialer fell back to Application Default Credentials, which CI runners don't have. * fix: exponentially spaced histogram buckets Per review: connect_latencies uses 20 buckets from 1 ms to 5 min and connection_durations 50 buckets from 0.1 s to 24 h, each boundary a constant multiple of the previous one, so percentile estimates have the same relative error across the range. TestHistogramBucketShape pins the layout. * fix: treat a final flush rejected for exceeding the metric's write rate as benign (#1193) The Monitoring API rejects a point written within 30s (half the 60s sample period) of the previous point on the same series. Periodic exports never hit this; the final flush on Shutdown does whenever the process exits within 30s of an export. The rejection came back as FailedPrecondition, which was classified as permanent, so a routine shutdown printed the standard-logger warning with an IAM hint and suspended export. Recognise the message, debug-log it once and move on. * refactor: group metricRecorder methods after the struct; separate the export pipeline (#1194) * refactor: group metricRecorder methods after the struct; separate the export pipeline Pure reordering of top-level declarations in internal/tel/tel.go, no code changes. metricRecorder's Record* methods had drifted ~210 lines away from the struct, behind the exporter/backoff code, and the exporter's helpers were interleaved too. Now: definitions, MetricRecorder (+ all methods), export pipeline, NullMetricRecorder, each under a banner comment. * fix: an export canceled by shutdown is not a failure The periodic reader cancels an in-flight export when it is shut down. That Canceled error differs from whatever the server was returning, so the deduplicated debug logging emitted a second line and the backoff schedule advanced, making TestTransientExportFailuresBackOffAndAreDeduplicated flaky (5/80 runs locally, 2/4 CI jobs). Treat context.Canceled / gRPC Canceled as an interrupted export: no log, no backoff change. Deadline errors are deliberately still transient failures. * fix: pre-merge review findings (OTel env labels, exporter notice, Close locking, recorder key) (#1195) * fix: pre-merge review findings: OTel env labels, exporter notice, Close locking, recorder key - Never promote resource attributes to metric labels (WithFilteredResourceAttributes(NoAttributes)). The SDK merges OTEL_SERVICE_NAME / OTEL_RESOURCE_ATTRIBUTES into the resource and the exporter copied service.* onto every series; the API rejected the write and metrics were silently dead in any OTel-instrumented environment. - Pin the exporter to v0.60.0: v0.61.0+ prints a deprecation notice on the standard logger on first use, for every user, once metrics default on. - Close releases d.lock and metricsMu before the (network) flush. - Key recorders by project:region:name, not the full ConnName, so a DNS name and a connection name for one instance share one series. - Bound metadata-server lookups to 2s; make concurrent Close safe; use a schemaless OTel resource. * fix: universe-aware metric credentials; actionable hints; run metrics e2e in CI; proxy version label - Metric credentials carry the Dialer's universe domain (DetectDefault UniverseDomain + option.WithAuthCredentials); caller token providers are wrapped in auth.Credentials with the configured universe. Without this every export failed the client's universe check outside googleapis.com, with an IAM hint. - The once-per-process warning now names the real fix for a disabled Monitoring API (SERVICE_DISABLED) and for a universe mismatch. - CI runs the e2e tests in package mode so e2e_metrics_test.go executes (it never ran: the scripts listed the three DB test files explicitly). - connector_version is the Auth Proxy's version when embedded in it. - Close the failed SQL Data stream before falling back; README closes the pool before the Dialer; bad-credentials e2e injects via AuthCredentials. * test: bill the metrics e2e export to QUOTA_PROJECT; assert open_connections drains; small review items - The CI service account lives in an auto-provisioned project without the Monitoring API enabled, so its exports failed SERVICE_DISABLED. The e2e test now passes WithQuotaProject(QUOTA_PROJECT) (and the same quota project for the read-back), billing the calls to the e2e project; the skip message names the extra role needed. - TestMetricsE2EExport asserts open_connections is 0 after Close (mutation-checked: dropping the decrement fails the test). - Exemplars off on the meter provider; unit "1" on the three counters; connectionInfoCache's unused bool return removed (restores main's signature); README notes on billing project and hermetic tests. * fix: keep a TPC credential file's universe; name the billed project in the hint; e2e runs with the production default and fails in CI - metricCredentials passed the Dialer's effective universe (googleapis.com when WithUniverseDomain is unset) to DetectDefault, which overrides the universe_domain in the credential file, so a TPC credential lost its universe and its tokens were aimed at monitoring.googleapis.com. Only an explicit WithUniverseDomain is passed now; the file's value wins otherwise. Tests cover TPC JSON and TPC ADC without WithUniverseDomain. - The SERVICE_DISABLED hint names the project the API reports (the billed consumer), not the instance project. - TestMain turns the built-in default off only when no real instance is configured, so the database e2e tests run with metrics on as users do; TestMetricsE2EExport fails rather than skips when CI=true. * test: bill the metrics read-back to the instance project timeSeries.list, unlike the write, requires billing on the consumer project, and the e2e quota project has none; CI's export succeeded but the read-back was rejected and the test skipped. The read-back is billed to the instance's project now, and a read-back permission error also fails (not skips) in CI. * fix: leave application_name empty when not set; resolve the applicationID TODO (#1197) Replace the TODO on dialerConfig.applicationName with the decision: the application_name label is only ever the explicit WithApplicationName value and is left empty otherwise (the "unknown" placeholder is removed). It is not inferred from the environment: the GCE VM name / GKE pod name are per-instance identifiers (client_uid already provides that) and a service-level default exists only on some platforms. Pinned by TestApplicationNameDefault; godoc and README updated. * fix: an export that runs out of its own deadline is never a permanent error (#1199) When an export's context expires before the Monitoring API answers (for example while a cold credential mints its first token), gRPC can report it as Unauthenticated ("per-RPC creds failed ... context deadline exceeded"), which was classified as permanent: export was suspended for the permanent-error backoff and the once-per-process warning told the user to grant roles/monitoring.metricWriter although IAM was fine. Seen on the macOS CI runner, where Dialer.Close's 3s final flush was the first export and GitHub WIF token minting took longer. Export now backs such errors off as transient, for periodic exports and the final flush alike. Also: - debug-log the first successful export once per instance; - keep the once-per-process warning on a single line (gRPC status details embed newlines); - TestMetricsE2EExport exports on the periodic path and polls Cloud Monitoring before Close instead of relying on the short final flush.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Findings from a final independent review of the complete feature →
maindiff, done before merging #1123. Two of them are real production issues that none of the existing tests covered; the rest are hardening. All verified against the real Monitoring API where applicable.1. OTel environment variables silently broke the export (blocker)
The OpenTelemetry SDK merges
OTEL_SERVICE_NAME/OTEL_RESOURCE_ATTRIBUTESinto every resource passed toWithResource, and the GCP exporter's defaultDefaultResourceAttributesFiltercopiesservice.name,service.namespace,service.instance.idonto every time series as metric labels. Our descriptors are Google-owned with a fixed label schema, soCreateServiceTimeSeriesrejects the write:Reproduced against the real API (
OTEL_SERVICE_NAME=my-app go test -run TestMetricsE2EExport→ fails).InvalidArgumentis classified transient, so the connector retried at 2–16 min forever, wrote nothing to the standard logger, and metrics were simply dead for that process. These variables are injected by the OTel Operator on GKE and set by any application that already uses OTel — a large part of the target population.Fix:
cmexporter.WithFilteredResourceAttributes(cmexporter.NoAttributes): resource attributes feed the monitored resource only, never metric labels.TestMetricLabelsIgnoreOTelEnvironmentsets both variables and asserts the exported metric labels are exactly the documented set while the resource is still fully populated. Re-ran the e2e with the variables set: passes.2. Exporter deprecation notice on every user's standard logger (blocker for "quiet by default")
opentelemetry-operations-go/exporter/metricv0.61.0+ prints "Google Cloud OpenTelemetry Monitoring exporter for Go is deprecated …" vialog.Printlnon first use — i.e. on the firstDialof every application and every Auth Proxy once metrics are on by default, even when everything works. That breaks the "at most one line, and only on a permanent error" contract.Fix: pin the exporter to v0.60.0 (last version without the notice; same API, builds against our OTel v1.47) with a
go.modcomment explaining why.TestEnabledMetricsWriteNothingToStandardLoggercaptureslog.Default()through create → record → export → close on the enabled path and asserts it stays empty. Replacing the exporter (OTLP or ~150 lines of in-house conversion) remains the proper follow-up before Jan 2027.3.
Close()heldd.lockandmetricsMuacross the metrics flushThe flush does network I/O (bounded by 3 s).
d.lockis needed by connections' error paths (removeCached) and by in-flightDial/Warmup;metricsMuby anyDialracingClose. A slow flush could therefore delay a connection'sRead/Writeerror return by up to 3 s.Closenow releasesd.lockafter closing caches, snapshots the recorder map undermetricsMuand flushes with no lock held (d.closedis already closed, sometricRecordercannot add to the map afterwards).TestCloseDoesNotHoldDialerLockDuringFlushpins it.4. Recorder keyed by full
ConnNameincluding the DNS nameDialing one instance by DNS name and by connection name created two recorders → two cumulative series with identical labels from one
client_uid→ rejected as conflicting. Recorders are now keyed byproject:region:nameonly (metricRecorderKey).TestMetricRecorderKeyIgnoresDomainName.5. Hardening
DetectClientRegion,DetectComputePlatformanddetectPrincipaluse the*WithContextmetadata APIs with a 2 s budget (the metadata client's own retries can otherwise add up to many seconds on a cold GKE node).Close: theselect/close(d.closed)pair was racy (two concurrent callers could both close the channel → panic). Guarded by anatomic.BoolCAS;TestConcurrentCloseIsSaferuns 8 concurrent closers × 20.resource.NewWithAttributes(monitoredResource, …)passed the resource type as the schema URL; nowresource.NewSchemaless(would have become anErrSchemaURLConflict→otel.Handle→ stderr with a future SDK/env that sets a schema).Known, accepted (not changed here)
WithTokenSource,sqlservice.adminonly) get the one-line scope warning and no metrics until they addmonitoring.write— this is the documented default-on trade-off (README, feat: add built-in client metrics exported to Cloud Monitoring, on by default (BETA) #1123 description).panicWarnOnceis a second possible standard-logger line, but only if a bug in the record path panics.dial_latencynow measures the IP leg only (startTime reset in fix: record metrics on the SQL Data path; span before resolve; idempotent Close #1189);WithSQLDatausers are new/rare.Verification
-raceon the touched tests; golangci-lint clean; full suite without credentials; real-instance e2e with and without OTel env vars.Second commit: items from Karan's swarm review (doc)
Independently confirmed and fixed here; the first commit already covered his B1 (OTel env labels), B3 (deprecation notice), B4-keying and M4 (Close lock).
metricCredentialsnow passesUniverseDomaintocredentials.DetectDefaultand wraps caller-supplied token providers inauth.Credentialscarrying the Dialer's universe; the client is configured withoption.WithAuthCredentials. Without this,WithUniverseDomain(...)+ explicitWithBuiltInMetrics(true)failed every export in the client's universe check and printed an IAM hint.TestMetricCredentialscovers ADC / JSON / token source × default / TPC universe (the TPC-ADC case needs a parseable key because the auth library builds self-signed JWTs eagerly outside googleapis.com).SERVICE_DISABLED→ "enable monitoring.googleapis.com on project …"; universe mismatch → "credentials must belong to the Dialer's universe".TestRemediationHintMatchesCause. (The IAM/API grants on the GitHub Actions project64201112091and the Kokoro impersonated SA are an owner action.)build.shand.ci/cloudbuild.yamllisted the three DB test files explicitly; they now run package mode with-run '^Test(MySQL|Postgres|SqlServer|MetricsE2E)'(exactly the 22 e2e tests). Verified the filter compiles and executes the metrics tests.connector_versionfor the proxy. Parsed from thecloud-sql-proxy/<version>user-agent token when present (TestConnectorVersionValue), so a proxy fleet can be broken down by proxy release.fallbackConncloses the failed stream before swapping in the direct-IP connection; README'sdatabase/sqlexample closes the pool beforecleanup()so close metrics are recorded. Drain-before-shutdown of a replaced recorder is a follow-up.option.WithTokenSource, whichoption.WithAuthCredentialsnow outranks, so it was running with valid credentials; it injects viaAuthCredentialsnow and passes for the right reason.Not changed (tracked as follow-ups): B4 recorder eviction, B5 drain tracking, B7 (
WithAdminAPIEndpointdefault / Monitoring endpoint option), B8 google3 pre-import CL, M1/M7 and the R1–R8 style items.Third commit: CI billing project and remaining small items
gh-13a715-cloud-sql-go--5cc0d1(project64201112091), an auto-provisioned project without the Monitoring API, so every CI export failedSERVICE_DISABLED(visible in this PR's own CI log once E2E-1 made the test run). Rather than depending on that project, the e2e test bills the export and the read-back toQUOTA_PROJECT(cloud-sql-connector-testing, already a CI secret) viaWithQuotaProject, which the connector forwards to the metric client. Remaining owner action oncloud-sql-connector-testingforcloud-sql-go-connector@gh-13a715-cloud-sql-go--5cc0d1.iam.gserviceaccount.com:roles/serviceusage.serviceUsageConsumer,roles/monitoring.metricWriter,roles/monitoring.viewer. Until then the test skips and prints exactly those commands.TestMetricsE2EExportassertsopen_connections == 0afterClose(mutation-checked).exemplar.AlwaysOffFilter). M7:WithUnit("1")on the three counters. R4:connectionInfoCacheback tomain's signature (the addedboolwas unused). B7: README tells hermetic tests to passWithBuiltInMetrics(false); billing project documented.Fourth commit: second-round review (Karan, tab "#1195 Verification")
credentials.DetectDefault'sUniverseDomainoverrides theuniverse_domainin the credential file (resolveUniverseDomain(opts, file)), so passing the effective defaultgoogleapis.comstripped a TPC credential of its universe whenWithUniverseDomainwas not set. Only an explicitWithUniverseDomainis passed now (cfg.clientUniverseDomain, empty when unset), matching the Admin API credentials; the configured/default universe is still applied to caller token sources.TestMetricCredentialsgains TPC-JSON and TPC-ADC cases withoutWithUniverseDomain(universe staysmy-tpc.goog).SERVICE_DISABLEDhint names the billed project fromErrorInfo.metadata(containerInfo/consumer), not the instance project.TestMetricsE2EExportusest.Fatalfinstead oft.SkipfwhenCI=true. The quota project now has the Monitoring API (cl/1008463619) and the CI identities have the roles (cl/1008444485).TestMainkeeps the built-in default off only when no real instance is configured (unit runs stay offline); e2e runs use the production default, so the 19 database e2e tests exercise metrics on. Chosen over atesting.Short()guard so a plaingo test ./...on a dev machine never reaches Cloud Monitoring.Closecompletion-barrier semantics for a second concurrent caller (pre-existing behaviour).Fifth commit: read-back billing
CI's export now succeeded (first run past the warning check), but the read-back
timeSeries.listwas rejected: unlike the write, it requires billing on the consumer project and the e2e quota project has none. The read-back is billed to the instance project (cloud-sql-connector-testing), and a read-back permission error also fails in CI. CI now shows--- PASS: TestMetricsE2EExporton ubuntu, macOS and Windows — the first end-to-end verification of the export from the CI identity.