feat(stats): query time histogram in OpenMetrics and OTLP - #1450
Open
alexkarp-umd wants to merge 22 commits into
Open
feat(stats): query time histogram in OpenMetrics and OTLP#1450alexkarp-umd wants to merge 22 commits into
alexkarp-umd wants to merge 22 commits into
Conversation
…FERENCE in both [otel] section and env var form
and AggregationTemporality easier to read
Make Otel::temporality_preference an Option; in ConfigAndUsers::load default it to Cumulative, or Delta when datadog_api_key is set. Move the Datadog-cumulative warning into ConfigAndUsers::check (with the match arm collapsed to a guard for readability). Add schema-only defaults so the generated JSON schema keeps the documented values instead of the derived Default (0 / null).
Moves the datadog-implied mapping into Otel::effective_temporality_preference() and calls it from the OTLP request builder. Fixes tests that install a config directly bypassing ConfigAndUsers::load
Adds a `Histogram` to `pgdog-stats` and records every completed query into it. Bucket bounds are latched process-wide at startup from the new `general.query_time_buckets` setting: histograms are indexed by position, so re-bucketing at runtime would silently reinterpret already-recorded samples. That keeps `Histogram` `Copy` and lets pool counts merge element-wise. Only `last_checkout` is bucketed, since that is what merges into the pool on check-in; bucketing `total` as well would double-count every sample. Nothing exports the histogram yet.
Adds `MeasurementType::Histogram` and renders it as the `_bucket`/`_sum`/
`_count` series the OpenMetrics spec expects, with cumulative counts and a
trailing `le="+Inf"`. A measurement can now render as several lines, so
`Metric`'s `Display` prefixes each one.
Bounds are formatted to nine decimals rather than via `{}`: Prometheus
rejects scientific notation, and two distinct bounds collapsing to the same
`le` label would fail the whole scrape.
Also adds the `OpenMetricType::Histogram` variant, which makes the OTLP
exporter's matches non-exhaustive. Both arms return nothing for now rather
than a misleading scalar, so the OTLP endpoint omits the metric entirely
until the next commit gives it a real data point.
Emits a real OTLP `Histogram` data point rather than flattening the distribution to a scalar. Bucket counts are converted from cumulative to per-bucket, and 64-bit integers are serialized as decimal strings per OTLP/JSON. Deltas are computed against the previous export. Unlike counters, which emit a full delta against zero on first sight, the first export of a histogram series is skipped: reporting a lifetime bucket distribution as one interval would skew per-interval percentiles.
Exporters branch on metric_type() and then pattern-match each measurement, so a disagreement fails silently: a histogram under a Gauge metric renders as a bare number or exports as 0.0 over OTLP, and scalars under a Histogram metric are dropped. Add MeasurementType::matches and a debug_assert! in Metric::new that rejects the mismatch at construction time, with should_panic tests covering both directions.
…ters Pools::load called Bounds::seconds() inside the per-pool loop and histogram_data_point cloned the bounds Vec again per data point: two Vec allocations per pool per scrape for process-constant data. Convert the bounds to seconds once per scrape and carry them as Arc<[f64]> on HistogramMeasurement and HistogramDataPoint, so every pool's measurement and both the OpenMetrics render and OTLP paths share the single allocation. OTLP JSON output is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review raised a set of correctness and API issues across the histogram series. None of them change an exported metric value. Recording and configuration: - Bounds::defaults() fell back to a zero-length ladder if the built-in bounds failed to parse, which would silently file every sample under +Inf. Those bounds are a const that parse cannot reject, so expect() it and pin the invariant with a test rather than degrade quietly. - Bounds are deduplicated on the f64 seconds they are exported as, rather than on Duration. Past roughly 10^7 seconds an f64 cannot resolve nanoseconds, and two bounds that render the same `le` label fail the entire Prometheus scrape. - set_bounds returns a Latch describing what happened instead of a bare bool. Losing the latch to a bounds() read before the configuration loaded is not the same as an operator changing the setting across a reload: a restart does not fix the first, and the warning now says so instead of sending the operator round a loop. - PGDOG_QUERY_TIME_BUCKETS discards the whole ladder when any element fails to parse. Keeping whichever values happened to parse built a histogram the operator never asked for, and every other setting in General is already all-or-nothing. - AddAssign merges in place and Add is defined in terms of it, rather than copying the histogram out and back. - Histogram::buckets debug-asserts that the counts it returns sum to count, since _bucket and _count reach the wire by separate routes. Export: - MeasurementType boxes its Histogram variant. The variant had taken the enum from 16 to 64 bytes, and every scalar measurement paid that padding on every scrape. - HistogramMeasurement stores per-bucket counts, matching its source. Cumulative counts are a property of the OpenMetrics `le` format, so a cumulative() helper derives them at render time and the OTLP path no longer un-accumulates what the OpenMetrics path had accumulated. OpenMetrics text and OTLP JSON are unchanged for the same input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XYLvjiaYCeQa7RvLEs4n8
Invalid bounds were dropped and the rest kept, so `[1, 2, -3, 4]` started PgDog with a three-bucket histogram and nothing in the exported metrics said which bound went missing. An operator who typos one value should hear about it, not scrape buckets they never configured. Bounds::from_millis, from_millis_checked and the Normalized enum are replaced by a single try_from_millis returning Result<Bounds, BoundsError>. A ladder is taken whole or not at all: every value must be finite, greater than zero and small enough for a Duration, and there must be at least one and no more than MAX_BUCKETS of them. Sorting and deduplication stay silent. Neither changes which bounds were asked for, so neither is worth failing over. config::set now propagates the failure, alongside check() and validate_lookup_queries(), so a bad ladder refuses startup and a bad reload leaves the running configuration untouched. This covers pgdog.toml only. A malformed PGDOG_QUERY_TIME_BUCKETS still falls back to the defaults without complaint, because every environment variable in General does — they are read through serde defaults, which have no channel to report a failure, and test_env_invalid_enum_values pins that behaviour deliberately. The asymmetry is documented on both the field and set_histogram_bounds rather than left to be discovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XYLvjiaYCeQa7RvLEs4n8
- Drop `Add`/`Sub` for `Histogram`: pooling merges with `AddAssign`, and the OTLP delta exporter computes its own differences, so the operators and their tests were dead. - Take owned per-bucket counts in `HistogramMeasurement::new` so the pools exporter moves the vector instead of cloning it. - Name the OTLP push interval (`DEFAULT_PUSH_INTERVAL`) instead of three `10_000` literals. - Correct the `DEFAULT_BOUNDS_MS` comment, which had the dependency direction backwards. Co-Authored-By: Claude <noreply@anthropic.com>
The `configured` flag picks between two distinct operator-facing warnings — `AlreadySet` (a restart applies the new ladder) and `DefaultedByRead` (a restart repeats the same ordering, so it loops). That branch sat inside the `OnceLock` wrapper, untested. Extract it into `LatchedBounds::conflict()` so the decision can be pinned down without touching the process-global latch. Co-Authored-By: Claude <noreply@anthropic.com>
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1409.
PgDog reports query latency via simple aggregates (
total_query_time,avg_query_time), so there's no way to calculate percentiles. This changeset adds a per-pool histogram of individual query durations to both OpenMetrics and OTel, sohistogram_quantile()works:This PR adds:
query_time_seconds_bucket/_sum/_count, cumulative counts with a trailingle="+Inf".query_time_seconds: aHistogramdata point withexplicit_bounds/bucket_counts(following the temporality preference from [Metrics] Datapoints follow OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE #1293)Implementation
Recording.
Stats::query()observes each query duration into a fixed-bucketHistogramin the server's local counters: a plain array ofu64, so there's no allocation and no lock on the query path. It's drained into the pool on check-in alongside the existing counts, the same waylast_checkoutalready works.Config.
query_time_bucketsin[general]— upper bounds in milliseconds, also readable fromPGDOG_QUERY_TIME_BUCKETS. Default[0.1, 0.3, 1, 3, 10, 30, 100, 300, 1000, 3000, 10000, 30000](100µs → 30s): 13 bucket series per pool including+Inf, plus_sumand_count.Notes
HistogramDataPointinbuild_request_with_state, reusesbuild_attributesunchanged, and stores previous bucket counts inCounterStaterather than a second global. The histogram commits sit on top of92ddcf3c; the rest are [Metrics] Datapoints follow OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE #1293's.pgdog-stats/src/histogram.rsis mostly self-contained business-logic/implementation. Most of the rest is plumbing to carry a second value alongsideCountsthrough check-in.otel.rshad to be adapted since histograms can't be aNumberDataPoint.value_for_data_pointandwrap_data_pointsreturnNoneforHistogramand the caller forks tohistogram_data_point. I'm not sure if this is the best approach but it does work.query_time_buckets. changing this config option requires a restart. It is not updated by a hot-reload.Follow-ups
examples/grafana_prometheus.AI use disclosure
LLMs wrote most of the code in this PR. I'm an SRE who wants this feature but I have limited Rust experience. I was thorough about designing and writing the tests. But I encourage you to review this code with extra skepticism.