goodhistogram: add Prometheus adapter layer by angles-n-daemons · Pull Request #2 · cockroachdb/goodhistogram

angles-n-daemons · 2026-04-16T19:33:36Z

Summary

Every Prometheus user needs an adapter to expose goodhistogram metrics through a registry. This adds PrometheusCollector and HistogramVec so callers can register histograms with prometheus.MustRegister and have them scraped without additional glue code. HistogramVec extends this to labeled metrics, managing a collection of Histograms partitioned by label values with on-demand creation via WithLabelValues.

The exported data includes both conventional cumulative buckets and native histogram sparse fields — no remapping required since goodhistogram's bucket layout is schema-aligned by construction.

cockroachlabs-cla-agent · 2026-04-16T19:33:50Z

All committers have signed the CLA.

angles-n-daemons · 2026-04-20T18:31:40Z

Buddy, we might need to split this into two changes which build off each other:

Collector + Vector support
Windowed histograms

Let me know if that seems like a reasonable breakdown, it seems difficult to break this changeset into non-dependent changes.

alyshanjahani-crl · 2026-04-28T19:05:28Z

+	ch <- p.desc
+}
+
+// Collect implements prometheus.Collector.


Collect sends p (the collector itself) as the metric. This means the same object is both the prometheus.Collector and the prometheus.Metric. This works with the current Prometheus library but is unusual and means re-registering or using the collector in multiple registries would be problematic.

More importantly, a single PrometheusCollector can only represent one metric, so this is correct for the current use case, but the design conflates two distinct roles.

CockroachDB's existing approach in metric.go separates these: histograms implement PrometheusExportable (which has ToPrometheusMetric()) and the registry handles collection. In the HistogramVec below in this same PR, a separate histogramMetric struct is used (vec.go:117-128), which is the cleaner pattern. Consider doing the same for PrometheusCollector for consistency.

CockroachDB's metric system defines PrometheusExportable (GetType(), ToPrometheusMetric()) and PrometheusVector (ToPrometheusMetrics()) interfaces. When goodhistogram is eventually integrated into CockroachDB, the adapter layer here will need to bridge to those interfaces. It might be worth considering whether PrometheusCollector should also implement a ToPrometheusMetric() *prometheusgo.Metric method (it essentially does this in Write already) to ease future integration.

Good ideas, I made the collector create a separate histogramMetric, and added a ToPrometheusMetric function for easy integration

alyshanjahani-crl · 2026-04-28T19:07:12Z

+// WithLabelValues returns the Histogram for the given label values,
+// creating it if it doesn't exist. Panics if the number of values
+// doesn't match the number of label names.
+func (v *HistogramVec) WithLabelValues(lvs ...string) *Histogram {


do we need cardinality protection here?

In CockroachDB, this is a known concern — HighCardinalityHistogram in aggmetric/histogram.go uses cache-based eviction with configurable MaxLabelValues (default 5000). The SQLHistogram similarly has cardinality controls.

Without any limit, a caller passing user-controlled label values (e.g., SQL query fingerprints, user IDs) could cause unbounded memory growth. Consider:

Adding a MaxChildren option to HistogramVec with a default cap.

Alternatively, documenting that callers must ensure bounded cardinality, since this is a library not directly exposed to user input.

Prometheus doesn't do bounding on cardinality (link), does it feel okay to match that pattern, and push bounding up to the consumers of the library?

ah cool, yeah that sounds fine

alyshanjahani-crl · 2026-04-28T19:09:22Z

+			len(v.labelNames), len(lvs),
+		))
+	}
+	key := strings.Join(lvs, "\x00")


Are we concerned at all about key collisions due to this literal/separator?

After checking, we changed it '0xFF' instead of 0x00. Does that feel better?

According to claude:

Prometheus's client_golang does not use \x00 as a separator. It uses model.SeparatorByte which is 255 (0xFF). However, the pattern is also different in a more important way: Prometheus hashes the separator-joined label values into a uint64 and uses that as the map key (in hashLabelValues() in vec.go), rather than using the raw joined string. So collisions there are handled by the hash collision resolution, not by relying on the separator being unambiguous.

