feat(datastore): time every query with a named-query metric
What
Every statement in internal/datastore now runs under a symbolic query name, so
gitlab_artifact_registry_database_query_duration_seconds{name} and
gitlab_artifact_registry_database_queries_total{name} cover the datastore
rather than only the storage-owned queries in internal/storage/queries.go.
Before this, every statement execution in the package emitted nothing: the
name label carried exactly the S06 upload/blob values, against a documented
budget of 200.
internal/datastore/queries.go—instrumentQuery/instrumentExec, which take the jet statement and close the timer after jet's scan.internal/datastore/query_names.go— the name catalog, grouped by target table, one name per statement.- Every call site rewritten; the
TODO(#54)/TODO(#92-followup-S03)markers the work satisfies are deleted. internal/metrics/cardinality.go— thenamebudget moves from 200 to 350.internal/storage— the two queries that were still bare (pgSession.reReadDirtyandPgBlobStore.blobLocation) now run underupload_sessions_select_dirtyandblob_storage_blobs_select_location, so the S06 set is complete too and its name-coverage test carries both.- Rebased twice during review, so the catalog also covers the statements
mainadded while this was open:counter_drain.go,npm_bulk_delete_scope.go, the container-image soft delete, the npm package row lock, and the npm/maven remote health seams. Files outside the original sweep appear in the diff for that reason, and the source-reading test is what makes the next arrival fail loudly instead of shipping bare. - Docs: the convention in
database-query-patterns.md, and the metric rows inobservability.mdandstorage.md, which named only the storage repositories.
Deviations from #54
The issue prescribes three things this MR does differently. Stating them rather than absorbing them, so the reviewer decides:
- Call form. The issue asks for
defer metrics.InstrumentQuery(name)()at every site; 236 sites use the two helpers instead. Adefercloses at function return, so it measures statement build, scan, and error wrapping rather than the statement. 162 of the 224 statement-running functions return early on argument guards, where a top-of-functiondefercounts a call that never reached Postgres. And 20 functions issue two statements, where onedefermerges both under one name. The eight raw-SQL sites that iterate*sql.Rowsthemselves keep the deferred form — only that spans the caller's scan — placed below their guards. - Names. Twelve of the marker-prescribed names used a method-derived verb
(
container_manifests_get_by_digest,maven_files_list_by_version). Every name here uses the statement's SQL verb (container_manifests_select_by_digest,maven_files_select_by_version), which is the grammar in the S03-B spec's own examples and in the live S06 set (blob_storage_blobs_select_by_digest). One grammar across the whole catalog is what makes a dashboard query predictable. Nothing consumes these values yet, so a sweep to the marker spelling is a search-and-replace if you prefer it. - Budget. The issue expects the names to fit the existing 200-name budget.
They do not, by some margin once both catalogs are counted. The budget moves to
350, and
TestNameBudget_CoversEveryDeclaredQueryNameininternal/metricsreads both catalogs and fails when they outgrow it — the audit itself only counts values it has observed, so without that test a catalog past the budget passes CI and trips in production.
Tests
internal/datastore/queries_test.go:
- The two helpers: the name reaches both vectors, the statement's own
sql.Resultis threaded back to the caller, and a failed statement is still counted — a name going quiet has to mean "not called", never "called and failing". - Invariants read off the package source: catalog names are unique and lower
snake case, each is used at exactly one call site, and no call to a
database/sqlexecution verb outsidequeries.goruns untimed unless its function is pinned as a deferred-timer site. The verb list is the whole surface, not the verbs the package happens to use — a narrower predicate is what let aQueryRowContextquery ship untimed on the first push. internal/storagegets its own sweep (TestEveryStatementIsTimed) over the closure form. It had none, which is how two of its queries stayed bare; it asserts nothing about per-name call-site counts, because the S06 catalog deliberately lets one name cover several sites or a whole transaction.
The source-reading invariant is the load-bearing one. A missing timer changes no
behavior and fails no other test: the query runs, the caller gets its rows, and
the series is simply absent from /-/metrics.
Verified locally: go test -short ./..., the full internal/datastore and
internal/storage integration suites with -tags=integration, golangci-lint run on the touched
packages, and golangci-lint run --build-tags=integration ./internal/datastore/
(no new findings in the files this MR changes).
No e2e scenario is added or affected: the change adds metric series and alters
no request path, response, or SQL. The catalogs in docs/testing/ describe
client-visible flows, and none of them observes /-/metrics.
Reviewable size
~1,240 added lines against the 500-LOC guidance, and the split is:
| Group | Shape |
|---|---|
query_names.go |
one-line constants, grouped by table |
queries_test.go |
three helper tests plus the source invariants |
| the call sites, across ~55 files | one line each: stmt.QueryContext(ctx, db, &row) → instrumentQuery(ctx, name, stmt, db, &row) |
queries.go |
45 lines, the only genuinely new logic |
internal/storage |
two names, two wraps, and the package's own sweep |
internal/metrics |
the budget test over both catalogs |
| docs | the convention section and the metric rows |
Splitting per store would produce eight MRs that each half-move the same
invariant, and none of them could add the completeness test until the last one
landed — the test fails while any statement remains bare. The mechanical bulk is
the one-line call-site edits; reviewing queries.go, query_names.go's grammar,
and the pinned deferred-timer sites covers the judgement in the change.
Related to #54