[Mixins] Option for a custom cluster label #1651

Whyeasy · 2022-04-07T10:48:47Z

What this PR does

This PR makes it possible to provide another label that indicates your cluster. The default is set to cluster and it shouldn't break any of the current rules or dashboards.

Checklist

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

Signed-off-by: Whyeasy <[email protected]>

CLAassistant · 2022-04-07T10:48:54Z

All committers have signed the CLA.

pracucci

Thanks for working on this! I left a couple of high-level comments on the direction where I think we should go to, instead of adding another config option which could add extra burden to maintain over time. Thanks!

pracucci · 2022-04-07T15:23:36Z

operations/mimir-mixin/recording_rules.libsonnet

@@ -479,7 +479,7 @@ local utils = import 'mixin-utils/utils.libsonnet';
 rules: [
 {
 // cortex_ingester_ingested_samples_total is per user, in this rule we want to see the sum per cluster/namespace/instance
- record: 'cluster_namespace_%s:cortex_ingester_ingested_samples_total:rate1m' % $._config.per_instance_label,
+ record: '%s_namespace_%s:cortex_ingester_ingested_samples_total:rate1m' % [$._config.clusterLabel, $._config.per_instance_label],


The actual aggregation is dynamic and based on alert_aggregation_labels. It's only the recording rule name that reference cluster_namespace hardcoded. Instead of using $._config.clusterLabel here, we should build the recording rule prefix based on alert_aggregation_labels. See this related issue: #1632

pracucci · 2022-04-07T15:26:56Z

operations/mimir-mixin/config.libsonnet

@@ -35,9 +35,12 @@
 overrides_exporter: 'overrides-exporter',
 },

+ // Custom Cluster label to use in dashboards.
+ clusterLabel: 'cluster',


I don't think we should introduce this, but we should just keep using job_labels and cluster_labels everywhere. For example, cluster_labels is already used to build alert_aggregation_labels which is then used in alerts and recording rules. However, not all of them are using it so I think we should just keep working to make sure they're used everywhere (see few other comments I left).

Will start working on using alert_aggregation_labels.

But what's your idea on the approach we should take here:

mimir/operations/mimir-mixin/dashboards/dashboard-utils.libsonnet

Line 57 in ca1b6d6

.addMultiTemplate('cluster', 'cortex_build_info', '%s' % $._config.clusterLabel)

. Because this selector should be updated accordingly.

I see the difficulty of getting rid of this config field at all. Ok, let's keep it. Can I just ask you to rename clusterLabel to per_cluster_label to keep it consistent with per_instance_label and per_node_label, please?

The comment could be something like:

// The label used to differentiate between different Kubernetes clusters.

Signed-off-by: Whyeasy <[email protected]>

Whyeasy · 2022-04-08T13:05:52Z

Added alert_aggregation_rule_prefix at the places where it was previously cluster_namespace. Same goes for the group by cases.

What would you preferred direction be for cases like these:

mimir/operations/mimir-mixin/dashboards/dashboard-utils.libsonnet

Line 57 in ca1b6d6

 .addMultiTemplate('cluster', 'cortex_build_info', '%s' % $._config.clusterLabel) 

mimir/operations/mimir-mixin/dashboards/alertmanager.libsonnet

Line 34 in 7b32d0a

sum(%s_job:cortex_alertmanager_alerts_invalid_total:rate5m{%s})

mimir/operations/mimir-mixin/dashboards/scaling.libsonnet

Line 44 in 7b32d0a

  %(alert_aggregation_rule_prefix)s_deployment_reason:required_replicas:count{%(clusterLabel)s=~"$cluster", namespace=~"$namespace"} 

https://github.com/Whyeasy/mimir/blob/7b32d0a0d97e44c3ca828005c3de4cd443b5167e/operations/mimir-mixin/recording_rules.libsonnet#L15

pracucci

Good job!

Could you update the doc at docs/sources/operators-guide/visualizing-metrics/requirements.md? It currently says the cluster label can't be customized.

pracucci · 2022-04-12T07:56:52Z

operations/mimir-mixin/dashboards/scaling.libsonnet

@@ -41,11 +41,11 @@ local utils = import 'mixin-utils/utils.libsonnet';
 $.tablePanel([
 |||
 sort_desc(
- cluster_namespace_deployment_reason:required_replicas:count{cluster=~"$cluster", namespace=~"$namespace"}
+ %(alert_aggregation_rule_prefix)s_deployment_reason:required_replicas:count{%(clusterLabel)s=~"$cluster", namespace=~"$namespace"}


I would use namespaceMatcher() utility instead of {%(clusterLabel)s=~"$cluster", namespace=~"$namespace"}.

Same comment applies below.

pracucci · 2022-04-12T08:04:23Z

operations/mimir-mixin/config.libsonnet

@@ -35,9 +35,12 @@
 overrides_exporter: 'overrides-exporter',
 },

+ // Custom Cluster label to use in dashboards.
+ clusterLabel: 'cluster',


I see the difficulty of getting rid of this config field at all. Ok, let's keep it. Can I just ask you to rename clusterLabel to per_cluster_label to keep it consistent with per_instance_label and per_node_label, please?

The comment could be something like:

// The label used to differentiate between different Kubernetes clusters.

pracucci · 2022-04-12T08:06:11Z

CHANGELOG.md

+* [ENHANCEMENT] Alerting rules: Add possibility to use a custom cluster label.
+* [ENHANCEMENT] Dashboards: Add possibility to use a custom cluster label.
+* [ENHANCEMENT] Recording rules: Add possibility to use a custom cluster label.


Suggested change

* [ENHANCEMENT] Alerting rules: Add possibility to use a custom cluster label.

* [ENHANCEMENT] Dashboards: Add possibility to use a custom cluster label.

* [ENHANCEMENT] Recording rules: Add possibility to use a custom cluster label.

* [ENHANCEMENT] Added `per_cluster_label` support to allow to change the label name used to differentiate between Kubernetes clusters. #1651

pracucci · 2022-04-12T08:09:40Z

Could you also run make check-mixin? We expect no diff in the compiled output (there are just few differences to fix).

Signed-off-by: Whyeasy <[email protected]>

CHANGELOG.md

pracucci

LGTM, thanks!

Whyeasy added 6 commits April 6, 2022 13:38

Add custom clusterlabel to dashboards and recording rules

10b2095

Signed-off-by: Whyeasy <[email protected]>

Use correct group for grouping metrics

d6d4066

Signed-off-by: Whyeasy <[email protected]>

Revert groups by labels

058c906

Signed-off-by: Whyeasy <[email protected]>

Add cluster label in alerts for recording rules

af70959

Signed-off-by: Whyeasy <[email protected]>

Update CHANGELOG.md

e9f6b09

Signed-off-by: Whyeasy <[email protected]>

Fix indentation config.libsonnet

ca1b6d6

Signed-off-by: Whyeasy <[email protected]>

pracucci reviewed Apr 7, 2022

View reviewed changes

Use alert_aggreation at the obvious locations

7b32d0a

Signed-off-by: Whyeasy <[email protected]>

pracucci reviewed Apr 12, 2022

View reviewed changes

Whyeasy added 2 commits April 13, 2022 10:23

rename clusterLabel and add namespaceMatcher

a420133

Signed-off-by: Whyeasy <[email protected]>

Merge branch 'main' into custom-cluster-label

b1637d2

pracucci reviewed Apr 13, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

d516f1b

pracucci approved these changes Apr 13, 2022

View reviewed changes

pracucci merged commit 40c6408 into grafana:main Apr 13, 2022

pracucci mentioned this pull request Apr 13, 2022

Test and fix custom labels support in mixin #1697

Merged

3 tasks

wilfriedroset mentioned this pull request Apr 27, 2022

Provide an example Prometheus config compatible with Mimir dashboards and alerts #1642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mixins] Option for a custom cluster label #1651

[Mixins] Option for a custom cluster label #1651

Whyeasy commented Apr 7, 2022 •

edited

Loading

CLAassistant commented Apr 7, 2022 •

edited

Loading

pracucci left a comment

pracucci Apr 7, 2022

pracucci Apr 7, 2022

Whyeasy Apr 8, 2022 •

edited

Loading

pracucci Apr 12, 2022

Whyeasy commented Apr 8, 2022

pracucci left a comment

pracucci Apr 12, 2022

Whyeasy Apr 13, 2022

pracucci Apr 12, 2022

pracucci Apr 12, 2022

Whyeasy Apr 13, 2022

pracucci commented Apr 12, 2022

pracucci left a comment

[Mixins] Option for a custom cluster label #1651

[Mixins] Option for a custom cluster label #1651

Conversation

Whyeasy commented Apr 7, 2022 • edited Loading

What this PR does

Checklist

CLAassistant commented Apr 7, 2022 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Whyeasy Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Whyeasy commented Apr 8, 2022

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Apr 12, 2022

pracucci left a comment

Choose a reason for hiding this comment

Whyeasy commented Apr 7, 2022 •

edited

Loading

CLAassistant commented Apr 7, 2022 •

edited

Loading

Whyeasy Apr 8, 2022 •

edited

Loading