Skip to content

Conversation

JoaoBraveCoding
Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding commented Feb 28, 2025

In #578 we completely removed the filtering code and relied only on the query parameter for Loki to filter the rules. However, the filtering only works for the Loki API and not for the Prometheus API

Fixes https://issues.redhat.com/browse/LOG-6148

@JoaoBraveCoding JoaoBraveCoding changed the title logs: re-introduce rules filtering for Prometheus API as this is still logs: re-introduce rules filtering for Prometheus API as this is still not supported by Loki Feb 28, 2025
@JoaoBraveCoding JoaoBraveCoding marked this pull request as ready for review February 28, 2025 18:11
Copy link
Contributor

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Seems to work fine in tests 👍

I have added some comments, but none of them would be a show-stopper.

ErrorType string `json:"errorType"`
}

func newModifyResponseProm(logger log.Logger, labelKeys map[string][]string) func(*http.Response) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the error code-paths are not covered by the current set of tests. Should we add one or more tests to cover some of them? Most of them seem to be simple cases, so it's more a question of "completeness" and if it's worth the effort (not talking about all the paths where it's an IO operation or marshalling).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for alerts let me know if you were also referring to anything else

@xperimental xperimental merged commit ebbaf26 into observatorium:main May 28, 2025
5 checks passed
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