That said, Brian's updated code does now use \xff (matching Prometheus's separator byte value), and the collision risk is theoretical — label values in Prometheus are valid UTF-8, and 0xFF is not a valid UTF-8 byte, so in practice no well-formed label value would contain it.

So yeah seems like we're ok here.

alyshanjahani-crl · 2026-04-28T19:11:07Z

+
+// DeleteLabelValues removes the Histogram for the given label values.
+// Returns true if the entry existed.
+func (v *HistogramVec) DeleteLabelValues(lvs ...string) bool {


Do we need the same len(lvs) len(v.labelNames) panic/check here as we have in WithLabelValues?

We do! adding it now

Every Prometheus user needs an adapter to expose goodhistogram metrics through a registry. PrometheusCollector wraps a *Histogram as a prometheus.Collector, implementing Describe/Collect/Write so that histograms can be registered with prometheus.MustRegister and scraped without additional glue code. HistogramVec extends this to labeled metrics, managing a collection of Histograms partitioned by label values with on-demand creation via WithLabelValues. The exported data includes both conventional cumulative buckets and native histogram sparse fields — no remapping required since goodhistogram's bucket layout is schema-aligned by construction. Co-Authored-By: roachdev-claude <[email protected]>

Cumulative histograms are the right primitive for Prometheus scraping, but operators and dashboards need rolling-window quantiles to see recent behavior. Windowed maintains a single cumulative histogram with two baseline snapshots rotated on a configurable interval. The windowed view is computed by subtracting the older baseline from the current cumulative state, covering 1-2x the window duration. Recording cost is identical to a plain Histogram (~20ns, lock-free); the mutex only protects baseline rotation. WindowedCollector and WindowedVec provide Prometheus integration for single and labeled windowed histograms, following the same adapter pattern as PrometheusCollector and HistogramVec. Co-Authored-By: roachdev-claude <[email protected]>

goodhistogram: add windowed histogram with Prometheus integration

angles-n-daemons force-pushed the bdillmann/prometheus-collector branch from 79f22bd to 83a0ed6 Compare April 16, 2026 20:06

angles-n-daemons changed the title ~~goodhistogram: add PrometheusCollector for registry integration~~ goodhistogram: add Prometheus adapter and windowed histogram Apr 20, 2026

angles-n-daemons force-pushed the bdillmann/prometheus-collector branch from 718efb4 to 06b75d4 Compare April 20, 2026 18:27

angles-n-daemons force-pushed the bdillmann/prometheus-collector branch from 06b75d4 to 5748700 Compare April 27, 2026 14:11

angles-n-daemons changed the title ~~goodhistogram: add Prometheus adapter and windowed histogram~~ goodhistogram: add Prometheus adapter layer Apr 27, 2026

angles-n-daemons mentioned this pull request Apr 27, 2026

goodhistogram: add windowed histogram with Prometheus integration #4

Merged

angles-n-daemons requested a review from alyshanjahani-crl April 27, 2026 17:40

alyshanjahani-crl reviewed Apr 28, 2026

View reviewed changes

angles-n-daemons force-pushed the bdillmann/prometheus-collector branch 3 times, most recently from 0b75663 to 824c291 Compare April 29, 2026 14:28

angles-n-daemons force-pushed the bdillmann/prometheus-collector branch from 824c291 to 765e86b Compare April 29, 2026 14:41

alyshanjahani-crl approved these changes Apr 29, 2026

View reviewed changes

angles-n-daemons and others added 3 commits May 4, 2026 10:00

Merge branch 'main' into bdillmann/prometheus-collector

6941efb

Merge pull request #4 from cockroachdb/bdillmann/windowed-histogram

796027d

goodhistogram: add windowed histogram with Prometheus integration

angles-n-daemons merged commit 48f109a into main May 6, 2026
3 checks passed

angles-n-daemons deleted the bdillmann/prometheus-collector branch May 6, 2026 15:28

Conversation

angles-n-daemons commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

cockroachlabs-cla-agent Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angles-n-daemons commented Apr 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

angles-n-daemons commented Apr 16, 2026 •

edited

Loading

cockroachlabs-cla-agent Bot commented Apr 16, 2026 •

edited

Loading