Skip to content

feat(histogram): add native histograms - #856

Open
ethanolchik wants to merge 2 commits into
prometheus:mainfrom
ethanolchik:feat/native-histograms
Open

ethanolchik wants to merge 2 commits into
prometheus:mainfrom
ethanolchik:feat/native-histograms

Conversation

@ethanolchik

@ethanolchik ethanolchik commented Sep 10, 2026 •

Copy link
Copy Markdown

Summary

Add opt-in native histogram collection to Histogram.

This PR is stacked on #860, which adds the Prometheus protobuf exposition used to scrape native histograms. Until #860 is merged, GitHub's diff against main includes that prerequisite; the native-histogram work is the single commit on top of feat/protobuf-exposition.

This adds:

  • Standard exponential schemas -4 through 8 with positive, negative, and zero buckets.
  • Configurable bucket factor, zero threshold, and populated-bucket budget.
  • Progressive resolution reduction when the bucket budget is exceeded, with a soft limit at schema -4.
  • Native exemplars and JSON-compatible snapshots.
  • Dedicated sumNative and firstNative aggregation through registries, clusters, workers, and orderly shutdown.
  • Schema and zero-threshold reconciliation during aggregation.
  • Public TypeScript configuration and snapshot types, documentation, tests, and an HTTP example.

Classic buckets remain enabled by default for migration and text compatibility; buckets: [] can be used for native-only protobuf output.

The README documents the bucket-saturation behavior and the requirement to coordinate resets across all contributors to an aggregate.

Fixes #576.

Split PRs

The original combined PR has been split as requested:

Testing

  • Full test suite, lint, formatting, and TypeScript checks pass locally.
  • Native aggregation tests cover schema reduction, zero-threshold reconciliation, labels, exemplars, worker/cluster paths, and retained shutdown snapshots.

Developed with AI assistance.

