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

Added source_tenants for use with Grafana Mimir multitenancy #1044

Closed
wants to merge 1 commit into from

Conversation

balous
Copy link

@balous balous commented Mar 24, 2025

Grafana Mimir implements multitenant monitoring where each rule group's source_tenants attribute specifies names of tenants used as metric source:

https://grafana.com/docs/mimir/latest/references/architecture/components/ruler/#federated-rule-groups

This PR adds this attribute to generated alerts. Does this make sense from maintainer's point of view?

@balous balous requested review from povilasv and skl as code owners March 24, 2025 14:54
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.

Generally - this repo should remain vendor-agnostic. That being said, the ability to optionally add vendor-specific properties to the rules file may be useful as long as it's entirely optional and disabled by default.

With the PR as it is at the moment, even setting sourceTenants: null will render the same into the alerts YAML file output, meaning that every user would see this property whether they want it or not. It also means that any new rules file will need to remember to include the source_tenants which is prone to error.

So, a few asks from me, to make this more likely to be considered:

  1. Can you make it completely optional, so that the rules file does not contain any reference to source_tenants at all by default?
  2. Rather than adding source_tenants everywhere manually, do you think there's a way to centralise that code into something less obtrusive, like maybe adding the new behaviour to the lib/ directory? Something that acts on all rules during rendering, to reduce human error. This might also solve the first point above.
  3. Taking this a step further, what if another vendor needs to add a different property to all alert groups in the same way? e.g. instead of only source_tenants, could this be generalised to imaginary_property: ['foo', 'bar']? I guess you might need additional config properties for this.

Those are the suggestions that come to mind after reviewing. Effectively, can you make it so that custom properties are configurable? Rather than specifically source_tenants only.

@balous balous marked this pull request as draft March 25, 2025 10:22
@balous balous closed this Apr 7, 2025
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