Thanks for this. As said, I do not have capacity for a detailed review right now (but maybe others have). I peeked at it, though, because it is very intriguing.

Just to clarify: From the PromQL perspective, this fixes all the cases where NHCB buckets are mismatched. (While the implementation happens in the Add/Sub methods, it helps for rate calculation and everything else, not just for + and - or the sum aggregation.) I'll change the PR's title so that people won't be confused later.

I have a lot of more strategic thoughts about this, which is more around how to release this (before/after declaring NH a stable feature, and how to do it if we do the latter) and if we should have annotation when buckets are reconciled. To not pollute the review of the actual implementation here, I'll add those thoughts to #17255.

General note (not having looked at the code in detail, maybe it already works):

We also want to reconcile buckets for calculation of rate and friends.

This then leads to the problem of counter reset detection. So far, we considered each change of the custom values a counter reset. However, now we should just do the normal detection after buckets have been reconciled.

About reset detection:

In sum, let's keep it simple and safe and have an info level annotation whenever we merge NHCB buckets in any way.

I've added info level annotation, this is now ready for review.

needs an update to do the reconciliation of custom buckets and then detect the resets normally.

IIUC this is being implemented separately, in #17312?

In sum, let's keep it simple and safe and have an info level annotation whenever we merge NHCB buckets in any way.

I've added info level annotation, this is now ready for review.

Thanks. @krajorama or myself will have a look ASAP.

needs an update to do the reconciliation of custom buckets and then detect the resets normally.

IIUC this is being implemented separately, in #17312?

No, #17312 is a separate thing.

I think we just have to update the code I have linked above to handle NHCB specially and include the reconciliation logic. I don't have smart ideas right now how to do it in detail, but I think it should/could be part of this PR.

About reset detection:

prometheus/model/histogram/float_histogram.go

Lines 642 to 646 in 9e4d23d

	if h.UsesCustomBuckets() != previous.UsesCustomBuckets() || (h.UsesCustomBuckets() && !CustomBucketBoundsMatch(h.CustomValues, previous.CustomValues)) {
	// Mark that something has changed or that the application has been restarted. However, this does
	// not matter so much since the change in schema will be handled directly in the chunks and PromQL
	// functions.
	return true

needs an update to do the reconciliation of custom buckets and then detect the resets normally.

I'm not sure I understand - IMHO a change in CustomValues is a symptom of code instrumentation change, which is enough to presume that the histogram has been reset, with or without the reconcillation? What do I miss here?

I'm not sure I understand - IMHO a change in CustomValues is a symptom of code instrumentation change, which is enough to presume that the histogram has been reset, with or without the reconcillation? What do I miss here?

That was our reasoning so far, and it was OK because we would never really aggregate NHCBs with different custom values anyway. But now that we can, thanks to your efforts, I realized that there are other things that could happen. For example, somebody might decide to dynamically remove a bucket, without really resetting the underlying classic histogram. The ability of removing buckets from a cumulative histogram, even on the fly or later in storage, was sold as one of the big reasons to go cumulative back in the days… So I guess it could actually happen, or maybe is already doing this.

With the bucket reconciliation, we can now aggregate across those bucket changes, in particular calculating a rate over a timespan that includes the bucket removal. A false-positive in counter reset detection could wreak havoc with the result. (While previously, we would just not have returned a result at all and said "incompatible bucket layout".) So even if it might be rare, I do think we have to treat this correctly.

Heads up: #17312 was merged, hence the conflicts. @linasm

Sorry, DetectReset changes are not ready for review yet, but I should be able to take the comments above into account, anyway,

No worries, my algorithm probably isn't perfect either, it might skip buckets that are not used, so the loop most likely needs to iterate on the custom values (boundaries) like intersectCustomBucketBounds , but at the same time count those accumulators.

@krajorama @beorn7 I've implemented counter reset detection based on reconciled buckets, O(n) complexity, no allocations.
@krajorama I've addressed your current feedback on this PR. This is now ready for further reviews.

@krajorama I've addressed your comments regarding custom bounds intersection loop. There was another comment about consistency that I'm not sure about (see my response inline).

@krajorama I've addressed your comments regarding custom bounds intersection loop. There was another comment about consistency that I'm not sure about (see my response inline).

Hi, I've elaborated on the comment. Not super important to be honest, as the result is the same, but I'd like to avoid confusion perhaps years from now.

Will give this a final pass ASAP. Thank you both of you for the epic work so far.

@beorn7 I've addressed your feedback:

1. Updated info message as suggested.
2. Added panic in case of non-NHCB arguments, updated the comments, additionally updated method name to make the contract more obvious.
3. Added a few more sophisticated tests. Note that I have used irate(foo[2m]) * 60 to eliminate any extrapolation effects which were making query results too hard to reason about (at least for me). Hope you won't mind introducing this pattern that seems to be new here (or maybe there is a better approach?).