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

feat: Add join label functionality to alerts #961

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

adberger
Copy link
Contributor

@adberger adberger commented Jul 22, 2024

This PR intends to get the work of #792 merged.
Fixes #791

@adberger
Copy link
Contributor Author

Anyone could take a look at this?

@michel-numan
Copy link

Much needed update!

lib/utils.libsonnet Outdated Show resolved Hide resolved
Copy link
Collaborator

@skl skl left a comment

Choose a reason for hiding this comment

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

lgtm for merging @povilasv

There's no change to the alert rules by default here, users need to explicity modify the xxx_join_labels config in order to enable this feature. PromQL looks good. So seems safe enough.

The only potential downside I can see, in a multi-cluster environemnt, for users that want to take advantage of this feature, the relevant KSM label metrics should be scraped for all clusters, otherwise (take pods for example) pods in clusters with missing kube_pod_labels will be dropped from the alerts because the join * on (...) kube_pod_labels will be empty for those clusters that do not scrape that metric for the affected pods.

This means if you have 3 clusters, and only one is configured to scrape kube_pod_labels, you'll never see KubePodCrashLooping alert for the other 2 clusters (they'll be silently dropped from the alert query).

Kinda convoluted scenario but maybe worth an additional comment in config.libsonnet @adberger.

@skl
Copy link
Collaborator

skl commented Sep 30, 2024

@adberger looks like a whitespace issue caused the lint check to fail

@adberger
Copy link
Contributor Author

adberger commented Oct 1, 2024

@adberger looks like a whitespace issue caused the lint check to fail

Will fix

@adberger
Copy link
Contributor Author

adberger commented Oct 1, 2024

@skl I've made the 2 changes, could you take another look?
Also do you prefer rebasing into 1 commit?

@skl
Copy link
Collaborator

skl commented Oct 1, 2024

Yeah, feel free to rebase/squash @adberger, the PRs on this repo seem to be normal merge commits so a single commit makes the history a little easier to follow 👍

Signed-off-by: Adrian Berger <adrian.berger@bedag.ch>
@povilasv povilasv merged commit cb72d73 into kubernetes-monitoring:master Oct 4, 2024
9 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.

Add support for joining labels into alerts from kube_XXX_labels metrics
4 participants