feat(metrics): add experimental otel metrics collector #2988

swiatekm-sumo · 2023-04-13T15:12:27Z

Experimental otel metrics collector to replace Prometheus. This is done by using the opentelemetry operator with the target allocator and Prometheus CR options enabled. This causes the target allocator to take the role of Prometheus Operator: tracking Service and Pod Monitors and translating that into raw Prometheus configuration. The target allocator also allocates the scrape targets to otel Pods, so this setup is scalable.

This collector replaces the metrics metadata StatefulSet - it's essentially the same pipeline, but with a different receiver

Intentionally missing in this PR:

Node metric aggregations via recording rules (we should add tests for these)
filtering, there's no remote writes and we export all collected metrics
partial histogram timeseries are not yet supported due to a difference in behaviour between telegrafreceiver and prometheusreceiver
autoscaling
various QoL options present for metadata

TODO:

more comments
some labels are missing
template tests

Checklist

Changelog updated or skip changelog label added
Template tests added for new features
Integration tests added or modified for major features

deploy/helm/sumologic/Chart.yaml

aboguszewski-sumo · 2023-04-26T13:02:26Z

deploy/helm/sumologic/conf/metrics/otelcol/pipeline.yaml

 receivers:
+{{- if not .Values.sumologic.metrics.collector.otelcol.enabled }}
  - telegraf
+{{- else }}
+  - prometheus
+{{- end }}


Are we going to enable users to add other receivers soon?

Do we have a use case for that? It's assumed right now that everything exposes Prometheus metrics either directly or via a sidecar. Either way, I wouldn't want to add that in this PR.

Yes, I know that it requires some more work for now, but I was just wondering what is going to be the future of this collector.

rnishtala-sumo · 2023-04-28T03:07:05Z

tests/integration/values/values_helm_ot_metrics.yaml

+  prometheus:
+    enabled: false
+  prometheusOperator:
+    enabled: false


Could both, the prometheus operator and the opentelemetry operator be enabled by the user?

Yes, they are. Unless I'm misunderstanding the question.

rnishtala-sumo · 2023-04-28T03:13:01Z

deploy/helm/sumologic/values.yaml

@@ -405,6 +405,20 @@ sumologic:
      ## Defines metrics metadata enrichment provider - `otelcol` or `fluentd`. `otelcol` is the default and is recommended. `fluentd` is deprecated.


Shouldn't we change this comment to say Prometheus is the default for metrics collection?

No, because it's not the default yet.

rnishtala-sumo · 2023-04-28T03:19:17Z

deploy/helm/sumologic/templates/metrics/collector/otelcol/opentelemetrycollector.yaml

@@ -0,0 +1,57 @@
+{{- if and (eq (include "metrics.otelcol.enabled" .) "true") .Values.sumologic.metrics.collector.otelcol.enabled }}
+apiVersion: opentelemetry.io/v1alpha1
+kind: OpenTelemetryCollector


opentelemetry CR! I guess we included the opentelemetry operator in the chart in an earlier PR

It's been included for a while actually, though it's disabled by default. It's the only way to do auto-instrumentation.

Could you add a warning informing user that Opentelemetry operator must be enabled to use otelcol metrics collector? Something similar to warnings in https://github.com/SumoLogic/sumologic-kubernetes-collection/blob/main/deploy/helm/sumologic/templates/NOTES.txt

I'd rather not add anything user facing here yet. This is clearly marked as experimental, and it's currently more for our internal testing than anything else. And there is a comment in values.yaml saying that opentelemetry-operator needs to be enabled.

rnishtala-sumo · 2023-04-28T03:31:41Z

deploy/helm/sumologic/templates/metrics/collector/otelcol/opentelemetrycollector.yaml

+        requests:
+          storage: {{ .Values.metadata.persistence.size }}
+{{- end }}
+  config: |


From the SIG meeting, I guess the way this configuration is specified could change...

Rather than change, there's probably going to be other ways to define it.

rnishtala-sumo · 2023-04-28T03:39:43Z

deploy/helm/sumologic/conf/metrics/collector/otelcol/config.yaml

+            #   regex: (id|name)
+          relabel_configs: *relabel_configs  # partially copied from what operator generates
+    target_allocator:
+      endpoint: http://{{ template "sumologic.metadata.name.metrics.targetallocator.name" . }}


so does the collector use this endpoint to discover the targets to be scraped?

Does this get converted to http_sd_configs by the operator under the hood...

This is actually a feature of prometheus receiver, and it doesn't use http_sd_configs as far as I know. The operator actually gives us the targetallocator service.

rnishtala-sumo · 2023-04-28T04:00:48Z

deploy/helm/sumologic/templates/metrics/collector/otelcol/opentelemetrycollector.yaml

+  mode: statefulset
+  replicas: 3
+  serviceAccount: {{ template "sumologic.metadata.name.metrics.collector.serviceaccount" . }}
+  targetAllocator:


Does the operator create a target allocator deployment for us?

rnishtala-sumo · 2023-04-28T04:08:30Z

deploy/helm/sumologic/templates/metrics/collector/otelcol/opentelemetrycollector.yaml

+  targetAllocator:
+    serviceAccount: {{ template "sumologic.metadata.name.metrics.targetallocator.serviceaccount" . }}
+    enabled: true
+    prometheusCR:


what does enabling the prometheus CR under the target allocator do?

It makes the target allocator watch for Service and Pod Monitors, and turn them into Prometheus scrape target configuration. Without this option it only acts on the discovery config defined explicitly in prometheus receiver config.

kkujawa-sumo · 2023-05-05T10:56:51Z

deploy/helm/sumologic/templates/metrics/collector/otelcol/opentelemetrycollector.yaml

+    {{- include "sumologic.labels.common" . | nindent 4 }}
+spec:
+  mode: statefulset
+  replicas: 3


maybe it is worthy to make this configurable, WDYT?

There's a lot of settings that should be made configurable before we move this to a user-facing beta. For this PR, I wanted to limit the complexity, since it's big and complex as-is. So I only exposed the configuration necessary to run integration tests successfully.

kkujawa-sumo · 2023-05-05T11:59:48Z

It can be done in another pull request but it will be worthy to add diagram and documentation to show data flow for metrics collection using otelcol metrics collector, without documentation a lot of people could have a problem with understanding this, even if it is experimental feature.

tests/helm/testdata/goldenfile/metrics_collector_otc/basic.output.yaml

andrzej-stencel · 2023-05-17T13:24:17Z

deploy/helm/sumologic/conf/metrics/collector/otelcol/config.yaml

+      endpoint: http://{{ template "sumologic.metadata.name.metrics.targetallocator.name" . }}
+      interval: 30s
+      collector_id: ${POD_NAME}
+extensions:


We should perhaps extract the extension definitions to another file, to prevent duplication. This can be done in another PR.

The extensions aren't the same, the new metrics collector is managed by the operator, and the operator injects the health check extension automatically, and takes care of opening the right port and adding the health checks in K8s. They're mostly the same though, so there's room to improve this.

andrzej-stencel

Awesome! 👏 Let's run this 🚀

github-actions bot added the integration-tests label Apr 13, 2023

github-actions bot approved these changes Apr 13, 2023

View reviewed changes

swiatekm-sumo changed the title ~~Feat/otel prometheus replacement~~ feat(metrics): add experimental otel metrics collector Apr 13, 2023

swiatekm-sumo force-pushed the feat/otel-prometheus-replacement branch from 5e9c4cf to 6c69ca2 Compare April 13, 2023 17:45

github-actions bot added the documentation documentation label Apr 13, 2023

swiatekm-sumo force-pushed the feat/otel-prometheus-replacement branch 5 times, most recently from 721b864 to 66798b4 Compare April 15, 2023 13:21

swiatekm-sumo force-pushed the feat/otel-prometheus-replacement branch 5 times, most recently from 60b9c23 to 0b835f2 Compare April 26, 2023 11:34

swiatekm-sumo marked this pull request as ready for review April 26, 2023 12:35

swiatekm-sumo requested a review from a team as a code owner April 26, 2023 12:35

swiatekm-sumo changed the base branch from main to chore/upgrade-operator-chart April 26, 2023 12:36

Base automatically changed from chore/upgrade-operator-chart to main April 26, 2023 12:52

aboguszewski-sumo reviewed Apr 26, 2023

View reviewed changes

swiatekm-sumo force-pushed the feat/otel-prometheus-replacement branch 5 times, most recently from 12ea9ba to b6c364a Compare April 27, 2023 15:16

rnishtala-sumo reviewed Apr 28, 2023

View reviewed changes

kkujawa-sumo reviewed May 5, 2023

View reviewed changes

swiatekm-sumo mentioned this pull request May 5, 2023

Add feature flag for target allocator config addition open-telemetry/opentelemetry-operator#1688

Merged

swiatekm-sumo force-pushed the feat/otel-prometheus-replacement branch 2 times, most recently from 71e56e2 to 042b761 Compare May 11, 2023 14:01

andrzej-stencel reviewed May 17, 2023

View reviewed changes

tests/helm/testdata/goldenfile/metrics_collector_otc/basic.output.yaml Show resolved Hide resolved

swiatekm-sumo requested review from andrzej-stencel, kkujawa-sumo and rnishtala-sumo May 17, 2023 13:12

feat(metrics): add experimental otel metrics collector

c16d098

andrzej-stencel reviewed May 17, 2023

View reviewed changes

andrzej-stencel approved these changes May 17, 2023

View reviewed changes

swiatekm-sumo force-pushed the feat/otel-prometheus-replacement branch from 042b761 to c66e812 Compare May 17, 2023 13:37

fix(metrics): exclude metrics collector from webhooks

c66e812

swiatekm-sumo enabled auto-merge (rebase) May 17, 2023 13:39

swiatekm-sumo merged commit a737107 into main May 17, 2023
57 checks passed

swiatekm-sumo deleted the feat/otel-prometheus-replacement branch May 17, 2023 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): add experimental otel metrics collector #2988

feat(metrics): add experimental otel metrics collector #2988

swiatekm-sumo commented Apr 13, 2023 •

edited

aboguszewski-sumo Apr 26, 2023

swiatekm-sumo Apr 26, 2023

aboguszewski-sumo Apr 26, 2023

rnishtala-sumo Apr 28, 2023

swiatekm-sumo Apr 28, 2023

rnishtala-sumo Apr 28, 2023

swiatekm-sumo Apr 28, 2023

rnishtala-sumo Apr 28, 2023

swiatekm-sumo Apr 28, 2023

kkujawa-sumo May 5, 2023

swiatekm-sumo May 5, 2023

rnishtala-sumo Apr 28, 2023

swiatekm-sumo May 5, 2023

rnishtala-sumo Apr 28, 2023 •

edited

swiatekm-sumo Apr 28, 2023

rnishtala-sumo Apr 28, 2023

swiatekm-sumo Apr 28, 2023

rnishtala-sumo Apr 28, 2023

swiatekm-sumo Apr 28, 2023

kkujawa-sumo May 5, 2023

swiatekm-sumo May 5, 2023

kkujawa-sumo commented May 5, 2023

andrzej-stencel May 17, 2023

swiatekm-sumo May 17, 2023 •

edited

andrzej-stencel left a comment

		@@ -405,6 +405,20 @@ sumologic:
		## Defines metrics metadata enrichment provider - `otelcol` or `fluentd`. `otelcol` is the default and is recommended. `fluentd` is deprecated.

feat(metrics): add experimental otel metrics collector #2988

feat(metrics): add experimental otel metrics collector #2988

Conversation

swiatekm-sumo commented Apr 13, 2023 • edited

Checklist

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

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

Choose a reason for hiding this comment

rnishtala-sumo Apr 28, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkujawa-sumo commented May 5, 2023

Choose a reason for hiding this comment

swiatekm-sumo May 17, 2023 • edited

Choose a reason for hiding this comment

andrzej-stencel left a comment

Choose a reason for hiding this comment

swiatekm-sumo commented Apr 13, 2023 •

edited

rnishtala-sumo Apr 28, 2023 •

edited

swiatekm-sumo May 17, 2023 •

edited