Instrument obsreport.receiver metrics with otel go #6222

paivagustavo · 2022-10-03T14:10:18Z

Description: Instruments existing obsreport.Receiver metrics with otel-go, side-by-side with the existing opencensus metrics.

This uses the same UseOtelForInternalMetricsfeatureGateID feature gate, adding a UseOtelForMetrics() method on the obsreportconfig that should be used by components of the collector core to check what sdk should be used for instrumentation.

Link to tracking Issue: part of #816

Testing: Manual tests comparing the current exported metrics and the metrics from the otel-go sdk after enabling the feature gate --feature-gates=telemetry.useOtelForInternalMetrics

codecov · 2022-10-03T14:17:51Z

Codecov Report

Base: 92.48% // Head: 92.04% // Decreases project coverage by -0.44% ⚠️

Coverage data is based on head (a5320de) compared to base (aa78209).
Patch coverage: 49.61% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6222      +/-   ##
==========================================
- Coverage   92.48%   92.04%   -0.45%     
==========================================
  Files         218      218              
  Lines       13124    13218      +94     
==========================================
+ Hits        12138    12166      +28     
- Misses        769      832      +63     
- Partials      217      220       +3

Impacted Files	Coverage Δ
obsreport/obsreport.go	`100.00% <ø> (ø)`
obsreport/obsreport_receiver.go	`63.58% <37.00%> (-36.42%)`	⬇️
internal/obsreportconfig/obsreportconfig.go	`96.90% <90.00%> (-3.10%)`	⬇️
service/telemetry.go	`89.02% <100.00%> (-0.58%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

bogdandrutu

We need this #5939, which is to not break the metrics names at this time. We will at one point later revisit all names.

internal/obsreportconfig/obsmetrics/obs_receiver.go

obsreport/obsreport_receiver.go

obsreport/views.go

jaronoff97

Just one thought, otherwise looks great!

obsreport/obsreport_receiver.go

paivagustavo · 2022-10-03T17:50:27Z

We need this #5939, which is to not break the metrics names at this time. We will at one point later revisit all names.

@bogdandrutu this pr actually fixes the name inconsistencies by wrapping the prometheus.Registry, this might be easier than trying add otelcol_ prefix to all metrics (including contrib metrics):

prometheus.WrapRegistererWithPrefix("otelcol_", prometheus.WrapRegistererWith(promLabels, registry))

If we still wants to move on with #5939, I can create a copy of obsmetrics.ReceiverPrefix, so this won't make otelcol_otelcol_ metrics and fix this after that get merged.

internal/obsreportconfig/obsmetrics/obsmetrics.go

obsreport/obsreport.go

obsreport/obsreport_receiver.go

service/telemetry.go

…zation changes to another pr; remove public InstrumentWithOtel from obsreport;

jpkrohling

This looks sane to me, a review from @Aneurysm9 would be helpful though.

component/telemetry.go

bogdandrutu

Current PR exposes new temporary public APIs because the OC bridge is not ready and we cannot really start the proper migration to otel.

I am not OK doing that.

paivagustavo · 2022-10-04T20:29:35Z

@bogdandrutu are you concerned about having this public TelemetrySettings.UseOtelForMetrics? Would it be better if we get rid of that and just started to record values to both sdks at the same time? Something like this:

rec.recordWithOtel(receiverCtx, dataType, numAccepted, numRefused)
rec.recordWithOC(receiverCtx, dataType, numAccepted, numRefused)

I'm trying to understand what other paths we might have other than just waiting for the bridge to get ready.

bogdandrutu · 2022-10-04T22:17:07Z

@paivagustavo to give you alternatives, I need to understand the goal :)

# Conflicts: # CHANGELOG.md # service/telemetry.go

codeboten

Just a couple of questions

internal/obsreportconfig/obsreportconfig.go

obsreport/obsreport_receiver.go

service/telemetry.go

internal/obsreportconfig/obsreportconfig.go

obsreport/obsreport_receiver.go

paivagustavo · 2022-10-06T03:54:47Z

Thanks for the reviews @bogdandrutu @codeboten @Aneurysm9, I think I've addressed all the comments at this point and it should be ready to be reviewed again.

codeboten

Thanks for addressing my comments 👍🏻

internal/obsreportconfig/obsreportconfig.go

obsreport/obsreport_receiver.go

# Conflicts: # CHANGELOG.md

internal/obsreportconfig/obsreportconfig.go

bogdandrutu · 2022-10-11T19:55:59Z

obsreport/obsreport_receiver.go

+	handleError := func(metricName string, err error) {
+		if err != nil {
+			rec.logger.Warn("failed to create otel instrument", zap.Error(err), zap.String("metric", metricName))
+		}


We need to return the error to the caller instead of logging. It should not be done in this PR, but we need to file an issue for this.

# Conflicts: # CHANGELOG.md

# Conflicts: # cmd/otelcorecol/go.mod

bogdandrutu

Just for my sanity, please create a separate PR for the version updates, maybe also include the 1.11 updates as well.

* upgrade otel go to latest version v1.11.0/v0.32.3/v0.36.3 (#6293) * [chore] move service pipeline id validation to main config Validate (#6281) Signed-off-by: Bogdan <bogdandrutu@gmail.com> Signed-off-by: Bogdan <bogdandrutu@gmail.com> * Instrument obsreport.receiver metrics with otel go (#6222) * Instrument obs_receiver with otel go * add changelog entry * remove views configuration * move otel-go metrics to obsreport * add UseOtelForMetrics to TelemetrySettings; extract otel sdk initialization changes to another pr; remove public InstrumentWithOtel from obsreport; * default UseOtelForMetrics to false * remove duplicate oc registry initialization * move `UseOtelForMetrics` to the internal package `obsreportconfig` * address comments * only register view if featuregate is disabled * prefix meter name with `receiver/` * change meter scope name * upgrade otel metric sdk * run make gotidy * update changelog * revert: otel go version upgrade * revert: x/sys upgrades * [chore] add chloggen for changelog (#6062) This change updates the changelog process in this repo to match the process in the contrib repo. * Fix support for new line in config URI location Signed-off-by: Bogdan <bogdandrutu@gmail.com> Signed-off-by: Bogdan <bogdandrutu@gmail.com> Co-authored-by: Gustavo Paiva <gustavo@lightstep.com> Co-authored-by: Alex Boten <aboten@lightstep.com>

paivagustavo requested review from a team and codeboten October 3, 2022 14:10

paivagustavo force-pushed the gustavo/otel-go-metrics branch from b3eea74 to ce32d57 Compare October 3, 2022 14:15

paivagustavo mentioned this pull request Oct 3, 2022

Migrate collector's metrics from OC to OpenTelemetry #816

Closed

bogdandrutu reviewed Oct 3, 2022

View reviewed changes

jaronoff97 approved these changes Oct 3, 2022

View reviewed changes

obsreport/obsreport_receiver.go Show resolved Hide resolved

bogdandrutu reviewed Oct 3, 2022

View reviewed changes

internal/obsreportconfig/obsmetrics/obsmetrics.go Outdated Show resolved Hide resolved

obsreport/obsreport.go Outdated Show resolved Hide resolved

obsreport/obsreport_receiver.go Show resolved Hide resolved

service/telemetry.go Outdated Show resolved Hide resolved

paivagustavo added 7 commits October 4, 2022 13:15

Instrument obs_receiver with otel go

017e5e6

add changelog entry

e39ca63

remove views configuration

740089e

move otel-go metrics to obsreport

5e3e5d9

add UseOtelForMetrics to TelemetrySettings; extract otel sdk initiali…

4b36a7f

…zation changes to another pr; remove public InstrumentWithOtel from obsreport;

default UseOtelForMetrics to false

c6049b2

remove duplicate oc registry initialization

32f6ad5

paivagustavo force-pushed the gustavo/otel-go-metrics branch from 3bf3ebd to 32f6ad5 Compare October 4, 2022 16:16

jpkrohling reviewed Oct 4, 2022

View reviewed changes

bogdandrutu reviewed Oct 4, 2022

View reviewed changes

component/telemetry.go Outdated Show resolved Hide resolved

bogdandrutu requested changes Oct 4, 2022

View reviewed changes

paivagustavo added 2 commits October 5, 2022 17:45

move UseOtelForMetrics to the internal package obsreportconfig

48b6778

Merge branch 'main' into gustavo/otel-go-metrics

1d1ac78

# Conflicts: # CHANGELOG.md # service/telemetry.go

codeboten reviewed Oct 5, 2022

View reviewed changes

internal/obsreportconfig/obsreportconfig.go Outdated Show resolved Hide resolved

obsreport/obsreport_receiver.go Outdated Show resolved Hide resolved

obsreport/obsreport_receiver.go Show resolved Hide resolved

Aneurysm9 reviewed Oct 6, 2022

View reviewed changes

service/telemetry.go Outdated Show resolved Hide resolved

internal/obsreportconfig/obsreportconfig.go Outdated Show resolved Hide resolved

obsreport/obsreport_receiver.go Show resolved Hide resolved

obsreport/obsreport_receiver.go Outdated Show resolved Hide resolved

address comments

d77d44f

codeboten approved these changes Oct 6, 2022

View reviewed changes

Aneurysm9 approved these changes Oct 7, 2022

View reviewed changes

evantorrie reviewed Oct 8, 2022

View reviewed changes

internal/obsreportconfig/obsreportconfig.go Outdated Show resolved Hide resolved

bogdandrutu reviewed Oct 10, 2022

View reviewed changes

obsreport/obsreport_receiver.go Outdated Show resolved Hide resolved

paivagustavo added 3 commits October 10, 2022 14:44

Merge remote-tracking branch 'origin/main' into gustavo/otel-go-metrics

80cb6f7

# Conflicts: # CHANGELOG.md

only register view if featuregate is disabled

d4195fd

prefix meter name with receiver/

03a30b1

paivagustavo commented Oct 10, 2022

View reviewed changes

internal/obsreportconfig/obsreportconfig.go Show resolved Hide resolved

change meter scope name

da46725

bogdandrutu approved these changes Oct 11, 2022

View reviewed changes

paivagustavo added 5 commits October 11, 2022 18:21

Merge remote-tracking branch 'origin/main' into gustavo/otel-go-metrics

3116f25

# Conflicts: # CHANGELOG.md

upgrade otel metric sdk

77f4323

run make gotidy

f058096

Merge remote-tracking branch 'origin/main' into gustavo/otel-go-metrics

5dd757f

# Conflicts: # cmd/otelcorecol/go.mod

update changelog

97481a5

bogdandrutu approved these changes Oct 12, 2022

View reviewed changes

paivagustavo added 2 commits October 12, 2022 19:04

revert: otel go version upgrade

7ce8fc9

revert: x/sys upgrades

a5320de

paivagustavo mentioned this pull request Oct 12, 2022

upgrade otel go to latest version v1.11.0/v0.32.3/v0.36.3 #6293

Merged

bogdandrutu merged commit a9f41a2 into open-telemetry:main Oct 12, 2022

This was referenced Oct 18, 2022

Instrument exporters lightstep/opentelemetry-collector#14

Merged

Instrument exporters' obsreport with OpenTelemetry #6346

Merged

moh-osman3 mentioned this pull request Oct 19, 2022

Instrument obsreport.Scraper lightstep/opentelemetry-collector#15

Closed

This was referenced Oct 31, 2022

Instrument obsreport.Scraper lightstep/opentelemetry-collector#19

Merged

Instrument obsreport.Scraper #6460

Merged

Instrument obsreport.Processor lightstep/opentelemetry-collector#22

Closed

This was referenced Nov 21, 2022

Instrument obsreport.Processor lightstep/opentelemetry-collector#36

Merged

Instrument obsreport.Processor #6607

Merged

moh-osman3 mentioned this pull request Dec 12, 2022

Instrument proctelemetry.ProcessMetrics lightstep/opentelemetry-collector#67

Merged

moh-osman3 mentioned this pull request Jan 4, 2023

Instrument proctelemetry.ProcessMetrics #6886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument obsreport.receiver metrics with otel go #6222

Instrument obsreport.receiver metrics with otel go #6222

paivagustavo commented Oct 3, 2022 •

edited

Loading

codecov bot commented Oct 3, 2022 •

edited

Loading

bogdandrutu left a comment

jaronoff97 left a comment

paivagustavo commented Oct 3, 2022 •

edited

Loading

jpkrohling left a comment

bogdandrutu left a comment

paivagustavo commented Oct 4, 2022

bogdandrutu commented Oct 4, 2022

codeboten left a comment

paivagustavo commented Oct 6, 2022

codeboten left a comment

bogdandrutu Oct 11, 2022

bogdandrutu left a comment

Instrument obsreport.receiver metrics with otel go #6222

Instrument obsreport.receiver metrics with otel go #6222

Conversation

paivagustavo commented Oct 3, 2022 • edited Loading

codecov bot commented Oct 3, 2022 • edited Loading

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

paivagustavo commented Oct 3, 2022 • edited Loading

jpkrohling left a comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

paivagustavo commented Oct 4, 2022

bogdandrutu commented Oct 4, 2022

codeboten left a comment

Choose a reason for hiding this comment

paivagustavo commented Oct 6, 2022

codeboten left a comment

Choose a reason for hiding this comment

bogdandrutu Oct 11, 2022

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

paivagustavo commented Oct 3, 2022 •

edited

Loading

codecov bot commented Oct 3, 2022 •

edited

Loading

paivagustavo commented Oct 3, 2022 •

edited

Loading