Basekick-Labs / Basekick-Labs/arc
Query observability is attached to handlers, not the engine — arcx will inherit the #801 gap
- 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
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