@ethanolchik ethanolchik changed the title Add native histograms and Prometheus protobuf exposition feat(metrics): Add native histograms and Prometheus protobuf exposition Sep 10, 2026
Comment thread lib/histogram.js Outdated
Comment thread lib/metricAggregators.js Outdated
* @returns {Function} aggregator function
*/
function AggregatorFactory(aggregatorFn) {
function AggregatorFactory(aggregatorFn, nativeAggregatorFn) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why you need the second parameter.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be curious to see how the other clients solve this. Do we need to reuse the aggregators across two incompatible types? I'll see if I can look this up.

Comment thread lib/metricAggregators.js Outdated
function AggregatorFactory(aggregatorFn, nativeAggregatorFn) {
return metrics => {
if (metrics.length === 0) return;
const hasNativeHistograms = metrics.some(

@jdmarshall jdmarshall Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah true, this was probably more defensive than necessary. I've changed it so it uses the first snapshot only.

Comment thread lib/registry.js Outdated
Comment thread lib/registry.js Outdated
if (defaultLabelNames !== undefined) {
for (const labelName of defaultLabelNames) {
seriesLabels[labelName] ??= this._defaultLabels[labelName];
if (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this fixing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread lib/registry.js Outdated
}

async getMetricsAsString(metrics) {
async getMetricsAsString(metrics, contentType = this.contentType) {

@jdmarshall jdmarshall Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah true, that's my bad. I've moved protobuf encoding into metrics() where the output format is selected.

Comment thread lib/registry.js Outdated
Comment on lines +73 to +76
if (contentType === Registry.PROMETHEUS_PROTOBUF_CONTENT_TYPE) {
const { encodeMetricFamily } = require('./protobuf');
return encodeMetricFamily(metric, this._defaultLabels);
}

@jdmarshall jdmarshall Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be hoisted up to the calling function, which fixes the early exit and the mismatched function name and method signature.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sure, I've moved this into metrics().

Comment thread lib/registry.js Outdated
return encodeMetricFamily(metric, this._defaultLabels);
}

const isOpenMetrics = contentType === Registry.OPENMETRICS_CONTENT_TYPE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ethanolchik ethanolchik Sep 12, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yeah I totally understand. I've split it into three commits now:

  • preserve counter names during OpenMetrics output (c45baee)
  • apply defaults to nullish histogram labels (43a3419)
  • native histograms and protobuf (6924fdf)

Comment thread lib/registry.js Outdated
@jdmarshall

Copy link
Copy Markdown
Contributor

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 worker.shutdown()

@ethanolchik
ethanolchik force-pushed the feat/native-histograms branch from faff229 to 6924fdf Compare September 12, 2026 18:03
@ethanolchik

Copy link
Copy Markdown
Author

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 worker.shutdown()

Well, I though that because worker.shutdown() transfers JSON snapshots rather than formatted text, the native data is included regardless of the worker registry’s content type and preserved as objects by the coordinator. So Protobuf is
only needed on the registry serving the final scrape to expose the native buckets.

@jdmarshall

Copy link
Copy Markdown
Contributor

Can you rebase on main? There were some benchmark changes and another PR that may or may not conflict, and I'm interested in seeing if this slows down metrics()

@ethanolchik
ethanolchik force-pushed the feat/native-histograms branch from 6924fdf to 527ad33 Compare September 20, 2026 18:52
@ethanolchik

Copy link
Copy Markdown
Author

Can you rebase on main? There were some benchmark changes and another PR that may or may not conflict, and I'm interested in seeing if this slows down metrics()

rebased

@jdmarshall

Copy link
Copy Markdown
Contributor

Thanks!

I finally took a peek at the python code. The aggregation is pretty much only for multiprocess work, so we are free to name aggregators anything we want to name them.

I suspect the simpler solution here is to introduce new aggregators for the native histograms, and establish a default aggregator for those types. So a 'sum-native' or 'sumNative' type instead of changing the factory function to take 2 parameters one of which is never used for 99% of all stats generated.

@ethanolchik

Copy link
Copy Markdown
Author

Yeah that sounds simpler, thanks for that!

I'll give native histograms their own aggregators and default them to sumNative. I'll also make sure shutdown picks those up, since it currently only keeps metrics using sum.

@krajorama krajorama left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at this from native histograms point of view - I've worked with native histograms for the past 3 years in Prometheus.

LGTM from native histograms point of view, with a few caveats:

  • one can implement more sophisticated ways to keep the number of buckets down and try to avoid reducing the resolution (called coarsen here), which is done in client_golang. Probably fine for now, but should be explained more in the README that there is no protection if a histogram is saturated other than restarting the program or implementing some monitoring and reset over histograms.
  • for exemplars it would be nice if the number was configurable and maybe the client_golang algorithm was applied to get a distribution that's more likely to keep outliers - which we assumed are more valuable insight - nothing to do in this PR though
  • classic and native histograms can observer NaN and +- infinity, but it's an existing limitation that they are rejected - nothing to do in this PR

(Also I think this PR is a bit large, it could be split up into at least: 2 PRs for the two bugfixes in the first two commit, a PR for adding Protobuf and a PR for adding native histograms).

Comment thread README.md Outdated
Comment thread lib/nativeHistogram.js Outdated
this.zeroCount = 0;
this.positiveBuckets = new Map();
this.negativeBuckets = new Map();
this.createdTimestamp = nowTimestamp();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm loving this, but it means counters and summaries and classic histograms are now different and don't have created timestamp. So maybe this should be commented out with a TODO() on top.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, i'll leave this out for now 👍

Comment thread lib/histogram.js
}

/**
* Initialize the metrics for the given combination of labels to zero.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function needs a warning that prohibits usage in flight when there's also sum or sumNative aggregation being used. The reason is that if you reset a histogram in the sum*, then the sum is no longer guaranteed to be monotonic. This is best illustrated if you imagine having zero() function on counters. Assume you have two counters that are at count==1 , then you reset of them and leave it at 0. The sum will look like: 2 2 2 2 1 1 1 1 . Which means that PromQL will measure an increase of 1 over this time range as it will detect a reset at the 5th sample.

So what you'd actually have to do is to reset all counters/histograms that you aggregate.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah I see, thanks for that example. I've added a warning to both zero() and reset() explaining that resetting as an individual contributor to a sum or sumNative aggregate can cause Prometheus to detect a reset of the whole combined series. I've also put it in the README.

@ethanolchik

Copy link
Copy Markdown
Author

I've looked at this from native histograms point of view - I've worked with native histograms for the past 3 years in Prometheus.

LGTM from native histograms point of view, with a few caveats:

  • one can implement more sophisticated ways to keep the number of buckets down and try to avoid reducing the resolution (called coarsen here), which is done in client_golang. Probably fine for now, but should be explained more in the README that there is no protection if a histogram is saturated other than restarting the program or implementing some monitoring and reset over histograms.
  • for exemplars it would be nice if the number was configurable and maybe the client_golang algorithm was applied to get a distribution that's more likely to keep outliers - which we assumed are more valuable insight - nothing to do in this PR though
  • classic and native histograms can observer NaN and +- infinity, but it's an existing limitation that they are rejected - nothing to do in this PR

(Also I think this PR is a bit large, it could be split up into at least: 2 PRs for the two bugfixes in the first two commit, a PR for adding Protobuf and a PR for adding native histograms).

Thanks for taking a look!

I'll split this PR up as you suggested to make things easier. I've also expanded the README to explain what happens when the histogram reaches its lowest resolution, including that there's no automatic recovery or reset. I'll leave the rest as-is.

Signed-off-by: Ethan Olchik <eitan.olchik@gmail.com>
Signed-off-by: Ethan Olchik <eitan.olchik@gmail.com>
@ethanolchik
ethanolchik force-pushed the feat/native-histograms branch from f2b0194 to 0d97517 Compare September 28, 2026 16:50
@ethanolchik ethanolchik changed the title feat(metrics): Add native histograms and Prometheus protobuf exposition feat(histogram): add native histograms Sep 28, 2026
@ethanolchik

ethanolchik commented Sep 28, 2026 •

Copy link
Copy Markdown
Author

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for native histograms

3 participants