Add missing label servicemonitor #2573

yuriolisa · 2024-01-29T11:08:04Z

Description:

Add missing label for Service/Pod Monitors
Link to tracking Issue:
Resolves #2251
Testing:
Added unit and e2e tests.
Documentation:

swiatekm · 2024-01-29T11:37:18Z

Could we use manifestutils.Labels for these?

yuriolisa · 2024-01-29T12:05:24Z

Could we use manifestutils.Labels for these?

No, but I could include a PodMonitorSelector and ServiceMonitorSelector. WDYT?

Signed-off-by: Yuri Sa <[email protected]> Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]> Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]>

internal/manifests/collector/podmonitor.go

jaronoff97

@yuriolisa i left a comment on the original issue for how I think we should solve this. Let me know your thoughts.

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97

A few questions. Do we have an e2e test for this that checks that we successfully pulled metrics based on the SM/PM?

internal/manifests/manifestutils/labels.go

tests/e2e-prometheuscr/create-pm-prometheus-exporters/01-assert.yaml

tests/e2e-prometheuscr/create-sm-prometheus-exporters/01-assert.yaml

internal/manifests/manifestutils/labels.go

Signed-off-by: Yuri Sa <[email protected]>

internal/manifests/collector/podmonitor_test.go

internal/manifests/manifestutils/labels.go

jaronoff97

i meant to click request changes for that podmonitor issue

Signed-off-by: Yuri Sa <[email protected]>

yuriolisa · 2024-02-12T11:41:32Z

i meant to click request changes for that podmonitor issue

Added the comment and changed the PodMonitor matchLabels to the default one.

jaronoff97

one comment, otherwise i think this is looking good!

tests/e2e-prometheuscr/create-pm-prometheus-exporters/01-assert.yaml

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 · 2024-02-13T16:02:11Z

looks like some legit e2e and lint failures

internal/manifests/manifestutils/labels.go

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97

one tiny comment, but i don't think it really matters.

internal/manifests/collector/podmonitor.go

swiatekm

Can we add a e2e test similar to this one: https://github.com/open-telemetry/opentelemetry-operator/tree/main/tests/e2e-targetallocator/targetallocator-prometheuscr? I'd like to check that our generated PodMonitors and ServiceMonitors allow us to actually scrape collector metrics.

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 · 2024-03-05T16:31:55Z

internal/manifests/collector/servicemonitor_test.go

+		"app.kubernetes.io/instance":   fmt.Sprintf("%s.%s", params.OtelCol.Namespace, params.OtelCol.Name),
+		"app.kubernetes.io/managed-by": "opentelemetry-operator",
+		"app.kubernetes.io/part-of":    "opentelemetry",
+		"app.kubernetes.io/name":       naming.MonitoringService(params.OtelCol.Name),


@yuriolisa i realized that we actually need to update the monitoring service for the collector to have its own special label in the same way we do for headless and instead match on that.

this is because the name label for the service actually is just the name of the overall collector

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 · 2024-03-07T20:21:23Z

internal/manifests/collector/service.go

-	headlessLabel  = "operator.opentelemetry.io/collector-headless-service"
-	headlessExists = "Exists"
+	headlessLabel    = "operator.opentelemetry.io/collector-headless-service"
+	headlessExists   = "Exists"


i think we can consolidate here

jaronoff97 · 2024-03-07T20:21:31Z

internal/manifests/collector/service.go

@@ -44,7 +46,7 @@ func HeadlessService(params manifests.Params) (*corev1.Service, error) {
 	h.Name = naming.HeadlessService(params.OtelCol.Name)
 	h.Labels[headlessLabel] = headlessExists

-	// copy to avoid modifying params.OtelCol.Annotations
+	// copy to avoid modifying params.OtelCol.annotations


jaronoff97 · 2024-03-07T20:22:06Z

internal/manifests/collector/servicemonitor.go

@@ -44,7 +44,6 @@ func ServiceMonitor(params manifests.Params) (*monitoringv1.ServiceMonitor, erro
 	}
 	name := naming.ServiceMonitor(params.OtelCol.Name)
 	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
-	selectorMatchLabels := manifestutils.SelectorMatchLabels(params.OtelCol.ObjectMeta, ComponentOpenTelemetryCollector)


we should still be using this to match on the overall labels we expect the service to have, otherwise our match would be too wide

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97

Just one last thing to improve test confidence, but the overall logic here looks 👌

tests/e2e-prometheuscr/create-sm-prometheus-exporters/06-assert.yaml

Signed-off-by: Yuri Sa <[email protected]>

* Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]> Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]> Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]> * Added manifests.utils to set labels Signed-off-by: Yuri Sa <[email protected]> * Fixed labels names Signed-off-by: Yuri Sa <[email protected]> * Fixed labels for PodMonitor Signed-off-by: Yuri Sa <[email protected]> * Fixed e2e test Signed-off-by: Yuri Sa <[email protected]> * Fixed e2e test Signed-off-by: Yuri Sa <[email protected]> * Fixed selectorMatchLabels Signed-off-by: Yuri Sa <[email protected]> * Removed the extra method Signed-off-by: Yuri Sa <[email protected]> * Adjusted nil Signed-off-by: Yuri Sa <[email protected]> * Changed labels Signed-off-by: Yuri Sa <[email protected]> * Changed labels Signed-off-by: Yuri Sa <[email protected]> * Readded common labels Signed-off-by: Yuri Sa <[email protected]> * Readded common labels Signed-off-by: Yuri Sa <[email protected]> * Added labels assert Signed-off-by: Yuri Sa <[email protected]> --------- Signed-off-by: Yuri Sa <[email protected]>

yuriolisa requested a review from a team January 29, 2024 11:08

