feat(metrics): Add native histograms and Prometheus protobuf exposition - #856
feat(metrics): Add native histograms and Prometheus protobuf exposition#856ethanolchik wants to merge 3 commits into
Conversation
| * @returns {Function} aggregator function | ||
| */ | ||
| function AggregatorFactory(aggregatorFn) { | ||
| function AggregatorFactory(aggregatorFn, nativeAggregatorFn) { |
There was a problem hiding this comment.
I don't see why you need the second parameter.
There was a problem hiding this comment.
I added it so sum and first can pass their native histogram aggregators while keeping the existing classic histogram callbacks unchanged. The existing callbacks returns numbers whereas the native callbacks return native histogram snapshots. It's an optional field but I'm happy to structure it differently.
| function AggregatorFactory(aggregatorFn, nativeAggregatorFn) { | ||
| return metrics => { | ||
| if (metrics.length === 0) return; | ||
| const hasNativeHistograms = metrics.some( |
There was a problem hiding this comment.
And this check runs on every single metric, on every single scrape. This is not when and where to do a sanity check. That should be farther up the chain.
Also why would this happen? You get one histogram with a particular name. Either all of the values will be natives or they won't, right?
There was a problem hiding this comment.
Yeah true, this was probably more defensive than necessary. I've changed it so it uses the first snapshot only.
| if (defaultLabelNames !== undefined) { | ||
| for (const labelName of defaultLabelNames) { | ||
| seriesLabels[labelName] ??= this._defaultLabels[labelName]; | ||
| if ( |
There was a problem hiding this comment.
This fixes a case where a histogram label is null or undefined and the registry has a default for that label. The existing line applies the default to seriesLabels, but the formatter uses the value from sharedLabels instead, so the default is ignored.
I've moved this change into a separate commit in line with your other comment.
| } | ||
|
|
||
| async getMetricsAsString(metrics) { | ||
| async getMetricsAsString(metrics, contentType = this.contentType) { |
There was a problem hiding this comment.
since encodeMetricFamily doesn't return a string, this is not the way to wire this up.
You're also returning from the middle of a function now.
There was a problem hiding this comment.
Ah yeah true, that's my bad. I've moved protobuf encoding into metrics() where the output format is selected.
| if (contentType === Registry.PROMETHEUS_PROTOBUF_CONTENT_TYPE) { | ||
| const { encodeMetricFamily } = require('./protobuf'); | ||
| return encodeMetricFamily(metric, this._defaultLabels); | ||
| } |
There was a problem hiding this comment.
This should be hoisted up to the calling function, which fixes the early exit and the mismatched function name and method signature.
There was a problem hiding this comment.
Ok sure, I've moved this into metrics().
| return encodeMetricFamily(metric, this._defaultLabels); | ||
| } | ||
|
|
||
| const isOpenMetrics = contentType === Registry.OPENMETRICS_CONTENT_TYPE; |
There was a problem hiding this comment.
You've got way too many changes in a single commit. I don't see how this one or the one I questioned below are part of native histograms. I know some devs prefer large commits that tie to the ticket, but I've yet to meet one who actually does forensics in git history so I'm pretty sure that's a Chesterton's Fence situation.
If you're going to fix other bugs in a driveby I'd prefer the be done as a separate commit in the same PR. It'll also help because there's already an open PR touching some of the label code and this is going to make a hash of things.
| @@ -223,9 +250,11 @@ class Registry { | |||
| setContentType(metricsContentType) { | |||
| if ( | |||
There was a problem hiding this comment.
We have two implementations of this.
There was a problem hiding this comment.
Ah yeah cheers, all fixed now. I moved the content-type check into a shared helper that both the constructor and setContentType() use.
|
So am I correct in thinking that native histograms are incompatible with the default Prometheus data type? If so then I'm not sure how to keep this compatible with |
Signed-off-by: Ethan Olchik <eitan.olchik@gmail.com>
Signed-off-by: Ethan Olchik <eitan.olchik@gmail.com>
Extend Histogram with opt-in exponential native buckets, exemplars, and resolution reduction. Preserve native snapshots through registry, worker, and cluster aggregation. Add protobuf exposition for all existing metric types, binary registry return types, schema generation, documentation, and an HTTP example. Fixes prometheus#576 Assisted-by: Codex Signed-off-by: Ethan Olchik <eolchik@cloudflare.com>
faff229 to
6924fdf
Compare
Well, I though that because |
Applications currently cannot expose native histogram samples from this client. This adds opt-in native collection to
Histogramand the Prometheus protobuf exposition needed to scrape it.Fixes #576.
Existing histogram configurations retain classic behavior. Native histograms can retain explicit classic buckets for migration, or use
buckets: []for native-only protobuf output. Protobuf registry methods return aBuffer, represented asUint8Arrayin the public TypeScript declarations.The implementation includes:
sum,first, andomitaggregation through registries, clusters, and workers, including reconciliation of different schemas and zero thresholds.protobufjs/lightuses the checked-inlib/metrics.jsondescriptor;lib/metrics.protoandnpm run generate-protobufmake its source and regeneration available.Validation performed locally:
checkworkflow passed throughacton Node 24.21.0: ESLint, Prettier, and TypeScript.npm run benchmarkscompleted on macOS/Node 26.4.0. A focused registry comparison against upstream, with increased sampling, found no significant regression above a 5% threshold; default-label cases measured roughly 2–4% overhead.The existing benchmark suite covers classic metrics. Dedicated native workload benchmarks and application-specific rollout validation remain follow-up work. Applications select the protobuf response format themselves; HTTP Accept negotiation is outside this change.
Developed with AI assistance (Codex), also disclosed in the commit trailer.