Skip to content

chore: wire telemetry into Connector, off by default - #572

Draft
enocom wants to merge 1 commit into
telemetry-6-refresh-metricsfrom
telemetry-7-wire-connector
Draft

enocom wants to merge 1 commit into
telemetry-6-refresh-metricsfrom
telemetry-7-wire-connector

Conversation

@enocom

@enocom enocom commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

Part 7 of 9 in the built-in telemetry stack, splitting what was previously one ~3,200 line commit (#539) into reviewable pieces.

Based on #571 — review that one first; only the last commit here is new.

Unit tests, ruff check, ruff format --check and mypy are green at every commit in the stack, not just at the tip.


Hand the synchronous Connector a metric recorder per instance and report
on every dial: a dial_count with a status, a dial_latency for successful
dials, and the byte and connection counts collected by
InstrumentedSocket. The refresh caches get the same recorder so their
refresh_count lands under the same instance.

enable_builtin_telemetry defaults to False for now. Everything here is
inert until that default flips, which is a change of its own so that the
moment telemetry starts running for everyone is one line to revert.

Dial failures are tagged with the phase that raised them

Every dial records exactly one dial_count with a status: success,
user_error, cache_error, tcp_error, tls_error, or mdx_error. The status
is set on every exit path and the count recorded in a finally, rather
than per except-branch, which would silently drop the count for any
failure that does not match a branch.

metadata_exchange names the phase that failed by tagging the exception on
its way out, and connect_async reads the tag back. An exception carrying
no tag is a problem with the cached connection info itself, e.g.
create_ssl_context on a malformed certificate, and records as
cache_error.

Tagging keeps the classification out of the exceptions callers see: a
refused connection is still a ConnectionRefusedError with its errno,
traceback and stdlib subclass intact, so nothing here is a breaking
change for callers. The Go connector likewise returns a plain error from
its metadata exchange rather than a named type. The tag travels on the
exception object itself, which survives the hop back from the executor
thread that ran the dial.

InstrumentedSocket is installed only when telemetry is enabled

The wrapper stands between every driver and its socket for the life of
the connection, reimplementing makefile() and close(). A caller who
opted out should get the socket the driver would have had without this
feature, so the wrap is behind the flag. The only thing lost is
open_connections for a caller whose metrics are not being exported
anyway.

Shutdown

close() bounds only the telemetry flush, not the whole shutdown sequence.
The provider gets TELEMETRY_SHUTDOWN_TIMEOUT_S, passed down to the write
RPC as an explicit deadline so a stalled write fails fast; the previous
3s budget covered the whole sequence, which would let a slow cache close
abandon the connector half-shut-down. Matches Dialer.Close in the Go
connector, which bounds only the metric shutdown.

The README's debug logging example is corrected while it is being
referenced: google-api-core stops the google logger from propagating to
the root logger as soon as any Google client is built, so
logging.basicConfig() on its own never showed these records.


The stack (merges bottom to top):

  1. refactor: extract _metadata_exchange helper #566 — refactor: extract _metadata_exchange helper
  2. fix: do not mask dial errors with KeyError #567 — fix: do not mask dial errors with KeyError
  3. fix: close the socket when the metadata exchange fails #568 — fix: close the socket when the metadata exchange fails
  4. chore: add telemetry provider, recorders, and exporter #569 — chore: add telemetry provider, recorders, and exporter
  5. chore: add InstrumentedSocket #570 — chore: add InstrumentedSocket
  6. chore: record refresh metrics #571 — chore: record refresh metrics
  7. chore: wire telemetry into Connector, off by default #572 — chore: wire telemetry into Connector, off by default ← you are here
  8. chore: wire telemetry into AsyncConnector, off by default #573 — chore: wire telemetry into AsyncConnector, off by default
  9. feat: enable built-in telemetry by default #574 — feat: enable built-in telemetry by default

Group A (1–3) is independent of telemetry and could ship on its own, and carries no breaking change: dial failures are classified for the metric by tagging the exception in #572, not by changing the type callers see. Group B (4–8) is inert because enable_builtin_telemetry defaults to False throughout. #574 flips that default, so reverting it alone turns the whole feature off.

Hand the synchronous Connector a metric recorder per instance and report
on every dial: a dial_count with a status, a dial_latency for successful
dials, and the byte and connection counts collected by
InstrumentedSocket. The refresh caches get the same recorder so their
refresh_count lands under the same instance.

enable_builtin_telemetry defaults to False for now. Everything here is
inert until that default flips, which is a change of its own so that the
moment telemetry starts running for everyone is one line to revert.

Dial failures are tagged with the phase that raised them
--------------------------------------------------------
Every dial records exactly one dial_count with a status: success,
user_error, cache_error, tcp_error, tls_error, or mdx_error. The status
is set on every exit path and the count recorded in a finally, rather
than per except-branch, which would silently drop the count for any
failure that does not match a branch.

metadata_exchange names the phase that failed by tagging the exception on
its way out, and connect_async reads the tag back. An exception carrying
no tag is a problem with the cached connection info itself, e.g.
create_ssl_context on a malformed certificate, and records as
cache_error.

Tagging keeps the classification out of the exceptions callers see: a
refused connection is still a ConnectionRefusedError with its errno,
traceback and stdlib subclass intact, so nothing here is a breaking
change for callers. The Go connector likewise returns a plain error from
its metadata exchange rather than a named type. The tag travels on the
exception object itself, which survives the hop back from the executor
thread that ran the dial.

InstrumentedSocket is installed only when telemetry is enabled
--------------------------------------------------------------
The wrapper stands between every driver and its socket for the life of
the connection, reimplementing makefile() and close(). A caller who
opted out should get the socket the driver would have had without this
feature, so the wrap is behind the flag. The only thing lost is
open_connections for a caller whose metrics are not being exported
anyway.

Shutdown
--------
close() bounds only the telemetry flush, not the whole shutdown sequence.
The provider gets TELEMETRY_SHUTDOWN_TIMEOUT_S, passed down to the write
RPC as an explicit deadline so a stalled write fails fast; the previous
3s budget covered the whole sequence, which would let a slow cache close
abandon the connector half-shut-down. Matches Dialer.Close in the Go
connector, which bounds only the metric shutdown.

The README's debug logging example is corrected while it is being
referenced: google-api-core stops the google logger from propagating to
the root logger as soon as any Google client is built, so
logging.basicConfig() on its own never showed these records.
@enocom
enocom force-pushed the telemetry-7-wire-connector branch from 7d91ea9 to 64a522a Compare October 21, 2026 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant