Add extra metric relabelings to scrape classes #6492

sathieu · 2024-04-09T16:06:07Z

Description

See #5947 (comment)

Inspired by #6379.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Add extra metric relabelings to scrape classes

simonpasquier

The main issue for me is that the scrape class metrics relabeling happens after the scrape object's metrics relabeling. Other scrape class fields behave differently and can be overridden by scrape objects. I suppose that you have a specific requirement in mind but the inconsistency brings a UX problem.

sathieu · 2024-04-10T06:33:13Z

@simonpasquier Yes. This is described in the doc, but if this is not enough we can use two configurations: metricRelabelingsBefore and metricRelabelingsAfter (I'm not good at naming)., or metricRelabelings and metricRelabelingsOverride, ....

simonpasquier · 2024-04-10T08:49:07Z

Can you share more details about why you need scrape class relabeling to happen after service/pod monitor relabelings rather than before?

sathieu · 2024-04-10T10:11:39Z

Yes.

As described in the issue, we have multi-tenant clusters, and each tenant is defined by a list of namespaces or a list of namespace regexps. We want the metrics relabeling to happen after the namespace label enforcement, to avoid a tenant injecting metrics into another tenant.

If scrape class metric relabelings happen before scrape resource metric relabelings, a malicious actor can easily change the tenant label.

simonpasquier · 2024-04-11T08:55:14Z

As described #5947 (comment), we have multi-tenant clusters, and each tenant is defined by a list of namespaces or a list of namespace regexps. We want the metrics relabeling to happen after the namespace label enforcement, to avoid a tenant injecting metrics into another tenant.

To be clear: you're already setting .spec.enforcedNamespaceLabel: "namespace"?
If yes, how do you deal with recording/alerting rules? Do you expect to enforce the tenant label there too?

sathieu · 2024-04-11T09:11:32Z

To be clear: you're already setting .spec.enforcedNamespaceLabel: "namespace"?

Yes.

If yes, how do you deal with recording/alerting rules? Do you expect to enforce the tenant label there too?

I forgot about those, I also need the tenant label for recording/alerting rules. Thanks 🙏 .

Maybe a new .spec.enforceMetricRelabelings in the prometheus CRD would be more appropriate (please note that I don't want excludedFromEnforcement to be applied here).

simonpasquier · 2024-04-11T11:06:50Z

I forgot about those, I also need the tenant label for recording/alerting rules.

🤔 I don't see an obvious solution for rules since relabeling can't be applied here...

Maybe a new .spec.enforceMetricRelabelings in the prometheus CRD would be more appropriate (please note that I don't want excludedFromEnforcement to be applied here).

I suppose that the latter is because you deploy generic rules generating namespace-aware metrics/alerts and you still want the metrics/alerts to have the right tenant?

There might still be other use cases that need to skip "enforced" metric relabelings for some monitor/rule resources though.

simonpasquier · 2024-04-11T11:19:56Z

Nevertheless this PR (with scrape class' relabeling happening before monitor's metric relabeling) is worth the effort even if it's not addressing @sathieu requirements.

sathieu · 2024-04-11T19:49:10Z

I've updated the PR to :

	// The Operator adds the scrape class metric relabelings defined here.
	// Then the Operator adds the target-specific metric relabelings defined in the scrape object.
	// Then the Operator adds namespace enforcement relabeling rule.

I'll handle my usecase in another PR.

ArthurSens

Hi @sathieu, sorry for taking so long to give you an answer here, I saw the failing tests and assumed the PR was not ready. I took a deeper look today and I think the failing tests are flaky ones. I've re-triggered them to be sure... let's see

pkg/apis/monitoring/v1/prometheus_types.go

sathieu · 2024-05-08T21:07:51Z

Thanks @ArthurSens I've added your suggestion. I also had to rebase to resolve conflicts. Back to you!

pkg/apis/monitoring/v1/prometheus_types.go

ArthurSens

Awesome, thanks for tackling this!

I've made a few nitpick comments, but if you want to focus on something I'd say improving test readability is what could receive some love right now.

Approving the PR because this seems to work as expected

ArthurSens · 2024-05-09T16:07:36Z

pkg/apis/monitoring/v1/prometheus_types.go

+	// MetricRelabelings configures the relabeling rules to apply to all samples before ingestion.
+	//
+	// The Operator adds the scrape class metric relabelings defined here.
+	// Then the Operator adds the target-specific metric relabelings defined in the scrape object.


Suggested change

// Then the Operator adds the target-specific metric relabelings defined in the scrape object.

// Then the Operator adds the target-specific metric relabelings defined in ServiceMonitors, PodMonitors, Probes and ScrapeConfigs.

ArthurSens · 2024-05-09T16:08:28Z

pkg/apis/monitoring/v1/prometheus_types.go

+	//
+	// The Operator adds the scrape class metric relabelings defined here.
+	// Then the Operator adds the target-specific metric relabelings defined in the scrape object.
+	// Then the Operator adds namespace enforcement relabeling rule.


Suggested change

// Then the Operator adds namespace enforcement relabeling rule.

// Then the Operator adds namespace enforcement relabeling rule, specified in '.spec.enforcedNamespaceLabel'.

ArthurSens · 2024-05-09T16:12:02Z

pkg/prometheus/promcfg_test.go

@@ -8783,3 +8783,205 @@ func TestPodMonitorWithNonDefaultScrapeClassRelabelings(t *testing.T) {
 	require.NoError(t, err)
 	golden.Assert(t, string(cfg), "podMonitorObjectWithNonDefaultScrapeClassWithRelabelings.golden")
 }
+
+func TestServiceMonitorWithDefaultScrapeClassMetricRelabelings(t *testing.T) {


Awesome tests!

We could improve code readability here if we reduce code duplication using table tests

ArthurSens · 2024-05-09T16:19:40Z

pkg/prometheus/promcfg_test.go

+	prometheus.Spec.ScrapeClasses = scrapeClasses
+	cg := mustNewConfigGenerator(t, prometheus)
+
+	cfg, err := cg.GenerateServerConfiguration(
+		context.Background(),
+		prometheus.Spec.EvaluationInterval,
+		prometheus.Spec.QueryLogFile,
+		prometheus.Spec.RuleSelector,
+		prometheus.Spec.Exemplars,
+		prometheus.Spec.TSDB,
+		prometheus.Spec.Alerting,
+		prometheus.Spec.RemoteRead,
+		map[string]*monitoringv1.ServiceMonitor{"monitor": serviceMonitor},
+		nil,
+		nil,
+		nil,
+		&assets.StoreBuilder{},
+		nil,
+		nil,
+		nil,
+		nil,
+	)
+	require.NoError(t, err)
+	golden.Assert(t, string(cfg), "serviceMonitorObjectWithDefaultScrapeClassWithMetricRelabelings.golden")


What I mean with code duplication is that this part is basically identical in all tests. We could have a single function with different inputs instead

testCases := []struct{ scrapeClass monitoringv1.ScrapeClass podMonitors map[string]monitoringv1.Podmonitor // can be nil serviceMonitors map[string]monitoringv1.ServiceMonitor // can be nil probes map[string]monitoringv1.Probe // can be nil scrapeConfigs map[string]monitoringv1alpha1.ScrapeConfig // can be nil goldenFile string }

You can put the implementation of the test inside a loop and ensure that your input results are equal to the appropriate golden file

sathieu · 2024-05-22T20:24:59Z

@nicolastakashi I've addressed your comments, rebased (there was a conflict because of 642d9cd) and force-pushed. Back to you 🏀 !

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

simonpasquier

Thanks! I'll let @ArthurSens and/or @nicolastakashi merge if they are happy.

ArthurSens

Perfect, thanks @sathieu !

sathieu requested a review from a team as a code owner April 9, 2024 16:06

pull-request-size bot added the size/M label Apr 9, 2024

sathieu marked this pull request as draft April 9, 2024 16:06

sathieu mentioned this pull request Apr 9, 2024

Support additional relabelings in scrape classes #5947

Open

sathieu force-pushed the scrapeclass-metric-relabeling branch from b10ca78 to 4e0d8f9 Compare April 9, 2024 16:13

simonpasquier reviewed Apr 9, 2024

View reviewed changes

sathieu force-pushed the scrapeclass-metric-relabeling branch from 4e0d8f9 to bb983f6 Compare April 9, 2024 19:55

pull-request-size bot added size/L and removed size/M labels Apr 9, 2024

sathieu force-pushed the scrapeclass-metric-relabeling branch from bb983f6 to 622d7c7 Compare April 9, 2024 20:47

sathieu marked this pull request as ready for review April 9, 2024 20:56

sathieu force-pushed the scrapeclass-metric-relabeling branch from 622d7c7 to 01933cb Compare April 10, 2024 06:29

sathieu force-pushed the scrapeclass-metric-relabeling branch from 01933cb to 8ce6306 Compare April 11, 2024 19:47

ArthurSens reviewed May 7, 2024

View reviewed changes

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

sathieu force-pushed the scrapeclass-metric-relabeling branch from 8ce6306 to a914e2d Compare May 8, 2024 21:06

sathieu force-pushed the scrapeclass-metric-relabeling branch from a914e2d to 850cfc2 Compare May 8, 2024 21:17

nicolastakashi reviewed May 9, 2024

View reviewed changes

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

sathieu force-pushed the scrapeclass-metric-relabeling branch from 850cfc2 to fff1d0d Compare May 9, 2024 09:25

ArthurSens previously approved these changes May 9, 2024

View reviewed changes

ArthurSens reviewed May 9, 2024

View reviewed changes

nicolastakashi previously approved these changes May 9, 2024

View reviewed changes

sathieu dismissed stale reviews from nicolastakashi and ArthurSens via ddc3b9e May 22, 2024 20:22

sathieu force-pushed the scrapeclass-metric-relabeling branch from fff1d0d to ddc3b9e Compare May 22, 2024 20:22

Add extra metric relabelings to scrape classes

555fa80

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

sathieu force-pushed the scrapeclass-metric-relabeling branch from ddc3b9e to 555fa80 Compare May 22, 2024 20:27

simonpasquier approved these changes May 23, 2024

View reviewed changes

ArthurSens approved these changes May 23, 2024

View reviewed changes

ArthurSens merged commit 37043cc into prometheus-operator:main May 23, 2024
17 checks passed

ArthurSens mentioned this pull request May 23, 2024

bugfix: Fix bug created from race conditions during merge #6613

Merged

1 task

nicolastakashi mentioned this pull request May 23, 2024

[FIX] typo on function naming #6614

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extra metric relabelings to scrape classes #6492

Add extra metric relabelings to scrape classes #6492

sathieu commented Apr 9, 2024 •

edited

simonpasquier left a comment

sathieu commented Apr 10, 2024

simonpasquier commented Apr 10, 2024

sathieu commented Apr 10, 2024

simonpasquier commented Apr 11, 2024

sathieu commented Apr 11, 2024 •

edited

simonpasquier commented Apr 11, 2024

simonpasquier commented Apr 11, 2024

sathieu commented Apr 11, 2024

ArthurSens left a comment

sathieu commented May 8, 2024

ArthurSens left a comment

ArthurSens May 9, 2024

ArthurSens May 9, 2024

ArthurSens May 9, 2024

ArthurSens May 9, 2024 •

edited

sathieu commented May 22, 2024

simonpasquier left a comment

ArthurSens left a comment

	// Then the Operator adds the target-specific metric relabelings defined in the scrape object.
	// Then the Operator adds the target-specific metric relabelings defined in ServiceMonitors, PodMonitors, Probes and ScrapeConfigs.

	// Then the Operator adds namespace enforcement relabeling rule.
	// Then the Operator adds namespace enforcement relabeling rule, specified in '.spec.enforcedNamespaceLabel'.

Add extra metric relabelings to scrape classes #6492

Add extra metric relabelings to scrape classes #6492

Conversation

sathieu commented Apr 9, 2024 • edited

Description

Type of change

Verification

Changelog entry

simonpasquier left a comment

Choose a reason for hiding this comment

sathieu commented Apr 10, 2024

simonpasquier commented Apr 10, 2024

sathieu commented Apr 10, 2024

simonpasquier commented Apr 11, 2024

sathieu commented Apr 11, 2024 • edited

simonpasquier commented Apr 11, 2024

simonpasquier commented Apr 11, 2024

sathieu commented Apr 11, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

sathieu commented May 8, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens May 9, 2024

Choose a reason for hiding this comment

ArthurSens May 9, 2024

Choose a reason for hiding this comment

ArthurSens May 9, 2024

Choose a reason for hiding this comment

ArthurSens May 9, 2024 • edited

Choose a reason for hiding this comment

sathieu commented May 22, 2024

simonpasquier left a comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

sathieu commented Apr 9, 2024 •

edited

sathieu commented Apr 11, 2024 •

edited

ArthurSens May 9, 2024 •

edited