Skip to content

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Jul 11, 2024

This adds equivalent NHCB tests to the existing classic histogram tests.

@beorn7 beorn7 requested a review from roidelapluie as a code owner July 11, 2024 22:50
@beorn7
Copy link
Member Author

beorn7 commented Jul 11, 2024

@zenador as promised. 😄

@beorn7 beorn7 requested review from krajorama and removed request for roidelapluie July 11, 2024 22:51
@@ -103,156 +113,314 @@ eval instant at 50m histogram_fraction(0, 0.2, rate(testhistogram3[5m]))
{start="positive"} 0.6363636363636364
{start="negative"} 0

# Test histogram_quantile.
# In the classic histogram, we can acces the corresponding bucket (if
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# In the classic histogram, we can acces the corresponding bucket (if
# In the classic histogram, we can access the corresponding bucket (if

Copy link
Contributor

@zenador zenador 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 doing this, LGTM!

krajorama
krajorama previously approved these changes Jul 16, 2024
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.

Approved with tiny nit

eval instant at 47m histogram_quantile(5./6., rate(testhistogram2_bucket[15m]))
{} 5

# Aggregated histogram: Everything in one.
# Aggregated histogram: Everything in one. Note how NHCB doesn't require aggregation by le.
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
# Aggregated histogram: Everything in one. Note how NHCB doesn't require aggregation by le.
# Aggregated histogram: Everything in one. Note how native histograms don't require aggregation by le.

This adds equivalent NHCB tests to the existing classic histogram
tests.

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

beorn7 commented Jul 16, 2024

Comments addressed, will merge on green.

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