feat(metrics): count database query failures by query name
What
Two per-name outcome counters alongside the existing
database_queries_total{name} and database_query_duration_seconds{name}:
| Metric | Counts |
|---|---|
gitlab_artifact_registry_database_query_errors_total{name} |
Query failures |
gitlab_artifact_registry_database_query_cancellations_total{name,cancellation_reason} |
Queries whose caller context ended first, cancellation_reason ∈ {client, deadline} |
metrics.InstrumentQueryErr(name) func(error) wraps InstrumentQuery and books
one of them from the query's error, and materializes both at 0 on a name's
first query so a failure-rate panel needs no or vector(0). name is already in
the cardinality table; cancellation_reason is a new closed set of two, pinned
in closedSetValues and budgeted in expectedDistinctValues.
Why
InstrumentQuery books duration and count unconditionally and never sees an
error, so a failing query is only visible as a 5xx downstream. An operator
cannot see which named query is failing, or at what rate.
Design
Option (b): the four instrumentQuery / instrumentExec helpers in
internal/datastore/queries.go and internal/storage/queries.go are the one
choke point, so naming their error returns and passing that to the new closure
covers 423 of the 452 instrumented call sites in four functions, leaving the
29 deferred-timer sites (measured at 36ca59287: 409 datastore + 14 storage
helper sites, 28 + 1 raw). Rejected (a),
migrating call sites to an error-taking closure: 30 further sites, most needing
a named return, and touching the prose that names metrics.InstrumentQuery
drags three large pre-existing rationale blocks (instrumentQuery's 16-line
doc, rawSQLTimedFunctions, TestEveryStatementIsTimed) over the comment-cap
hook, which would force compressing them to 1-2 lines. Rejected (c), a pgx
tracer: the pool is built inside LabKit, no pgx.QueryTracer seam is reachable,
and the symbolic name is not in the driver's context anyway.
Why the classifier lives in internal/metrics
It costs this package an import of github.com/go-jet/jet/v2/qrm for one
sentinel. The alternative is a copy of the same five-line switch in
internal/datastore and internal/storage, since both need it and neither
imports the other — and two copies of a classification rule is how the two
packages start disagreeing about what counts as a failure. database.go in
this package is already the database-specific file (it models pgxpool.Stat
in PoolStats), and putting the rule beside the counters keeps the metric's
meaning in one place, which is where a reader looks for it.
What each counter excludes, and why
Stated in both Help strings and in the docs rows.
- No row is not a failure. jet returns
qrm.ErrNoRowsfor an empty result anddatabase/sqlreturns its own distinctsql.ErrNoRows; both reach the classifier and neither books. Counting them would leave most read paths in permanent failure. - A canceled context gets its own counter, split by who gave up. Folding a
disconnect into the failures makes the error rate track client behavior;
dropping it hides a request that never got its answer.
cancellation_reason=clientis the disconnect,deadlineis a budget this service imposed because Postgres sets nostatement_timeouthere, sodeadlineis the health signal and the one to alert on. A Postgres-side57014reply to the cancel books as a cancellation too, since which of the two shapes returns first is a race. - A constraint violation is not excluded. A duplicate-name insert books a failure before the caller turns it into a 409, so create names carry a floor that tracks client retries. The handled and unhandled cases share a SQLSTATE, and dropping both would hide a genuine constraint bug, so the row and the Help say so instead.
- A panic books no outcome. The named return is still nil when the deferred call runs, so the invocation counts and neither counter moves.
Coverage gap
The raw-SQL sites pinned in rawSQLTimedFunctions
(internal/datastore/queries_test.go), plus upload_sessions_commit_tx, keep
defer metrics.InstrumentQuery(name)(), which never sees an error. They publish
no series on either counter — and since a helper-routed name now publishes both
at 0 from its first query, that absence is the signal, not an ambiguity.
Recorded at its owning section in docs/dev/database-query-patterns.md, which
the metric's row points at. Closing it is a per-site named-return change and
can land separately, tracked in
#1208.
One knock-on: instrumentQuery's doc comment in
internal/datastore/queries.go still enumerates only the duration and
invocation series. Editing that sentence puts its 16-line block under the 1-line
unexported cap, so the enumeration is left as-is rather than trading the block's
rationale for it.
Tests
internal/metrics/database_test.go: a plain failure books the error counter and not the cancellation one;nilbooks neither; both no-row sentinels book neither (ast.Runsubtests, each wrapped with%wso the classification has to traverse);context.Canceledandcontext.DeadlineExceededbook the cancellation counter and not the error one. Every case asserts both counters, so a misrouted error cannot pass.internal/datastore/queries_test.go: through the real helpers — a failing statement books one failure under its name, a succeeding one books none, and a wrappedqrm.ErrNoRowsbooks none.internal/storage/queries_test.go: the same contract over this package's closure-shaped helpers, which reach the counters by a different route, sofn's error is what the deferred call has to see. All five outcomes as subtests, plus thesql.Resultvariant.
Injection checks, each restored after:
- routing cancellations to
databaseQueryErrorsTotalfails bothTestInstrumentQueryErr_CanceledContextBooksItsOwnCountersubtests; - routing both no-row sentinels to the cancellation arm fails both
TestInstrumentQueryErr_AbsentRowIsNotAFailuresubtests, so neither sentinel is carried by the other; - reverting
internal/storage'sinstrumentQueryto the plain timer failsTestInstrumentQuery_BooksTheOutcomeOfFnon its failure, cancelled and deadline subtests.
Conflict note
internal/metrics/database.go is also touched by !2406 (merged) (pool churn counters).
The hunks are disjoint — this MR is the query-vector and classifier region at
the top of the file, !2406 (merged) the pool collector at the bottom.
git merge-tree --write-tree over the two branch heads returns a tree with no
conflict, so whichever lands second rebases clean; measured at 83d45cba94 and
400d4eb133.
e2e
Observability-only. No request path, handler, route, or query changes: the
helpers' returned values are unchanged, only their error returns are named. No
docs/testing/ scenario is added or affected.
Closes #1205 (closed)