Skip to content

feat(telemetry): trace database queries as children of request spans - #822

Merged
illegalprime merged 2 commits into
mainfrom
feat/db-query-tracing
Jul 29, 2026
Merged

feat(telemetry): trace database queries as children of request spans#822
illegalprime merged 2 commits into
mainfrom
feat/db-query-tracing

Conversation

@illegalprime

@illegalprime illegalprime commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Reviewable diff: +154/-92 across 18 files (excludes generated, test, and story files).

Summary

Extends the request tracing shipped in #813 down to the database layer: every sqlc query now emits an OpenTelemetry client span (db.<MethodName>) nested under the incoming request span, so a single trace in Jaeger/Datadog shows which queries a request ran, how long each took, and which one failed. Queries on a context without an active span (background pollers, MQTT ingest) are deliberately not traced, so no new root traces are created.

How it works

The otelhttp middleware already puts the request span on the context, and that context is threaded through handlers → services → stores → sqlc. The new db.NewTracingQuerier decorator plugs into the generated QueryRetrier seam (the same interceptor seam failoverResetRetrier uses), which hands it the context plus the sqlc method name for all ~700 query methods. For each call it starts a CLIENT span named after the method with db.system=postgresql / db.operation.name attributes, runs the query, and records the outcome: errors mark the span failed (except sql.ErrNoRows, an expected lookup outcome), and a panic unwinding through the deferred End marks the span query panicked instead of exporting a success-looking span.

The decorator is installed at every place a query handle is built, always outermost so one span covers all retry attempts:

  • NewFailoverResettingQuerier — the handle every SQL store uses
  • NewPreparedQuerier — the prepared-statement handle (MQTT path)
  • executeTransaction — the tx-scoped handle passed to WithTransaction actions
  • authz.Service — previously built an ad-hoc untraced handle for ListRoles; now holds the canonical failover-reset + retry + tracing stack

To let the traced handle flow through transactions, WithTransaction/WithTransactionNoResult actions now take the sqlc.Querier interface instead of the concrete *sqlc.Queries — that mechanical signature change is the bulk of the diff.

Diagrams

flowchart TD
    A["otelhttp middleware<br/>(request span in ctx)"] --> B["Connect handler / service"]
    B --> C["store method (ctx)"]
    C --> D["NewTracingQuerier<br/>span: db.&lt;Method&gt;"]
    D --> E["retry / failover-reset querier"]
    E --> F["sqlc.Queries"]
    F --> G["RetryDB / *sql.Tx"]
    G --> H[("PostgreSQL")]

    B2["WithTransaction action"] --> D2["NewTracingQuerier"]
    D2 --> F2["sqlc.Queries (tx-bound)"]
    F2 --> G
Loading
sequenceDiagram
    participant I as tracingInterceptor
    participant Q as query fn
    I->>I: ctx has valid span context?
    alt no (background poll, telemetry off)
        I->>Q: run query untraced
    else yes
        I->>I: start span db.Method (CLIENT)
        I->>Q: run query
        alt error (not ErrNoRows)
            I->>I: status Error (SQLSTATE code only)
        else panic
            I->>I: status Error "query panicked"
        end
        I->>I: end span
    end
Loading

Areas of the code involved

