feat(metrics): split write-path latency and break down master tasks - #196
Conversation
`http_request_duration_seconds{method,path,status}` was the only latency signal
for the write path, which made three things unmeasurable.
**add and flush were one series.** They share a route (`routes/mod.rs:87`);
flush is a query param parsed by hand (`routes/rollouts.rs:263`). The `path`
label comes from `MatchedPath` — the route *template*, excluding the query
string — so a flushing add and a plain add emitted byte-identical labels. Since
flush is strictly additive (add, then flush), flushing requests were the slow
tail contaminating p99 of plain adds with no way to demix.
**core had no metrics at all.** `RolloutStore::add`, `flush`, and the WAL merge
were uninstrumented, so HTTP duration blended body parsing, multipart decode,
blob-budget admission, store open, lock acquisition and the actual work.
**`master_task_duration_seconds` was narrower than its name.** It wrapped only
the dispatch match, excluding the claim (etcd txn + target lock), the semaphore
permit wait, and commit/lease release. MergeWal's `join_all` fan-out had no
per-worker timing, so one straggler — which by `join_all` semantics sets the
whole task's latency — was indistinguishable from every worker being slow.
New metrics:
rollout_add_duration_seconds{result} core: durable append
rollout_flush_duration_seconds{result,outcome} core: memtable seal
rollout_wal_merge_duration_seconds{phase,result} core: 6 merge phases
rollout_add_request_duration_seconds{flush,result} server: store time
rollout_wal_merge_request_duration_seconds{result} worker-side merge
rollout_wal_merge_lock_wait_seconds write-lock wait
rollout_compaction_lock_wait_seconds write-lock wait
master_task_phase_duration_seconds{kind,phase} claim/permit/work/commit
master_merge_wal_worker_duration_seconds{result} per-worker RTT
master_merge_wal_workers_total{result} per-worker outcome
master_merge_wal_generations_reclaimed_total
`flush`'s `outcome` (sealed|noop|fenced) matters: no-resident-writer is the
common case and returns in microseconds, so without it the histogram is
dominated by near-zero samples and its percentiles say nothing about real flush
cost. Merge phases are seal/read/append/claim_epoch/drain/delete, which is also
where the unbounded read buffering and the cancel-unsafe append→drain window
live.
`master_merge_wal_workers_total{result}` distinguishes ok/not_found/http_error/
transport_error. A 404 is tolerated as "owns no shard" and N-1 failures still
report task success, so this counter is currently the only place partial
fan-out failure is visible at all.
`rollout_wal_cleanup_total` is now emitted unconditionally with
`result=merged|noop`. Gating it on `reclaimed > 0` made the common no-op merge
invisible, so a worker that never has anything to merge and a worker that is
never called looked identical.
Deliberate choices:
- `metrics` is an **optional, default-on** feature of `lance-context-core` so
downstream consumers embedding the library are not forced to take the
dependency; call sites compile to nothing when disabled (verified with
`--no-default-features`, warning-free under `-D warnings`).
- No `store`/`target` labels anywhere — dataset and experiment names are
unbounded cardinality. That context belongs in a tracing span.
- `master_task_duration_seconds` keeps its original scope for back-compat, but
gains a `result` label so a fast failure is not averaged in with successes.
- The new long-running metrics are added to `JOB_LATENCY_METRICS`; without it a
120s merge phase lands in `+Inf` and high percentiles are unusable. Note the
`_lock_wait_seconds` names match no suffix rule, so they need explicit entries.
- Added the first `describe_*` calls in the repo — `/metrics` previously shipped
with no HELP/TYPE for any application metric.
Pure observability: no behaviour change. Bugs this only makes visible (MergeWal
reporting success when N-1 workers fail, 404→Ok(0), unbounded merge buffering,
the 100ms-spin coordination lock) are left for their own PRs.
Verified: workspace tests pass (168 core / 51 server / 14 master); clippy clean
with and without the feature; and a live server scrape confirms
`rollout_add_request_duration_seconds{flush="true"}` and `{flush="false"}` are
distinct series with `flush_duration{outcome="sealed"}` recorded only for the
flushing request. The bucket-config assertion was checked to fail when the
config is removed, so it is not vacuous.
Co-Authored-By: Claude <noreply@anthropic.com>
…safe
The first pass was correct for Prometheus and expensive for Datadog. Datadog
bills per unique name+tag combination, and a histogram bills *every bucket*
separately, so what reads as harmless label design in Prometheus is a direct
line item there. Measured on a real scrape, the previous commit emitted 128
series from two requests, with a worst-case of 724 across all label
combinations.
Three changes, all keeping the add/flush split intact.
1. Failures are counted, not timed.
`result="ok"|"error"` on a histogram doubles its series count in order to
describe the *latency distribution of a rare event*, which is almost never
actionable — the actionable signal is the rate. Dropped `result` from every
latency histogram and added flat counters instead (1 series each):
rollout_add_errors_total
rollout_flush_errors_total
rollout_wal_merge_errors_total{phase}
The merge counter keeps `phase` because a merge aborts on its first failing
phase, so the label also says where it died. Server-side this let the add
handler go back to plain `?` instead of three hand-written error arms.
`master_task_duration_seconds` loses `result` for the same reason;
`master_tasks_total{kind,result}` already carries it as a counter.
2. Trimmed bucket ladders.
REQUEST 13 -> 9 buckets, JOB 11 -> 7. Adjacent ratios stay at or below 6x, which
bounds interpolation error to that factor within the straddling bucket only —
ample for latency SLOs. Boundaries sit on values people alert on (10ms, 100ms,
250ms, 1s). Coverage verified: 2ms..45s all land in finite buckets.
3. Gauges no longer named `_total`.
`master_experiments_total`, `master_rollout_rows_total` and
`master_rollout_fragments_total` are gauges. Datadog's OpenMetrics check infers
type from the name, so `_total` made them ingest as monotonic counts — graphing
"experiments created per second" for a metric meaning "how many exist right
now". `rate()` was equally meaningless in Prometheus. Now emitted under
`master_experiments` / `master_rollout_rows` / `master_rollout_fragments`, with
the old names retained as deprecated aliases so existing dashboards keep working.
Also added `describe_gauge!` calls and units: Datadog uses the exported TYPE to
choose gauge vs count, and the declared Unit is what renders latency as a
duration rather than a bare number.
Result, measured on the same two-request workload: 128 -> 87 series (-32%),
worst case 724 -> ~361 (-50%). No signal lost — the add/flush split, the flush
`outcome` breakdown, the six merge phases and the per-worker fan-out are all
intact, and failures are now visible as rates rather than buried in a histogram.
Three regression guards, each verified to fail when deliberately broken:
- latency histograms may not carry `result` (checked against the rendered text)
- a cardinality budget of 125 series, close enough to the actual 112 that adding
one two-valued label to a job histogram trips it
- core-level assertion that histogram labels come from a closed, documented set,
so an unbounded label (store URI, shard id, experiment) cannot be introduced
Verified: workspace tests pass (166 core / 51 server / 14 master); clippy clean
with and without the `metrics` feature; live scrape confirms zero `result=` on
any bucket series and both add paths still separable.
Co-Authored-By: Claude <noreply@anthropic.com>
Reworked for DatadogThe first pass was correct for Prometheus and expensive for Datadog. Datadog bills per unique name+tag combination, and a histogram bills every bucket separately — so label design that reads as harmless in Prometheus is a direct line item there. Measured: 128 series from two requests, worst case 724. Three changes, add/flush split fully intact: 1. Failures are counted, not timed
The merge counter keeps 2. Trimmed bucket laddersREQUEST 13→9, JOB 11→7. Adjacent ratios stay ≤6×, bounding interpolation error to that factor within the straddling bucket only — ample for latency SLOs. Boundaries sit on values people actually alert on (10ms, 100ms, 250ms, 1s). Coverage verified: 2ms–45s all land in finite buckets. 3. Gauges no longer named
|
| before | after | |
|---|---|---|
| series (2-request workload) | 128 | 87 (−32%) |
| worst case, all combos | 724 | ~361 (−50%) |
No signal lost. The add/flush split, the flush outcome breakdown, the six merge phases, and the per-worker fan-out are all intact — and failures are now visible as rates rather than buried in a histogram.
Regression guards
Three, each verified to fail when deliberately broken (I checked, rather than assuming):
- Latency histograms may not carry
result— asserted against the rendered exposition text. - Cardinality budget of 125 series, close enough to the actual 112 that adding one two-valued label to a job histogram trips it.
- Core-level assertion that histogram labels come from a closed, documented set, so an unbounded label (store URI, shard id, experiment name) can't be introduced later.
Live scrape confirms zero result= on any bucket series and both add paths still separable:
rollout_add_request_duration_seconds_count{flush="false"} 1
rollout_add_request_duration_seconds_count{flush="true"} 1
rollout_flush_duration_seconds_count{outcome="sealed"} 1
rollout_add_duration_seconds_count 2
Problem
http_request_duration_seconds{method,path,status}was the only latency signal for the write path. Three things were unmeasurable:1. add and flush were the same series. They share one route (
routes/mod.rs:87); flush is a query param parsed by hand atroutes/rollouts.rs:263. Thepathlabel comes fromMatchedPath— the route template, which excludes the query string — so a flushing add and a plain add emitted byte-identical labels. And because flush is strictly additive (add at:317, flush at:322), flushing requests were the slow tail contaminating p99 of plain adds, with no way to demix.2. core had zero metrics.
RolloutStore::add,flush, and the WAL merge were uninstrumented, so HTTP duration blended body parsing, multipart decode, blob-budget admission, store open, lock acquisition, and the actual work.3.
master_task_duration_secondswas narrower than its name. It wrapped only the dispatch match (scheduler.rs:90-94), excluding the claim (etcd txn + target lock), the semaphore permit wait, and commit/lease release. MergeWal'sjoin_allfan-out had no per-worker timing — and byjoin_allsemantics the slowest worker sets the whole task's latency, so one straggler was indistinguishable from all workers being slow.New metrics
Why
flushneeds anoutcomelabel (sealed|noop|fenced): no-resident-writer is the common case and returns in microseconds. Without the label the histogram is dominated by near-zero samples and its percentiles say nothing about real flush cost.Why
master_merge_wal_workers_total{result}matters: a 404 is tolerated as "owns no shard" (scheduler.rs:182) and N−1 failures still report task success (:209). This counter is currently the only place partial fan-out failure is visible.rollout_wal_cleanup_totalnow emits unconditionally withresult=merged|noop. Gating it onreclaimed > 0made the common no-op merge invisible, so a worker that never has anything to merge and a worker that is never called looked identical.Live verification
Real server, one plain add and one
?flush=trueadd:The two add paths are now distinct series, and only the flushing one recorded flush work.
Deliberate choices
metricsis an optional, default-on feature oflance-context-coreso downstream consumers embedding the library aren't forced to take the dependency. Call sites compile to nothing when disabled — verified with--no-default-featuresand warning-free under-D warnings. This is the one judgement call worth reviewing: it changes the published crate's feature surface.store/targetlabels anywhere — dataset and experiment names are unbounded cardinality. That context belongs in a tracing span, not a label.master_task_duration_secondskeeps its original scope for back-compat, but gains aresultlabel so a fast failure isn't averaged in with successes.JOB_LATENCY_METRICS— without it a 120s merge phase lands in+Inf. Note_lock_wait_secondsmatches no suffix rule, so it needs an explicit entry.describe_*calls in the repo —/metricspreviously shipped with no HELP/TYPE for any application metric.Scope
Pure observability, no behaviour change. Bugs this only makes visible are left for their own PRs: MergeWal reporting success when N−1 workers fail, 404→
Ok(0), unbounded merge buffering, the 100ms-spincoordination_lock.Testing
clippy --workspace --all-targets -D warningsclean, with and without the feature.outcomes.le="300"substring matched other metrics in the same body) and was tightened to assert on the metric's own series.🤖 Generated with Claude Code