Skip to content

Ruler: Add support for per-user external labels #6340

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

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

damnever
Copy link
Contributor

@damnever damnever commented Nov 15, 2024

What this PR does:

Add support for per-user external labels, this currently applies only to alerting rules, but we may consider adding them to remote read in the future.

Which issue(s) this PR fixes:

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@dosubot dosubot bot added the component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. label Nov 15, 2024
@damnever damnever force-pushed the feat/external-labels branch 2 times, most recently from 8551250 to 24be605 Compare November 15, 2024 10:07
@damnever damnever changed the title Ruler: add support for per-user external labels Ruler: Add support for per-user external labels Nov 15, 2024
@alanprot
Copy link
Member

@rapphil @rajagopalanand Maybe u guys can take a look on this one?

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I think the change makes sense.
We still need some tests

r.deleteRuleCache(user)
if err != nil {
r.lastReloadSuccessful.WithLabelValues(user).Set(0)
level.Error(r.logger).Log("msg", "unable to update rule manager", "user", user, "err", err)
return
}
if externalLabelsUpdated {
if err = r.notifierApplyExternalLabels(user, externalLabels); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a test case to verify the notifier applies external labels successfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discovered alertmanagers will be reset after applying the config: prometheus/prometheus#14987

@damnever damnever force-pushed the feat/external-labels branch 2 times, most recently from 9d6bb66 to 8c543ae Compare November 18, 2024 10:17
@damnever
Copy link
Contributor Author

not sure why the tests failed..

Copy link
Contributor

@rajagopalanand rajagopalanand left a comment

Choose a reason for hiding this comment

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

Will continue to review today

@yeya24
Copy link
Contributor

yeya24 commented Nov 21, 2024

Saw the same test failure in https://github.com/cortexproject/cortex/actions/runs/11944198449/job/33295615418?pr=6353 so probably unrelated. I will retry the test

@yeya24
Copy link
Contributor

yeya24 commented Nov 22, 2024

We can try rebasing master to get #6357

@damnever damnever force-pushed the feat/external-labels branch from c518968 to 7b5b187 Compare November 25, 2024 02:37
Copy link
Contributor

@yeya24 yeya24 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 for the contribution!

Copy link
Contributor

@rajagopalanand rajagopalanand left a comment

Choose a reason for hiding this comment

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

LGTM. Just added one nit for idiomatic change

@yeya24 yeya24 merged commit bc1b5be into cortexproject:master Dec 3, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants