Ruler: validation and logging with disabled tenant federation #4555
Conversation
| @@ -55,11 +57,12 @@ func TenantFederationQueryFunc(regularQueryable, federatedQueryable rules.QueryF | |||
| } | |||
| } | |||
|
|
|||
| func RemoveFederatedRuleGroups(groups map[string]rulespb.RuleGroupList) { | |||
| func removeFederatedRuleGroups(groups map[string]rulespb.RuleGroupList, logger log.Logger) { | |||
There was a problem hiding this comment.
This wasn't used outside the package. I also checked github code search and couldn't find any usages of it in upstream projects (including GEM)
There was a problem hiding this comment.
LGTM! I just left a nit. Also good question from @charleskorn.
This PR bundles two changes because they are related and small * reject creating federated rule groups when ruler tenant federation is disabled. Currently, the rule groups would be silently ignored during bucket syncs. This made the UX around creating federated rules confusing because there was no feedback as to why the groups aren't evaluating * log an info line on each sync when a federated rule group is skipped because the feature is disabled Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
force-pushed
the
dimiatr/ruler-federated-groups
branch
from
a93b3b6
to
bea61fa
Compare
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pkg/ruler/tenant_federation.go
Outdated
| for userID, groupList := range groups { | ||
| amended := make(rulespb.RuleGroupList, 0, len(groupList)) | ||
| for _, group := range groupList { | ||
| if len(group.GetSourceTenants()) > 0 { | ||
| level.Warn(logger).Log("msg", "skipping federated rule group because rule federation is disabled", "namespace", group.Namespace, "group_name", group.Name, "user", userID) |
There was a problem hiding this comment.
👌
This has the potential to be very very noisy, perhaps we should consider using rate limited logger? Also if it feels more natural to me to indicate back to the operator here that we should include To enable the feature, configure the Mimir or GEM ruler instance with the following parameter.... you had earlier on.
There was a problem hiding this comment.
I agree that having the config options in the log line is more appropriate.
I'm not sure about the rate limiter. It will also mute the warning for some groups. Let's see how this works in grafana cloud, maybe we can add it afterwards.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pkg/ruler/api_test.go
Outdated
| labels: | ||
| test: test | ||
| `, | ||
| err: errors.New("invalid rules config: rule group 'test' is a federated rule group, but rules federation is disabled; please contact your service administrator to have it enabled"), |
There was a problem hiding this comment.
| err: errors.New("invalid rules config: rule group 'test' is a federated rule group, but rules federation is disabled; please contact your service administrator to have it enabled"), | |
| err: errors.New("invalid rules configuration: rule group 'test' is a federated rule group, but rules federation is disabled; contact your service administrator to have it enabled."), |
Are you talking about a configuration or a config file. (I assume the former.)
There was a problem hiding this comment.
(Not sure if the operator or end user will see this text.)
There was a problem hiding this comment.
Are you talking about a configuration or a config file. (I assume the former.)
It's more a configuration - you're right. I'll also change the validations that include this string.
(Not sure if the operator or end user will see this text.)
this will be seen by end users (although this string in particular is part of our tests, but it matches the string that the user will see)
There was a problem hiding this comment.
also - it's a go convention to not put punctuation at the end of errors, so I'll only replace config with configuration
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
What this PR does
This PR bundles two changes because they are related and small
disabled. Currently, the rule groups would be silently ignored during
bucket syncs. This made the UX around creating federated rules confusing
because there was no feedback as to why the groups aren't evaluating
because the feature is disabled
Technically the first is a breaking change, but I think no one would
depend on this behaviour.
Signed-off-by: Dimitar Dimitrov dimitar.dimitrov@grafana.com
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]