database: add per-query Prometheus metrics - #379
Conversation
1ac5bda to
ace10f6
Compare
3bf0781 to
ae28a4b
Compare
patrick-east
left a comment
There was a problem hiding this comment.
Code changes LGTM, just a couple things I'd recommend cleaning up in the comments
| // instrumentedConn wraps a driver.Conn to record query metrics. Unlike an | ||
| // earlier version of this file, it does NOT try to detect which optional | ||
| // driver.Conn interfaces (QueryerContext, ExecerContext, Pinger, ...) the | ||
| // wrapped conn implements and construct a matching wrapper type — that | ||
| // approach requires exhaustively enumerating every combination of ~8 | ||
| // optional interfaces, and it's easy to silently drop one (as a test here | ||
| // caught for driver.Pinger). |
There was a problem hiding this comment.
We probably don't need commentary from previous iterations included in the comments. It seems like the only take away that is important here is that instrumentedConn must implement all of the driver.Conn interfaces.
|
|
||
| func TestInstrumentedConn_ForwardsUnrelatedOptionalInterfaces(t *testing.T) { | ||
| // Pinger is never touched by our wrapper; verify it's still forwarded | ||
| // (the earlier, type-switch-based design silently dropped this). |
There was a problem hiding this comment.
Similar here, we don't need comments about previous issues
ashutosh-narkar
left a comment
There was a problem hiding this comment.
The changes look good. We should add some tests to exercise the metrics package.
- trim commentary about the earlier design iteration from comments in instrumented.go/instrumented_test.go, keeping just the load-bearing takeaway (patrick-east) - add pkg/metrics tests covering database query metrics: nil-safety, per-flag disable behavior, count/duration recording, DatabaseOperation parsing, and namespace-prefixed metric names (ashutosh-narkar)
70c4f80 to
49e2a85
Compare
Adds per-query Prometheus metrics for the Database layer
(CockroachDB/postgres/mysql), which currently has none. Build-time DB
failures today only surface as
BundleBuilderTaskFailureRateHigh{error_type=service} (in a consumer)
with no further detail on which query or operation failed.
New metrics, following the existing gitsync/http/worker pattern in
internal/metrics:
- ocp_database_query_count_total{operation, state} - counter, state
is SUCCESS/FAILED
- ocp_database_query_duration_seconds{operation} - histogram,
operation parsed from the SQL statement's first keyword
(select/insert/update/...)
The instrumentation is wired in at the driver.Connector level
(internal/database/instrumented.go), not by touching any of the ~50
methods in database.go - every one of them already funnels through
sql.OpenDB(connector) for postgres/CockroachDB/mysql (sqlite's
sql.Open path is left alone; it's the in-memory/dev default, not a
production case).
The wrapper unconditionally implements every optional driver.Conn
interface (QueryerContext, ExecerContext, Pinger, ConnPrepareContext,
ConnBeginTx, SessionResetter, NamedValueChecker, Validator), and each
method checks at call time whether the wrapped conn actually supports
it, returning driver.ErrSkip when it doesn't - the sentinel
database/sql defines for exactly this case, so it falls back exactly
as it would without any wrapping. This avoids silently dropping a
capability the wrapped conn supports (e.g. falling back to a slower
prepare-then-exec path without erroring), which a wrapper covering
only some interfaces would risk.
Database.WithMetrics(*metrics.Metrics) mirrors the existing
WithLogger/WithAuthorizer pattern. Service.WithMetrics now also
propagates to s.database, mirroring how it already does for
WithLogger.
Also adds DatabaseMetrics to the public MetricsConfig schema
(internal/config/config.go, regenerated schema.json/config/schema.json)
for parity with GitSyncMetrics/WorkerMetrics/HTTPMetrics, so operators
can fine-tune or disable it the same way.
Signed-off-by: Yi Chen <yi.chen.roger@gmail.com>
49e2a85 to
91877de
Compare
| } | ||
| } | ||
|
|
||
| func TestDatabaseOperation(t *testing.T) { |
There was a problem hiding this comment.
Not sure how this test fits into what we're implementing here. Please remove if not needed.
There was a problem hiding this comment.
DatabaseOperation is new logic added by this PR (parses the SQL keyword for the operation label) — this test covers its parsing edge cases (unrecognized/empty/multi-keyword input) directly, separate from the end-to-end recording test.
ashutosh-narkar
left a comment
There was a problem hiding this comment.
Thanks for adding the tests @yi-chen-roger. The changes lgtm. I'll let @patrick-east give the final sign-off.
Summary
Adds per-query Prometheus metrics for the
Databaselayer (CockroachDB/postgres/mysql), which currently has none. Build-time DB failures today only surface asBundleBuilderTaskFailureRateHigh{error_type=service}(in a consumer) with no further detail on which query or operation failed.New metrics, following the existing
gitsync/http/workerpattern ininternal/metrics:ocp_database_query_count_total{operation, state}— counter,stateisSUCCESS/FAILEDocp_database_query_duration_seconds{operation}— histogram,operationparsed from the SQL statement's first keyword (select/insert/update/...)Design
The instrumentation is wired in at the
driver.Connectorlevel (internal/database/instrumented.go), not by touching any of the ~50 methods indatabase.go— every one of them already funnels throughsql.OpenDB(connector)for postgres/CockroachDB/mysql (sqlite'ssql.Openpath is left alone; it's the in-memory/dev default, not a production case).The tricky part of this kind of wrapping is
driver.Conn's several optional interfaces (QueryerContext,ExecerContext,Pinger,ConnPrepareContext,ConnBeginTx,SessionResetter,NamedValueChecker,Validator) —database/sqldetects which ones aConnimplements to decide its execution path, and a wrapper that doesn't preserve the same set of capabilities silently changes behavior (e.g. falling back to a slower prepare-then-exec path) without erroring.An earlier version of this PR tried to solve this by constructing a different wrapper type per detected capability combination.
TestInstrumentedConn_ForwardsUnrelatedOptionalInterfacescaught a real bug in that approach (driver.Pingersupport silently dropped), which led to redesigning around the pattern used bygithub.com/XSAM/otelsql(not taken as a dependency — it's OTel-native, this stays Prometheus-native to match the rest ofinternal/metrics): the wrapper unconditionally implements every optional interface, and each method checks at call time whether the wrapped conn actually supports it, returningdriver.ErrSkipwhen it doesn't — the sentineldatabase/sqldefines for exactly this case, so it falls back exactly as it would without any wrapping. This avoids the combinatorial-type problem entirely: there's one wrapper type, and no capability can be silently dropped.PrepareContext/BeginTxfallback logic (used when the wrapped conn doesn't implement the optional context-aware version) is adapted fromotelsql, which itself credits it todatabase/sql's own internalctxutil.go.Wiring
Database.WithMetrics(*metrics.Metrics)mirrors the existingWithLogger/WithAuthorizerpattern.Service.WithMetricsnow also propagates tos.database, mirroring how it already does forWithLogger.Also adds
DatabaseMetricsto the publicMetricsConfigschema (internal/config/config.go, regeneratedschema.json/config/schema.json) for parity withGitSyncMetrics/WorkerMetrics/HTTPMetrics, so operators can fine-tune or disable it the same way.Test plan
TestInstrumentedConn_FallsBackWhenUnsupported— wrapping a conn with neitherQueryerContextnorExecerContextreturnsdriver.ErrSkip, not a panic or a silent no-op.TestInstrumentedConn_ForwardsUnrelatedOptionalInterfaces—driver.Pingersupport is preserved through the wrapper (the regression test for the bug the earlier design had).TestInstrumentedConn_RecordsMetrics— a successful query and a failed exec both show up correctly inocp_database_query_count_totalvia a realprometheus.Registry.TestInstrumentedConn_BeginTxFallback— thectxutil.go-derived fallback path propagates errors correctly and rejects unsupported non-default isolation levels.go test ./internal/database/... ./internal/metrics/... ./internal/config/...— all pass except the pre-existingpostgres/mysql/cockroachdbtestcontainers-based subtests, which fail in my sandbox with "rootless Docker not found" — confirmed this also fails identically on unmodifiedmain, so it's an environment limitation, not something this PR broke. Would appreciate a maintainer or CI run to confirm this actually instruments real CockroachDB/postgres/mysql connections end-to-end, since that's the one thing my sandbox couldn't verify directly.