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

adding support to filter /api/v1/rules response #5749

Merged
merged 14 commits into from Feb 9, 2024
Merged

adding support to filter /api/v1/rules response #5749

merged 14 commits into from Feb 9, 2024

Conversation

victoramsantos
Copy link
Contributor

@victoramsantos victoramsantos commented Feb 1, 2024

Is your feature request related to a problem? Please describe

By the Prometheus doc we have the possibility to filter the rules by alert or record rules (using the type param). On the vm-alert doc it's just saying that this api list of all loaded groups and rules . When I run with the type it returns the same info for both param value.

Describe the solution you'd like

I would like that this API have the same behavior as in Prometheus, if I pass the parameter ?type=alert it would return only the alerts, if I pass ?type=record only the record rules and if it's without type both data.

Describe alternatives you've considered

No response

Additional information

  • I know it's possible to iterate over the response and filter each rule however I trying to make a cardinality analyzer that works seamless between Prometheus and VM
  • I didn't change the vmalert/api/v1/rules endpoint, since I don't know if it's desired to it be equal to api/v1/rules. Feedbacks here please 😊

Related feature-request: #5743

@Haleygo
Copy link
Collaborator

Haleygo commented Feb 5, 2024

Hey @victoramsantos , thanks for the PR!

I didn't change the vmalert/api/v1/rules endpoint, since I don't know if it's desired to it be equal to api/v1/rules.

Yes, they should behave the same.
And since we're adding this, I think below apis could share the same extra filter parameter:

  1. /vmalert/groups
  2. /rules
  3. /vmalert/api/v1/rules, /api/v1/rules

And I think we can add this filter parameter in

func (rh *requestHandler) groups() []apiGroup {

@victoramsantos
Copy link
Contributor Author

Hi @Haleygo job done!
Please any concern or feedback are very welcome 😊

app/vmalert/web.go Outdated Show resolved Hide resolved
@victoramsantos
Copy link
Contributor Author

victoramsantos commented Feb 7, 2024

Yes, I like it :)
I just changed the if ruleType != "" because for cases where the type is wrong I think we should ignore it and return everything. Do you agree? It's the same behavior I found for Prometheus' API.

@Haleygo
Copy link
Collaborator

Haleygo commented Feb 7, 2024

Looks good to me!
Could you help add a feature description to the changelog?

@victoramsantos
Copy link
Contributor Author

@Haleygo I think I did it right 😅

Copy link
Collaborator

@Haleygo Haleygo left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks!

docs/CHANGELOG.md Outdated Show resolved Hide resolved
victoramsantos and others added 2 commits February 8, 2024 13:35
Co-authored-by: Hui Wang <haley@victoriametrics.com>
@victoramsantos
Copy link
Contributor Author

Nice! @Haleygo I need to do something else or just wait for the merge?

Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hagen1778 hagen1778 merged commit 62e5e2a into VictoriaMetrics:master Feb 9, 2024
5 of 8 checks passed
@hagen1778
Copy link
Collaborator

Please also see #5787

hagen1778 pushed a commit that referenced this pull request Feb 9, 2024
…e by rule type (#5749)

Co-authored-by: Hui Wang <haley@victoriametrics.com>
(cherry picked from commit 62e5e2a)
@valyala
Copy link
Collaborator

valyala commented Feb 14, 2024

FYI, this pull request has been included in vmalert v1.98.0.

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.

None yet

4 participants