Skip to content

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!

@adberger adberger requested a review from povilasv as a code owner September 10, 2024 08:52
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 👍

@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