Basekick-Labs / Basekick-Labs/arc

Query observability is attached to handlers, not the engine — arcx will inherit the #801 gap

Open
#823 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
677
Forks
53
Avg merge
9h 14m
Merged PRs (30d)
164

Description

## Summary

Query observability is attached to **handlers**, not to the **engine**. Every new execution path has had to re-implement counting, and each one has gotten it subtly wrong. When arcx becomes the primary engine, this stops being a metrics nit and becomes "we cannot see the engine that runs our queries."

Filing now, while 26.09.2 has the evidence fresh, rather than after the migration.

## The pattern, three times over

| Path | Requests | Success | Errors | Result |
|---|---|---|---|---|
| `executeQuery` (JSON) | ✅ | ✅ | ✅ | correct |
| `executeQueryArrow` | ❌ → ✅ | ❌ → ✅ | ✅ | **fixed in #801 this release** |
| `query.ParallelExecutor` | ❌ | ❌ | ❌ | invisible — see #809 |
| `arcxrouter` / `arcx_hook` | ❌ | ✅ (3 sites) | ✅ (3 sites) | **same shape as #801** |

`internal/api/arcx_hook.go` has **zero** `IncQueryRequests` call sites while incrementing `IncQuerySuccess` three times and `IncQueryErrors` three times. That is precisely the #801 defect — a path that counts its outcomes but not its attempts, so `success + errors != requests` and any error-rate expression is unbounded.

The cause is the same in all three cases: **counters live at the HTTP handler, so a path that bypasses the handler bypasses the counting.**

## Why arcx makes this structural

`internal/arcxrouter/router.go:816` executes through its own `h.DB.QueryContext` rather than the `DuckDB` wrapper — the same bypass that made `arc_db_queries_total` unwireable in #809, where I instrumented `DuckDB.Query`/`Exec` and watched the counter stay at 1 through twelve writes and two queries because `query.ParallelExecutor` holds the raw `*sql.DB`.

Today arcx is an opt-in fast path, so the gap is bounded. Once it is the engine:

- `arc_query_requests_total` under-reports by however much traffic arcx serves
- `rate(errors) / rate(requests)` becomes unbounded for the majority of queries, exactly as it was for Arrow before #801
- There is still no DuckDB-level counter to fall back on, because that bypass is unchanged
- Query latency has no histogram at all (`arc_query_latency_*` does not exist; only a lifetime average in the JSON snapshot)

The failure mode is not "a metric reads zero." It is "the dashboard looks fine while most queries are invisible" — the class this release spent five issues fixing.

## Proposed direction

Move query accounting **below** the handler, to a single seam every execution path must cross, so a new engine inherits observability instead of re-implementing it.

Candidate shapes, roughly in order of preference:

1. **A `queryobs` wrapper around the execution entry point.** Every path — JSON, Arrow, msgpack, arcx, parallel executor — obtains its rows through one function that counts the attempt, the outcome, the rows and the latency. New engines get it by construction.
2. **A recorder on `QueryHandler`** that every path calls in a `defer`, so the attempt is counted even on a panic path.
3. **Instrument at the `*sql.DB` seam** (a wrapping `driver.Connector`), which catches ParallelExecutor and arcx automatically but loses the API-level distinction between "a user query" and "an internal maintenance query."

Worth deciding before the arcx migration rather than during it: retrofitting counting into an engine cutover is how #801 happened in the first place.

## Additionally worth folding in

- **Query latency histogram.** `arc_http_latency_seconds` is per-HTTP-endpoint and mixes writes and health checks. There is no query-specific histogram, so p95 query latency is not answerable from `/metrics` today.
- **An engine label.** Once two engines coexist, `arc_query_requests_total{engine="duckdb|arcx"}` is the difference between "queries are slow" and "queries are slow *on the new engine*." Engine cardinality is bounded, so the label is safe.

## Acceptance

- [ ] A single seam that every query execution path crosses, with a test that fails if a path bypasses it
- [ ] `success + errors == requests` holds across a mixed JSON/Arrow/msgpack/arcx workload, verified on a running binary
- [ ] arcx-served queries are distinguishable from DuckDB-served ones
- [ ] A query-latency histogram exists

## Related

#801 (Arrow under-counting, fixed), #809 (DuckDB pool gauges; documents the ParallelExecutor bypass), #802 umbrella.

Contributor guide

Open the contributing guide

Research direction

Start with internal/api/arcx_hook.go and internal/arcxrouter/router.go:816, then trace executeQuery, executeQueryArrow, query.ParallelExecutor, and the msgpack path to identify their execution seams. Read #801 and #809 for existing instrumentation, then verify behavior on a running mixed workload. Done means every path crosses one tested accounting seam, request totals reconcile with successes and errors, engine labels distinguish arcx and DuckDB, and a query-latency histogram is exposed.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
observability-sre
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.