Skip to content

Various native histogram spec updates#2752

Merged
beorn7 merged 5 commits intomainfrom
beorn7/histogram
Oct 22, 2025
Merged

Various native histogram spec updates#2752
beorn7 merged 5 commits intomainfrom
beorn7/histogram

Conversation

@beorn7
Copy link
Member

@beorn7 beorn7 commented Oct 8, 2025

See commit descriptions for details.

Let's only merge this once we are positive about how and when to tackle the related issues:

FYI:

  • @linasm (Please check if I caught the implications of NHCB reconciliations correctly.)
  • @crush-on-anechka (WRT counter reset hint implications – but as said, I plan to create a PR and will cc you on it.)

Neither +Inf nor NaN are valid custom values.

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7 beorn7 requested a review from krajorama October 8, 2025 16:16
Copy link

@linasm linasm left a comment

Choose a reason for hiding this comment

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

LGTM for the parts related to NHCB custom values (validation, reconciling).
I'm not familiar enough with counter reset hints, someone else should review that part.

@linasm
Copy link

linasm commented Oct 10, 2025

Related question: it is not very clear from the spec, whether -Inf is a valid (first, obviously) custom bound value. Currently, Validate() would reject that, likely accidentally (no tests are failing if I change that check).

Also the spec should state that -Inf is inclusive first lower bound. Otherwise it is not clear where observations with -Inf value would be counted. For the sake of symmetry - observations with +Inf value are counted in the last bucket.

@beorn7
Copy link
Member Author

beorn7 commented Oct 10, 2025

Right, we should make the handling of -Inf explicit (as we do for the standard exponential schemas).

@beorn7
Copy link
Member Author

beorn7 commented Oct 10, 2025

I added a commit for that.

WRT -Inf as the 1st custom value: I think it should be allowed. That's implicit from the spec. I don't think we have to make it explicit in the spec. Just fix the Validate method. (Will create a PR momentarily.)

@beorn7
Copy link
Member Author

beorn7 commented Oct 10, 2025

See prometheus/prometheus#17320

@beorn7
Copy link
Member Author

beorn7 commented Oct 15, 2025

I'll give prometheus/prometheus#17278 a final review ASAP. But since we already know that we want to merge it, this can be merged now, too.

@krajorama maybe give it a final pass?

We originally planned to not include this in the initial feature set.
But now prometheus/prometheus#17278 is
underway, so let's aim for it.

Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: beorn7 <beorn@grafana.com>
We are currently not really implementing this, but we should. It's
just a minor issue, but see
prometheus/prometheus#17308 to get it right.

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7
Copy link
Member Author

beorn7 commented Oct 16, 2025

I manoeuvred GH into a state where it marks all my comments as "pending" but I cannot submit them.

@beorn7
Copy link
Member Author

beorn7 commented Oct 16, 2025

image

@beorn7
Copy link
Member Author

beorn7 commented Oct 16, 2025

image

@beorn7
Copy link
Member Author

beorn7 commented Oct 16, 2025

PHAL.

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Small fix and an optional nit suggestion

Copy link
Member Author

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Thanks for the review.

However, I would like to keep both commented sections as they are. 😁

@beorn7 beorn7 merged commit a923400 into main Oct 22, 2025
5 checks passed
@beorn7 beorn7 deleted the beorn7/histogram branch October 22, 2025 11:38
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.

3 participants