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

Conserve labels in query result, add selector support #8

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

Conversation

bobertrublik
Copy link

I've been using your mixin for quite some time but ran into an issue when I implemented label based alerting. The certificate alert queries currently strip away all labels except for the ones select in the by clause.

Then I had a closer look at the queries and in my opinion you don't need the aggregations for CertManagerCertExpirySoon and CertManagerCertNotReady. The certificates are already unique through the namespace + name combination and it's not possible to group them, therefore I removed the aggregations.

For CertManagerHittingRateLimits I looked at the metric and from what I see sum without (method, path) should be sufficient for aggregation.

Last I would find it very useful to set a selector variable similar how it's done in other mixins, examples:

I refactored certManagerJobLabel to certManagerSelector and placed it in all alerts. I also updated the tests.

@bobertrublik
Copy link
Author

However it might make sense to add sum without (pod, instance, container, endpoint) to all alerts to prevent unforseen behavior with pod restarts or cert-manager running in HA? The labels in the without clause could also be made configurable in case somebody runs into a regression.

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.

1 participant