Skip to content

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.)

beorn7 added 4 commits October 8, 2025 15:51
Neither +Inf nor NaN are valid custom values.

Signed-off-by: beorn7 <beorn@grafana.com>
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 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

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.

2 participants