Area What changed Why it matters for review
server/internal/infrastructure/db/tracing_querier.go (new) The tracing interceptor + NewTracingQuerier constructor Core logic: skip rule, error/panic status, span naming
server/internal/infrastructure/db/{failover_reset_querier,prepared_querier,with_transaction}.go Wrap handles with tracing; WithTransaction action signature *sqlc.Queriessqlc.Querier Wrap ordering decides whether spans cover retries
server/internal/domain/authz/service.go Constructor-held querier replaces per-call sqlc.New(s.conn) in ListRoles Also restores failover pool-reset + retry behavior that the ad-hoc handle lacked
server/internal/{domain,infrastructure,handlers,testutil}/** (~20 files) Mechanical closure param change to sqlc.Querier Compile-checked; no behavior change
server/internal/infrastructure/db/tracing_querier_test.go (new) Unit tests: nesting, error, ErrNoRows, parentless skip, panic, end-to-end via generated decorator

Key technical decisions & trade-offs

  • Reuse the generated QueryRetrier seam instead of adding otelsql/otelpgx driver-level instrumentation: method-name span granularity, zero new dependencies, and spans cover row scanning — at the cost of not covering raw-SQL paths outside sqlc.
  • Widen WithTransaction actions to sqlc.Querier instead of wrapping the tx DBTX: uniform span fidelity (a DBTX wrapper can't see QueryRow errors or row-iteration time) in exchange for a mechanical, compile-checked signature sweep.
  • Skip queries on span-less contexts: the 1s queue-dequeue poll, reapers, and MQTT ingest would otherwise export ~90k+ single-span root traces/day at the default sample rate. Adding a job-level span to any background loop re-enables its query tracing automatically. Side effect: telemetry-disabled deployments do no span work at all.
  • sql.ErrNoRows is not a span failure — it's the normal not-found outcome for :one lookups and would otherwise pollute error-rate views.
  • Deliberately untraced: the TimescaleDB metrics-ingest writers (timescaledb/telemetry_store.go, metrics/store.go — span-per-row would flood the exporter) and the transaction itself (no begin/commit span yet). Failed spans export only a SQLSTATE code (SQLSTATE 23505) or query failed as the status description, never err.Error() — raw Postgres messages can embed user-supplied values (e.g. unique-key DETAIL), which belong in server logs, not the tracing backend. Trade-off: correlating a specific span to its exact error means jumping to logs by trace/time.

Testing & validation

  • just lint green (buf, eslint, golangci-lint — 0 issues).
  • go build ./... / go vet ./... clean after rebasing onto current main.
  • New unit tests cover span naming/attributes, parent-child nesting under a request span, error status, ErrNoRows, the parentless skip, panic marking, and an end-to-end call through the generated decorator; db package also run with -shuffle=on and -race.
  • Integration suites against the dev database (DB_PASSWORD=fleet, -p 1): infrastructure/db, authz, command, sqlstores (rerun on this base), plus apikey, handlers/{apikey,command,onboarding}, timescaledb, metrics earlier on the same diff — all pass.
  • /code-review max --fix ran over the diff; its fixes (parentless skip, panic status, authz querier) are included.
  • Not covered: no automated assertion that spans land in Jaeger end-to-end (verified logic-level only); metrics-ingest stores intentionally untraced.

🤖 Generated with Claude Code

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@illegalprime
illegalprime requested a review from a team as a code owner July 28, 2026 16:01
@github-actions github-actions Bot added the review-policy: needs-review Managed by the Review Policy workflow. label Jul 28, 2026
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

  • Reviewed pull request diff only (41237c6c108b56f304c7ba15b81000cda97e4670...56e003e0cc7917d722458258d77b7da17819c9c2, exact PR three-dot diff)
  • Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.


Review Summary

Overall Risk: NONE

Findings

No findings.

Notes

Reviewed .git/codex-review.diff for the scoped PR changes at commit 56e003e0cc7917d722458258d77b7da17819c9c2. The diff primarily adds sqlc query tracing wrappers and updates transaction callbacks from *sqlc.Queries to sqlc.Querier; I did not identify a concrete security, correctness, or reliability issue in the changed hunks.


Generated by Codex Security Review |
Triggered by: @illegalprime |
Review workflow run

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added review-policy: human-approved Managed by the Review Policy workflow. and removed review-policy: needs-review Managed by the Review Policy workflow. labels Jul 28, 2026
@illegalprime
illegalprime merged commit e44d0a3 into main Jul 29, 2026
73 of 77 checks passed
@illegalprime
illegalprime deleted the feat/db-query-tracing branch July 29, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-policy: human-approved Managed by the Review Policy workflow. server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants