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

api/rules: Add filtering on rule name/group/file #7560

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

jacobbaungard
Copy link
Contributor

@jacobbaungard jacobbaungard commented Jul 19, 2024

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Added the possibility of filtering rules by rule_name, rule_group or file. This brings the rules api closer in-line with Prometheus.

Verification

  • Added unit tests

This commits adds the option of filtering rules by rule name, rule
group, or file. This brings the rule API closer in-line with the current
Prometheus api.

Signed-off-by: Jacob Baungard Hansen <jacobbaungard@redhat.com>
@jacobbaungard jacobbaungard marked this pull request as ready for review July 19, 2024 14:31
Copy link
Member

@saswatamcode saswatamcode 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 bringing it up to speed, some minor comments

}

// filterRulesbyMatchers filters rules in a group according to given matcherSets.
func filterRulesByMatchers(ruleGroups []*rulespb.RuleGroup, matcherSets [][]*labels.Matcher) []*rulespb.RuleGroup {
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to update this function now to better mimic prometheus/prometheus#10194?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, most likely. We also need exclude_alerts to be fully aligned with the Prometheus API. Could we deal with those in separate PRs perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

Sure

Comment on lines +99 to +101
rnSet := queryFormToSet(ruleName)
rgSet := queryFormToSet(ruleGroup)
fSet := queryFormToSet(file)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think just using slices.Contains instead of set might be more memory intensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I did it like this, was simply because that's how the code looks like in Prometheus. I am not sure if there are significant performance benefits either way, although perhaps the code is a bit cleaner using slices.Contains, so happy to change to that approach if you think it's best?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, fine if it mirrors prometheus then

@saswatamcode saswatamcode merged commit fb20d8c into thanos-io:main Jul 23, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants