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.goinstrumentQuery / 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 — the name budget moves from 200 to 350.
  • internal/storage — the two queries that were still bare (pgSession.reReadDirty and PgBlobStore.blobLocation) now run under upload_sessions_select_dirty and blob_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 main added 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 in observability.md and storage.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. A defer closes 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-function defer counts a call that never reached Postgres. And 20 functions issue two statements, where one defer merges both under one name. The eight raw-SQL sites that iterate *sql.Rows themselves 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_CoversEveryDeclaredQueryName in internal/metrics reads 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.Result is 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/sql execution verb outside queries.go runs 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 a QueryRowContext query ship untimed on the first push.
  • internal/storage gets 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

Edited by Suleimi Ahmed

Merge request reports

Loading
Loading