Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to disable metrics coming from OTC #994

Merged
merged 7 commits into from Oct 22, 2020

Conversation

mat-rumian
Copy link
Contributor

@mat-rumian mat-rumian commented Oct 8, 2020

###### Description

New variable otelcol.metrics_enabled gives possibility to disable or enable (false/true) metrics coming from OpenTelemetry Collector. Changes are touching otc-service configuration (ports), otc-deployment (additional argument in command), values.yaml (default value).

Expected log when metrics are enabled:
{"level":"info","ts":1602188423.6246598,"caller":"service/service.go:256","msg":"Setting up own telemetry..."} {"level":"info","ts":1602188423.6251273,"caller":"service/telemetry.go:100","msg":"Serving Prometheus metrics","address":":8888","legacy_metrics":false,"new_metrics":true,"level":3,"service.instance.id":"c8020a8f-9ccb-4d98-8619-b0ddf1f24331"} {"level":"info","ts":1602188423.6261094,"caller":"service/service.go:293","msg":"Loading configuration..."}

Expected log when metrics are disabled:
{"level":"info","ts":1602188357.0610797,"caller":"service/service.go:256","msg":"Setting up own telemetry..."} {"level":"info","ts":1602188357.0619454,"caller":"service/service.go:293","msg":"Loading configuration..."} {"level":"info","ts":1602188357.0664148,"caller":"service/service.go:304","msg":"Applying configuration..."}

###### Testing performed

Tested on test cluster.

@sumo-drosiek
Copy link
Contributor

I would rather put it as otelcol.metrics_enabled than sumologic.traces.otc_metrics.enabled, because its about otelcol not sumologic per se

@sumo-drosiek sumo-drosiek added this to the v2.0 milestone Oct 9, 2020
Copy link
Contributor

@pmm-sumo pmm-sumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on Dominik's suggestion. Also, we need to update the list of params with this attribute. Otherwise, it's a nice and clean PR 👍

Copy link
Contributor

@pmm-sumo pmm-sumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I am only wondering if we should enable or disable it by default... hmm. @sumo-drosiek and @perk - what's your take?

@@ -57,6 +57,9 @@ spec:
command:
- "/otelcontribcol"
- "--config=/conf/traces.otelcol.conf.yaml"
{{- if eq .Values.otelcol.metrics_enabled false }}
- "--metrics-level=none"
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the other metrics levels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find anything about levels description but levels itself are described here Looks like to get information what's under which level you have to go thru the code.

@sumo-drosiek
Copy link
Contributor

@pmaciolek, @mat-rumian How big is expected metrics load from the opentelemetry collector?

@pmm-sumo
Copy link
Contributor

@pmaciolek, @mat-rumian How big is expected metrics load from the opentelemetry collector?

Not sure on DPM, but depending on the setup I think that there are typically ~50 metrics (including histograms)

@mat-rumian mat-rumian merged commit a5c698f into master Oct 22, 2020
@mat-rumian mat-rumian deleted the mat-rumian-otc-metrics-switch branch October 22, 2020 13:07
@perk-sumo
Copy link
Contributor

There is a follow-up PR: #1037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants