Create ServiceMonitors to gather metrics from the OTEL Operands #1874

iblancasa · 2023-06-27T22:36:06Z

Resolves #1768

…Telemetry Collector instances Signed-off-by: Israel Blancas <[email protected]>

…or into feature/1768

Signed-off-by: Israel Blancas <[email protected]>

…or into feature/1768

Signed-off-by: Israel Blancas <[email protected]>

TylerHelmuth · 2023-06-28T14:51:23Z

pkg/collector/reconcile/servicemonitor.go

+
+func desiredServiceMonitors(_ context.Context, params Params) []monitoringv1.ServiceMonitor {
+	col := params.Instance
+	return []monitoringv1.ServiceMonitor{


I don't love the idea of the OpenTelemetry Operator being able to create objects that depend on other CRDs being preset, it feels like an opportunity for our users to get into weird situations.

I agree, I think this opens up some issues if we make the assumption that every operator user wants to use service monitors. This is a good topic for our next SIG meeting.

this also creates a direct dependency on the prometheus operator from ours which concerns me. Right now we have a dependency in the TA which makes sense as it is the component pulling down that functionality, but I worry what happens with this added dependency.

I don't love the idea of the OpenTelemetry Operator being able to create objects that depend on other CRDs being preset

From the conversations we had during the 25 May SIG meeting, I understood it was OK to implement the proposal described in #1768.

it feels like an opportunity for our users to get into weird situations.

What situations? I would be happy to check them and apply a fix if needed.

I agree, I think this opens up some issues if we make the assumption that every operator user wants to use service monitors.

I don't think this will be a problem since everything is protected with a feature flag. If the feature flag is not enabled, the Service Monitors will not be created. Also, you need to set to true the feature in the OpenTelemetry Collector instance.

This is a good topic for our next SIG meeting.

I'll not be able to attend since I'll be on vacation next week. But I'll be happy to talk about it in another moment.

We have discussed this on the SIG meeting that the operator will create Service/Pod monitors for operands with the assumption that the functionality will be put behind a feature flag. The implementation has to be flexible to allow OTLP in the future (once the collector will report OTLP metrics).

This work is needed to move the OTEL operator to operator level 4 https://operatorhub.io/operator/opentelemetry-operator. There is a lot of other operators that do the same.

ah okay, yeah sorry checked my notes and saw this now. Proceed :) will review now.

…rator into feature/1768

…or into feature/1768

pavolloffay · 2023-06-30T15:03:01Z

apis/v1alpha1/opentelemetrycollector_types.go

@@ -345,6 +352,26 @@ type AutoscalerSpec struct {
 	TargetMemoryUtilization *int32 `json:"targetMemoryUtilization,omitempty"`
 }

+// MetricsConfigSpec defines a metrics config.
+type MetricsConfigSpec struct {
+	// CreateServiceMonitors specifies if ServiceMonitors should be created for the OpenTelemetry Collector.


Document here, how this feature is enabled

jaronoff97

The approach here makes sense, could you also add an e2e test please?

main.go

jaronoff97 · 2023-06-30T19:20:35Z

main.go

@@ -71,6 +73,7 @@ func init() {

 	utilruntime.Must(otelv1alpha1.AddToScheme(scheme))
 	utilruntime.Must(routev1.AddToScheme(scheme))
+	utilruntime.Must(monitoringv1.AddToScheme(scheme))


Should we also put this behind the flag?

When this code is executed, the flag value is unknown.

…rator into feature/1768

…or into feature/1768

Signed-off-by: Israel Blancas <[email protected]>

iblancasa · 2023-07-11T14:11:07Z

I'm trying to fix the problems associated to the E2E tests

tests/e2e/prometheus-config-validation/03-promreceiver-nopromconfig.yaml

Signed-off-by: Israel Blancas <[email protected]>

…or into feature/1768

openshift-ci · 2023-07-12T14:55:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: iblancasa, jaronoff97

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-07-12T15:22:57Z

@iblancasa: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/opentelemetry-e2e-tests	`17e4bb5`	link	true	`/test opentelemetry-e2e-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…or into feature/1768

jaronoff97 · 2023-07-18T19:34:20Z

@TylerHelmuth mind giving one more review?

TylerHelmuth · 2023-07-18T19:47:29Z

@jaronoff97 I am still concerned about #1874 (comment). It is bad that users can't modify their metrics endpoint today, I am hesitant to add on more functionality dependent on the metrics endpoint without the flexibility to configure it. While I recognize we could release this first and add support for configuration later, it feels like an implementation that drives the Operator to Level 4 should also allow the users to configure the managed object (in this instance the Collector) however they need.

jaronoff97 · 2023-07-18T19:49:12Z

I see, in that case let's block merging this until we have #1931 closed

TylerHelmuth · 2023-07-18T19:48:59Z

pkg/featuregate/featuregate.go

+	PrometheusOperatorIsAvailable = featuregate.GlobalRegistry().MustRegister(
+		"operator.observability.prometheus",
+		featuregate.StageAlpha,
+		featuregate.WithRegisterDescription("enables features associated to the Prometheus Operator"))


Please add a featuregate.FromVersion that marks the Operator version this feature gate was added. (This was a rather late concept in the featuregate package and we should go back and fix our other gates to include it)

iblancasa · 2023-07-19T10:04:29Z

I see, in that case let's block merging this until we have #1931 closed

Even without merging this PR, the problem is still there because the service created by the operator will be pointing to a port that does nothing. I'll work on #1931 but, TBH, I don't think there is a real need to block this PR until #1931 is merged.

…or into feature/1768

Signed-off-by: Israel Blancas <[email protected]>

TylerHelmuth · 2023-07-19T14:43:01Z

Even without merging this PR, the problem is still there because the service created by the operator will be pointing to a port that does nothing

Correct. I my opinion it feels bad to add more complex logic to an inflexible configuration.

…or into feature/1768

.github/workflows/e2e.yaml

…rator into feature/1768

Signed-off-by: Israel Blancas <[email protected]>

…or into feature/1768

…-telemetry#1874) * Allow the creation of ServiceMonitors to gather metrics from the OpenTelemetry Collector instances Signed-off-by: Israel Blancas <[email protected]> * Add missing changelog Signed-off-by: Israel Blancas <[email protected]> * Fix unprotected statement Signed-off-by: Israel Blancas <[email protected]> * Fix lint issues Signed-off-by: Israel Blancas <[email protected]> * Apply changes requested in code review Signed-off-by: Israel Blancas <[email protected]> * Add missing generated files Signed-off-by: Israel Blancas <[email protected]> * Change the way to enable the feature flag Signed-off-by: Israel Blancas <[email protected]> * Change the way to enable the feature flag Signed-off-by: Israel Blancas <[email protected]> * Fix merge * Fix enable feature flag * Change the name of the option and move the E2E tests to their own folder * Fix unit test * Fix docs * Fix CRD field * Fix CRD field * Add from version to feature gate Signed-off-by: Israel Blancas <[email protected]> * Move the E2E tests to their own section for the CI Signed-off-by: Israel Blancas <[email protected]> --------- Signed-off-by: Israel Blancas <[email protected]>

iblancasa added 6 commits June 28, 2023 00:33

Allow the creation of ServiceMonitors to gather metrics from the Open…

621888a

…Telemetry Collector instances Signed-off-by: Israel Blancas <[email protected]>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

e5fb93f

…or into feature/1768

Add missing changelog

66d5660

Signed-off-by: Israel Blancas <[email protected]>

Fix unprotected statement

ab26178

Signed-off-by: Israel Blancas <[email protected]>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

796b323

…or into feature/1768

Fix lint issues

80e25f5

Signed-off-by: Israel Blancas <[email protected]>

iblancasa marked this pull request as ready for review June 28, 2023 14:37

iblancasa requested a review from a team June 28, 2023 14:37

TylerHelmuth reviewed Jun 28, 2023

View reviewed changes

iblancasa added 3 commits June 28, 2023 17:24

Merge branch 'main' into feature/1768

7880104

Merge branch 'feature/1768' of github.com:iblancasa/opentelemetry-ope…

638853f

…rator into feature/1768

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

4d5e3d7

…or into feature/1768

pavolloffay reviewed Jun 30, 2023

View reviewed changes

Merge branch 'main' into feature/1768

3c2bc7d

jaronoff97 approved these changes Jun 30, 2023

View reviewed changes

iblancasa added 4 commits July 10, 2023 20:23

Merge branch 'feature/1768' of github.com:iblancasa/opentelemetry-ope…

42c6c90

…rator into feature/1768

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

27e2a7e

…or into feature/1768

Apply changes requested in code review

4e718df

Signed-off-by: Israel Blancas <[email protected]>

Add missing generated files

90b070b

Signed-off-by: Israel Blancas <[email protected]>

jaronoff97 reviewed Jul 11, 2023

View reviewed changes

tests/e2e/prometheus-config-validation/03-promreceiver-nopromconfig.yaml Outdated Show resolved Hide resolved

iblancasa added 6 commits July 11, 2023 16:54

Change the way to enable the feature flag

a5953e7

Signed-off-by: Israel Blancas <[email protected]>

Change the way to enable the feature flag

5604f1f

Signed-off-by: Israel Blancas <[email protected]>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

3101866

…or into feature/1768

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

8fe848c

…or into feature/1768

Fix merge

7a66076

Fix enable feature flag

17e4bb5

Merge branch 'main' into feature/1768

4f79a2d

iblancasa added 6 commits July 17, 2023 12:13

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

f9fca96

…or into feature/1768

Change the name of the option and move the E2E tests to their own folder

f8af040

Fix unit test

3791100

Fix docs

bd81e57

Fix CRD field

cb1f6ba

Fix CRD field

a259991

iblancasa mentioned this pull request Jul 17, 2023

Expose the metrics port according to the OTEL Collector config #1931

Closed

TylerHelmuth reviewed Jul 18, 2023

View reviewed changes

jaronoff97 mentioned this pull request Jul 18, 2023

Update feature gates to include a version #1935

Closed

iblancasa added 2 commits July 19, 2023 12:16

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

25effa4

…or into feature/1768

Add from version to feature gate

175c0dc

Signed-off-by: Israel Blancas <[email protected]>

iblancasa added 2 commits July 21, 2023 17:09

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

61ee15a

…or into feature/1768

Merge branch 'main' into feature/1768

1ca9357

TylerHelmuth reviewed Jul 24, 2023

View reviewed changes

.github/workflows/e2e.yaml Outdated Show resolved Hide resolved

iblancasa added 3 commits July 24, 2023 20:48

Merge branch 'feature/1768' of github.com:iblancasa/opentelemetry-ope…

1dc416a

…rator into feature/1768

Move the E2E tests to their own section for the CI

297af31

Signed-off-by: Israel Blancas <[email protected]>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

83d31d5

…or into feature/1768

TylerHelmuth approved these changes Jul 25, 2023

View reviewed changes

jaronoff97 merged commit 42ff92f into open-telemetry:main Jul 25, 2023

iblancasa deleted the feature/1768 branch July 25, 2023 17:13

cmergenthaler mentioned this pull request Aug 14, 2023

Opentelemetry-operator service account permissions differ in latest release open-telemetry/opentelemetry-helm-charts#859

Closed

cmergenthaler mentioned this pull request Nov 6, 2023

[target allocator] Create ServiceMonitor to monitor target allocator #2311

Closed

This was referenced Dec 5, 2023

Create ServiceMonitor to monitor target allocator #2415

Closed

Create ServiceMonitor to monitor target allocator #2416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create ServiceMonitors to gather metrics from the OTEL Operands #1874

Create ServiceMonitors to gather metrics from the OTEL Operands #1874

iblancasa commented Jun 27, 2023

TylerHelmuth Jun 28, 2023

jaronoff97 Jun 28, 2023 •

edited

Loading

jaronoff97 Jun 28, 2023

iblancasa Jun 28, 2023

pavolloffay Jun 30, 2023

jaronoff97 Jun 30, 2023

pavolloffay Jun 30, 2023

jaronoff97 left a comment

jaronoff97 Jun 30, 2023

iblancasa Jul 10, 2023

iblancasa commented Jul 11, 2023

openshift-ci bot commented Jul 12, 2023

openshift-ci bot commented Jul 12, 2023

jaronoff97 commented Jul 18, 2023

TylerHelmuth commented Jul 18, 2023

jaronoff97 commented Jul 18, 2023

TylerHelmuth Jul 18, 2023

iblancasa commented Jul 19, 2023

TylerHelmuth commented Jul 19, 2023

Create ServiceMonitors to gather metrics from the OTEL Operands #1874

Create ServiceMonitors to gather metrics from the OTEL Operands #1874

Conversation

iblancasa commented Jun 27, 2023

Choose a reason for hiding this comment

jaronoff97 Jun 28, 2023 • edited Loading

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

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iblancasa commented Jul 11, 2023

openshift-ci bot commented Jul 12, 2023

openshift-ci bot commented Jul 12, 2023

jaronoff97 commented Jul 18, 2023

TylerHelmuth commented Jul 18, 2023

jaronoff97 commented Jul 18, 2023

Choose a reason for hiding this comment

iblancasa commented Jul 19, 2023

TylerHelmuth commented Jul 19, 2023

jaronoff97 Jun 28, 2023 •

edited

Loading