From 9bf6058982d9029a604101bc5aaad4dd9dfc58a6 Mon Sep 17 00:00:00 2001 From: Beinan Date: Fri, 24 Jul 2026 21:46:57 +0000 Subject: [PATCH] feat(metrics): export latency as bucketed histograms, not summaries With no bucket configuration, metrics-exporter-prometheus 0.16 renders every histogram!() as a rolling summary (quantile series + monotonic _sum/_count) rather than a bucketed Prometheus histogram. That makes latency hard to reason about operationally: _sum/_count give a since-process-start average that never decays, and the exporter's summary quantiles use an opaque slow-decaying window, so a p99 stays pinned at an old spike long after latency recovers and can't be recomputed over an arbitrary window at query time. Configure explicit buckets so all *_duration_seconds metrics export as true histograms (_bucket{le="..."}), letting downstream compute histogram_quantile() over any window: - Suffix("_duration_seconds") -> request-scale buckets (5ms..60s), covering http_request_duration_seconds and master_scan_duration_seconds. - Full-name overrides for the job-scale metrics master_task_duration_seconds and rollout_compaction_duration_seconds -> coarser buckets extending to 1800s. A Full matcher outranks the Suffix matcher (exporter precedence is Full > Prefix > Suffix), so minute-plus jobs don't collapse into +Inf and distort high percentiles. Counters are unaffected. Extends the metrics test to assert histogram TYPE + _bucket series and the absence of summary quantiles. Co-Authored-By: Claude --- crates/lance-context-metrics/src/lib.rs | 73 ++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/crates/lance-context-metrics/src/lib.rs b/crates/lance-context-metrics/src/lib.rs index f718d79..e5882e4 100644 --- a/crates/lance-context-metrics/src/lib.rs +++ b/crates/lance-context-metrics/src/lib.rs @@ -19,9 +19,32 @@ use axum::{ routing::get, Router, }; -use metrics_exporter_prometheus::{PrometheusBuilder, PrometheusHandle}; +use metrics_exporter_prometheus::{Matcher, PrometheusBuilder, PrometheusHandle}; use metrics_process::Collector; +/// Explicit histogram buckets (upper bounds, seconds) for request-scale latency +/// metrics — anything matching the `_duration_seconds` suffix. Tuned for +/// sub-second-to-tens-of-seconds work like HTTP requests and rollout scans. +const REQUEST_LATENCY_BUCKETS: &[f64] = &[ + 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.5, 5.0, 10.0, 30.0, 60.0, +]; + +/// Coarser buckets (upper bounds, seconds) for long-running background jobs +/// (compaction, WAL merge, index builds) whose latency can reach minutes. +/// Without the extended tail every sample over 60s would fall into `+Inf`, +/// pinning `histogram_quantile` for high percentiles at the last finite bucket. +const JOB_LATENCY_BUCKETS: &[f64] = &[ + 0.1, 0.5, 1.0, 5.0, 10.0, 30.0, 60.0, 120.0, 300.0, 600.0, 1800.0, +]; + +/// Metric names whose latency is job-scale rather than request-scale. A `Full` +/// matcher outranks the `_duration_seconds` `Suffix` matcher (the exporter +/// applies Full > Prefix > Suffix), so these get [`JOB_LATENCY_BUCKETS`]. +const JOB_LATENCY_METRICS: &[&str] = &[ + "master_task_duration_seconds", + "rollout_compaction_duration_seconds", +]; + /// Handle used to render the Prometheus exposition text on demand, plus a /// process-resource collector that is refreshed on each scrape. #[derive(Clone)] @@ -34,8 +57,25 @@ pub struct MetricsHandle { /// /// Must be called exactly once, before any metrics are emitted. Panics if a /// recorder was already installed (mirrors the metrics ecosystem contract). +/// +/// Latency histograms (`*_duration_seconds`) are configured with explicit +/// buckets so they export as true Prometheus histograms (`_bucket{le="..."}`) +/// rather than the exporter's default rolling summaries. This lets downstream +/// compute `histogram_quantile()` over an arbitrary window at query time +/// instead of reading an exporter-internal, slow-decaying quantile. pub fn install_recorder() -> MetricsHandle { - let prometheus = PrometheusBuilder::new() + let mut builder = PrometheusBuilder::new() + .set_buckets_for_metric( + Matcher::Suffix("_duration_seconds".to_string()), + REQUEST_LATENCY_BUCKETS, + ) + .expect("request latency buckets are non-empty"); + for name in JOB_LATENCY_METRICS { + builder = builder + .set_buckets_for_metric(Matcher::Full((*name).to_string()), JOB_LATENCY_BUCKETS) + .expect("job latency buckets are non-empty"); + } + let prometheus = builder .install_recorder() .expect("failed to install Prometheus recorder"); @@ -103,6 +143,10 @@ mod tests { // tests don't double-install. let handle = install_recorder(); metrics::counter!("test_counter_total").increment(7); + // A request-scale and a job-scale latency sample, to assert both bucket + // sets render as true histograms rather than summaries. + metrics::histogram!("http_request_duration_seconds").record(0.3); + metrics::histogram!("master_task_duration_seconds").record(45.0); let app = metrics_router(handle); let resp = app @@ -121,5 +165,30 @@ mod tests { .unwrap(); let text = String::from_utf8(bytes.to_vec()).unwrap(); assert!(text.contains("test_counter_total 7"), "body: {text}"); + + // Latency metrics must export as bucketed histograms, not summaries. + assert!( + text.contains("# TYPE http_request_duration_seconds histogram"), + "request latency should be a histogram, not a summary; body: {text}" + ); + assert!( + text.contains("http_request_duration_seconds_bucket{le=\"1\"}"), + "request latency should expose _bucket series; body: {text}" + ); + assert!( + !text.contains("http_request_duration_seconds{quantile="), + "request latency must not export summary quantiles; body: {text}" + ); + + // Job-scale metric gets the extended tail (a 300s bucket exists), so a + // 45s sample is not lumped straight into +Inf. + assert!( + text.contains("# TYPE master_task_duration_seconds histogram"), + "job latency should be a histogram; body: {text}" + ); + assert!( + text.contains("master_task_duration_seconds_bucket{le=\"300\"}"), + "job latency should use the extended (job) bucket set; body: {text}" + ); } }