Repository navigation
Conversation
panavenue
force-pushed
the
feature/client-side-metrics
branch
from
July 8, 2026 00:26
f70ef7b to
604ab5d
Compare
…tatus (#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
#1129) * feat: add support for connection_durations and open_connection_count metrics * fix from comment
panavenue
force-pushed
the
feature/client-side-metrics
branch
from
October 14, 2026 18:39
3447d9f to
d20b14a
Compare
…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.
…tor_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.
…d 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
…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)
… 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.
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".
panavenue
changed the title
panavenue
marked this pull request as ready for review
October 19, 2026 22:13
panavenue
requested review from
enocom,
hessjcg,
jackwotherspoon,
kgala2 and
nancynh
and removed request for
jackwotherspoon
October 19, 2026 22:13
panavenue
changed the title
…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.
…nic 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.
kgala2
previously approved these changes
Oct 22, 2026
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.
hessjcg
reviewed
Oct 22, 2026
| } | ||
|
|
||
| // DatabaseEngine returns the configured database engine type for this recorder. | ||
| func (m *metricRecorder) DatabaseEngine() string { |
Member
There was a problem hiding this comment.
Reorganize this file: group the metricRecorder methods together after the struct. You have a second batch of variables and structs interleaved between these methods.
…tent 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.
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.
…lose-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.
…f 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.
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.
Summary
Adds built-in client-side metrics to the Go connector, enabled by default. The
Dialerexports connection telemetry to Cloud Monitoring undercloudsql.googleapis.com/client/connector/*using thecloudsql.googleapis.com/InstanceClientmonitored resource.WithBuiltInMetrics(false)turns them off.Important
These metrics are in BETA (
launch_stage: BETA). Label keys are stable; label values are documented below and pinned by tests, but the feature as a whole is subject to the Cloud SQL BETA terms.Note
On by default. Every upgrader starts exporting unless they pass
WithBuiltInMetrics(false). Export is strictly fail-open: connectivity is never affected. A user whose identity lacksroles/monitoring.metricWriteron the instance project gets one line per process on the standard logger with exact remediation (role, project, principal, how to disable), then exports back off; nothing is written to stderr via OpenTelemetry's handler. See "Impact on existing users".Metrics
client/connector/open_connection_countstatusandconnection_part(also records failures)client/connector/connect_latenciesclient/connector/open_connectionsclient/connector/closed_connection_countclient/connector/connection_durationsResource labels (
InstanceClient):resource_container,resource_id(project:instance),region,client_uid,application_name,client_region,compute_platform,connector_type,connector_version,database_engine_type.Metric labels:
instance_auth_type,instance_ip_type,status,connection_part.Label value contract (lowercase
snake_case, enforced byTestLabelValuesAreLowercaseSnakeCase):instance_ip_type ∈ {public, private, psc, auto, sql_data, unknown}(reports the IP type actually used;auto/sql_dataonly when a dial fails before resolution),compute_platform ∈ {gce, gke, cloud_run, cloud_functions, app_engine, ai_studio, unknown},database_engine_type ∈ {mysql, postgresql, sqlserver, unknown},connector_type ∈ {go, auth_proxy},instance_auth_type ∈ {built_in, iam}.API
WithBuiltInMetrics(enabled bool): defaulttrue; passfalseto turn export off.WithComputePlatform(ComputePlatform): overrides runtime detection. Only theComputePlatform*constants are accepted (GCE,GKE,CloudRun,AppEngine,CloudFunctions,AIStudio); anything else makesNewDialerfail. Intended for embedding clients such as the Auth Proxy.WithApplicationName(string): already existed; now also populates theapplication_nameresource label.Behaviour
WithCredentialsFile/WithCredentialsJSONa second credential scoped tomonitoring.writeis derived; for caller-supplied token sources the API token source is used as-is and must carrymonitoring.writeorcloud-platform.roles/monitoring.metricWriterthere.Dialer.Close()flushes and shuts down all exporters (3 s timeout), so short-lived processes (Cloud Run) don't lose the final window. OneMetricClientis shared per Dialer; a wrapper prevents the GCP exporter'sShutdownfrom closing it out from under other recorders.PermissionDenied/Unauthenticated/NotFound/FailedPrecondition) is reported once per process on the standard logger with the IAM hint and suspends export with exponential backoff (10 min doubling to 1 h); transient errors (API unavailable, timeouts, quota) back off 2 min doubling to 16 min; both jittered and reset on success; everything else is debug-logger only and deduplicated. Connectivity is never affected.Impact on existing users (please read)
roles/monitoring.metricWriteron the instance project see one standard-logger line per process (example below) and no other effect. A separate effort is addingmetricWritertoroles/cloudsql.clientso that line disappears for most users over time.cloud.google.com/go/monitoring,go.opentelemetry.io/otel(+metric,sdk,sdk/metric),github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric,cloud.google.com/go/compute/metadata. Largergo.sumand binaries; no runtime effect.Constituent PRs (all reviewed on
feature/client-side-metrics)#1041 (metrics setup) · #1125 (attributes, dial status) · #1129 (
connection_durations,open_connection_count) · #1175 (default off, opt-in) · #1178 (Dialer fields regression) · #1176 (credentials + flush on Close, incl. #1177) · #1179 (copyright) · #1180 (lowercase label values) · #1181 (WithComputePlatform,ai_studio) · #1182 (resolvedinstance_ip_type) · #1183 (whitespace) · #1184 (open_connections regression test) · #1185 (default on, bool option, quiet/backed-off export, README)Testing
go test -short -race ./...andgolangci-lint v2.14.0(0 issues) on the feature branch.Close()(single and multi-instance), recorder replacement, label value contract,ComputePlatformvalidation, resolved IP type.jinphou-playground:us-west1:pg-dev): all five metrics visible in Cloud Monitoring within ~1 minute of process exit with the documented label values.Follow-ups (not in this PR)
--enable-builtin-metrics/--compute-platformflags.connectors/toolsexercising the released connector and asserting the metrics inspeckle-connectors-cep.closed_connection_count.statusis currently alwayssuccess; error tracking on close is a planned follow-up.