Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add target histogram rule #91

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add target histogram rule #91

wants to merge 2 commits into from

Conversation

rgeyer
Copy link
Collaborator

@rgeyer rgeyer commented Jul 20, 2022

Resolves #56.

Fairly simple, and literal check that finds any metric/series containing _bucket and ensures that it is inside of a function call to histogram_quantile.

There was some internal slack discussion about forthcoming features which may need to be accounted for in the future, but we will wait until those are finalized before addressing them in the rules.

@rgeyer rgeyer marked this pull request as ready for review July 21, 2022 15:32
@rgeyer rgeyer requested review from mshahzeb and beorn7 July 21, 2022 15:34
Copy link

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

As discussed on Slack, it's perfectly reasonable to access a ..._bucket metric outside of a histogram_quantile argument. It's more the other way around: The histogram_quantile function can only reasonably act on ..._bucket time series (with only very obscure exceptions).

}

err = parser.Walk(inspector(func(node parser.Node, parents []parser.Node) error {
// We're looking for either a VectorSelector. This skips any other node type.
Copy link

Choose a reason for hiding this comment

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

Spurious "either"?

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.

Check every use of a histogram (_bucket) is inside a histgram_quantile(rate(...))
2 participants