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.ErrNoRows for an empty result and database/sql returns its own distinct sql.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=client is the disconnect, deadline is a budget this service imposed because Postgres sets no statement_timeout here, so deadline is the health signal and the one to alert on. A Postgres-side 57014 reply 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; nil books neither; both no-row sentinels book neither (as t.Run subtests, each wrapped with %w so the classification has to traverse); context.Canceled and context.DeadlineExceeded book 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 wrapped qrm.ErrNoRows books none.
  • internal/storage/queries_test.go: the same contract over this package's closure-shaped helpers, which reach the counters by a different route, so fn's error is what the deferred call has to see. All five outcomes as subtests, plus the sql.Result variant.

Injection checks, each restored after:

  • routing cancellations to databaseQueryErrorsTotal fails both TestInstrumentQueryErr_CanceledContextBooksItsOwnCounter subtests;
  • routing both no-row sentinels to the cancellation arm fails both TestInstrumentQueryErr_AbsentRowIsNotAFailure subtests, so neither sentinel is carried by the other;
  • reverting internal/storage's instrumentQuery to the plain timer fails TestInstrumentQuery_BooksTheOutcomeOfFn on 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)

Edited by João Pereira

Merge request reports

Loading
Loading