Add missing label for Service/Pod Monitors

ea1e00a

Signed-off-by: Yuri Sa <[email protected]> Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]> Add missing label for Service/Pod Monitors Signed-off-by: Yuri Sa <[email protected]>

yuriolisa force-pushed the add-missing-label-servicemonitor branch from d88ee20 to ea1e00a Compare January 29, 2024 12:31

jaronoff97 reviewed Jan 29, 2024

View reviewed changes

internal/manifests/collector/podmonitor.go Outdated Show resolved Hide resolved

jaronoff97 requested changes Jan 29, 2024

View reviewed changes

Added manifests.utils to set labels

3f6fd0b

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 reviewed Jan 30, 2024

View reviewed changes

Fixed labels names

32bf984

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 approved these changes Jan 31, 2024

View reviewed changes

internal/manifests/collector/podmonitor_test.go Outdated Show resolved Hide resolved

internal/manifests/manifestutils/labels.go Outdated Show resolved Hide resolved

jaronoff97 requested changes Jan 31, 2024

View reviewed changes

Fixed labels for PodMonitor

b456ba6

Signed-off-by: Yuri Sa <[email protected]>

yuriolisa requested a review from jaronoff97 February 12, 2024 11:42

jaronoff97 reviewed Feb 12, 2024

View reviewed changes

tests/e2e-prometheuscr/create-pm-prometheus-exporters/01-assert.yaml Outdated Show resolved Hide resolved

yuriolisa added 2 commits February 12, 2024 21:08

Fixed e2e test

fc03579

Signed-off-by: Yuri Sa <[email protected]>

Fixed e2e test

00d2a04

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 reviewed Feb 13, 2024

View reviewed changes

internal/manifests/manifestutils/labels.go Outdated Show resolved Hide resolved

yuriolisa added 2 commits February 13, 2024 17:55

Fixed selectorMatchLabels

2ee13bb

Signed-off-by: Yuri Sa <[email protected]>

Removed the extra method

81ae4ab

Signed-off-by: Yuri Sa <[email protected]>

yuriolisa requested a review from jaronoff97 February 29, 2024 15:32

jaronoff97 approved these changes Feb 29, 2024

View reviewed changes

internal/manifests/collector/podmonitor.go Outdated Show resolved Hide resolved

swiatekm reviewed Feb 29, 2024

View reviewed changes

iblancasa approved these changes Feb 29, 2024

View reviewed changes

yuriolisa added 2 commits March 1, 2024 13:52

Adjusted nil

a8a518a

Signed-off-by: Yuri Sa <[email protected]>

Merge branch 'main' into add-missing-label-servicemonitor

525eb94

jaronoff97 reviewed Mar 5, 2024

View reviewed changes

Changed labels

fd39eb9

Signed-off-by: Yuri Sa <[email protected]>

Changed labels

50a375d

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 reviewed Mar 7, 2024

View reviewed changes

yuriolisa added 2 commits March 7, 2024 22:31

Readded common labels

956b94e

Signed-off-by: Yuri Sa <[email protected]>

Readded common labels

d41662b

Signed-off-by: Yuri Sa <[email protected]>

pavolloffay approved these changes Mar 8, 2024

View reviewed changes

jaronoff97 reviewed Mar 8, 2024

View reviewed changes

tests/e2e-prometheuscr/create-sm-prometheus-exporters/06-assert.yaml Show resolved Hide resolved

Added labels assert

38464fb

Signed-off-by: Yuri Sa <[email protected]>

jaronoff97 approved these changes Mar 11, 2024

View reviewed changes

pavolloffay merged commit 75fb162 into open-telemetry:main Mar 11, 2024
31 checks passed

yuriolisa mentioned this pull request Mar 11, 2024

Create e2e test to validate if PodMonitors and ServiceMonitors allow scraping collector metrics #2749

Open

yuriolisa mentioned this pull request Apr 18, 2024

Fix labels for Service Monitors #2878

Merged

pavolloffay mentioned this pull request May 17, 2024

Collector service monitor does not scape prometheus exporter #2965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing label servicemonitor #2573

Add missing label servicemonitor #2573

yuriolisa commented Jan 29, 2024

swiatekm commented Jan 29, 2024

yuriolisa commented Jan 29, 2024

jaronoff97 left a comment

jaronoff97 left a comment

jaronoff97 left a comment

yuriolisa commented Feb 12, 2024 •

edited

Loading

jaronoff97 left a comment

jaronoff97 commented Feb 13, 2024

jaronoff97 left a comment

swiatekm left a comment

jaronoff97 Mar 5, 2024

jaronoff97 Mar 5, 2024

jaronoff97 Mar 7, 2024

jaronoff97 Mar 7, 2024

jaronoff97 Mar 7, 2024

jaronoff97 left a comment

Add missing label servicemonitor #2573

Add missing label servicemonitor #2573

Conversation

yuriolisa commented Jan 29, 2024

swiatekm commented Jan 29, 2024

yuriolisa commented Jan 29, 2024

jaronoff97 left a comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

yuriolisa commented Feb 12, 2024 • edited Loading

jaronoff97 left a comment

Choose a reason for hiding this comment

jaronoff97 commented Feb 13, 2024

jaronoff97 left a comment

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

jaronoff97 Mar 5, 2024

Choose a reason for hiding this comment

jaronoff97 Mar 5, 2024

Choose a reason for hiding this comment

jaronoff97 Mar 7, 2024

Choose a reason for hiding this comment

jaronoff97 Mar 7, 2024

Choose a reason for hiding this comment

jaronoff97 Mar 7, 2024

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

yuriolisa commented Feb 12, 2024 •

edited

Loading