update config dependency by codeboten · Pull Request #11611 · open-telemetry/opentelemetry-collector

codeboten · 2024-11-05T21:12:08Z

This PR does a couple of things that I couldn't quite split up so I put together a PR w/ individual commits to help reviewers get through it. This PR does the following:

update go.opentelemetry.io/contrib/config package to latest. this brings in breaking changes. in order to prevent those breaking changes from impacting end users, i've also added a layer of config unmarshaling
updates the collector to instantiate the meter provider (and exporters) via the config package. this allows us to remove all the code in otelinit. the reason for including this change was that unmarshaling the config was causing circular dependencies i didn't want to address by moving code that could be deleted around.

Replacement for #11458.

Fixes #12021

mjnowen · 2024-11-12T09:27:31Z

Looking forward to seeing this get merged!

github-actions · 2024-11-27T03:22:40Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mjnowen · 2024-11-27T09:09:08Z

Not stale? @codeboten

codecov · 2024-12-09T19:13:31Z

Codecov Report

Attention: Patch coverage is 82.05805% with 68 lines in your changes missing coverage. Please review.

Project coverage is 91.62%. Comparing base (2447a81) to head (f2cddc3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
service/service.go	72.38%	34 Missing and 3 partials ⚠️
service/telemetry/internal/migration/v0.3.0.go	54.54%	12 Missing and 3 partials ⚠️
service/telemetry/internal/migration/v0.2.0.go	93.67%	8 Missing and 2 partials ⚠️
component/componenttest/configtest.go	0.00%	2 Missing and 1 partial ⚠️
service/telemetry/metrics.go	60.00%	1 Missing and 1 partial ⚠️
service/telemetry/config.go	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11611      +/-   ##
==========================================
- Coverage   91.75%   91.62%   -0.14%     
==========================================
  Files         464      465       +1     
  Lines       24763    24775      +12     
==========================================
- Hits        22722    22699      -23     
- Misses       1656     1687      +31     
- Partials      385      389       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codeboten · 2024-12-10T00:38:41Z

Could use some feedback from @open-telemetry/collector-approvers on this latest commit to the PR to update the config support from v0.2.0 to v0.3.0.

tl;dr there's a backwards incompatible change in how headers are passed in and I wanted to support users transitioning

i wanted to support both the 0.3.0 and the 0.2.0 schema, which resulted in some migration code. I still have a few tests to add, and this change will need a PR in go-contrib to be merged before it can be completely done. Anyways, i could use your input: df83265

This is waiting on open-telemetry/opentelemetry-go-contrib#6412

dmitryax · 2024-12-10T01:06:00Z

The approach to handle the backward incompatible change looks good to me 👍

dmitryax · 2024-12-10T01:07:16Z

service/telemetry/internal/migration/v0.3.0.go

+			// compatibility attempts
+			return err
+		}
+		// TODO: add a warning log to tell users to migrate their config


I guess it might be complicated to pass a logger here, but it's pretty important. I think we should come up with a solution by the time we release this change

The way we do this on the Datadog exporter is to have a private field that is written to during unmarshaling and then a method to log warnings.

I don't love it, but I haven't been able to come up with anything better

jpkrohling

I appreciate your efforts to not break our current users. And explicitly about this:

Anyways, i could use your input: df83265

I like your approach. We don't have a schema version anywhere, so trying to fallback to the previous version before failing is a good approach. It doesn't scale nicely, but I also don't see a good way to determine the version other than requiring users to provide a schema version (which is bad UX for this specific place of the config, IMO).

jpkrohling · 2024-12-10T08:42:45Z

service/service.go

 	return pcommonRes
 }
+
+func disableHighCardinalityMetrics() []config.View {


looks OK for now, but if we expect this list to grow before we remove the gate, it might make sense to externalize this somehow?

This is the purpose of #11754

service/service.go

mx-psi

I think this is a good opportunity to resolve #10808 by writing down what our plans are regarding this.

mx-psi · 2024-12-10T11:40:28Z

service/telemetry/internal/migration/v0.3.0.go

+			// compatibility attempts
+			return err
+		}
+		// TODO: add a warning log to tell users to migrate their config


The way we do this on the Datadog exporter is to have a private field that is written to during unmarshaling and then a method to log warnings.

I don't love it, but I haven't been able to come up with anything better

mx-psi · 2024-12-10T11:41:37Z

service/service.go

 	return pcommonRes
 }
+
+func disableHighCardinalityMetrics() []config.View {


This is the purpose of #11754

github-actions · 2025-01-01T03:19:18Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

codeboten · 2025-01-22T23:37:00Z

Attempting to fix the integration test that's failing in contrib: open-telemetry/opentelemetry-collector-contrib#37425

Unclear to me whether this change is making that test flakier or not as this issue exists: open-telemetry/opentelemetry-collector-contrib#34836

codeboten · 2025-01-23T06:28:29Z

Another attempt at fixing the contrib test open-telemetry/opentelemetry-collector-contrib#37431

mx-psi

The new place for views looks reasonable

service/service.go

mx-psi

I am fine with copy-pasting the scope name, it seems unlikely to change

codeboten · 2025-01-23T16:23:04Z

The contrib exporter-1 test is still consistently failing https://github.com/open-telemetry/opentelemetry-collector/actions/runs/12932562549/job/36070135348?pr=11611, @songy23 do you have any thoughts on what's causing it to fail? i can't repro locally

codeboten · 2025-01-23T17:01:23Z

i can't repro locally
was finally able to repro locally... will continue debugging the issue

songy23 · 2025-01-23T17:33:55Z

do you have any thoughts on what's causing it to fail?

Datadog relies on the internal meter to report some component specific metrics. Those metrics are then exposed with the internal Prometheus server. If anything changes the metric names / attributes from internal meter and/or Prometheus, it will break the Datadog integrations.

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

codeboten · 2025-01-24T01:31:00Z

Thanks for your help @songy23, bug has been fixed

This PR does a couple of things that I couldn't quite split up so I put together a PR w/ individual commits to help reviewers get through it. This PR does the following: 1. update `go.opentelemetry.io/contrib/config` package to latest. this brings in breaking changes. in order to prevent those breaking changes from impacting end users, i've also added a layer of config unmarshaling 2. updates the collector to instantiate the meter provider (and exporters) via the config package. this allows us to remove all the code in `otelinit`. the reason for including this change was that unmarshaling the config was causing circular dependencies i didn't want to address by moving code that could be deleted around. Replacement for open-telemetry#11458. Fixes open-telemetry#12021 --------- Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

dmitryax · 2025-02-13T05:41:43Z

service/service_test.go

 		"otelcol_process_runtime_heap_alloc_bytes":       false,
 		"otelcol_process_runtime_total_alloc_bytes":      false,
 		"otelcol_process_uptime":                         false,
+		"promhttp_metric_handler_errors_total":           false,


@codeboten, is this metric expected to be added? I'm not sure if it provides any value to the collector internal metrics...

it's added by the prometheus server that is started via the SDK. we might be able to drop it via a view

ok, that sounds good. Let me create an issue

codeboten force-pushed the codeboten/update-config-dependency branch 3 times, most recently from 6f225b7 to 47f403a Compare November 6, 2024 21:45

codeboten mentioned this pull request Nov 19, 2024

wip: update config dependency #11458

Closed

github-actions bot added the Stale label Nov 27, 2024

codeboten removed the Stale label Nov 27, 2024

codeboten force-pushed the codeboten/update-config-dependency branch from 47f403a to 2a0e6f8 Compare December 9, 2024 19:03

dmitryax reviewed Dec 10, 2024

View reviewed changes

jpkrohling approved these changes Dec 10, 2024

View reviewed changes

mx-psi reviewed Dec 10, 2024

View reviewed changes

codeboten force-pushed the codeboten/update-config-dependency branch 4 times, most recently from 3bcf5f1 to b0021fb Compare December 16, 2024 23:07

codeboten marked this pull request as ready for review December 16, 2024 23:07

codeboten requested a review from a team as a code owner December 16, 2024 23:07

codeboten requested a review from atoulme December 16, 2024 23:07

codeboten force-pushed the codeboten/update-config-dependency branch from 41beb42 to e30a736 Compare December 17, 2024 19:44

jade-guiton-dd mentioned this pull request Dec 20, 2024

Change internal logs config example to use OTLP/HTTP open-telemetry/opentelemetry.io#5831

Merged

codeboten mentioned this pull request Dec 21, 2024

Update All opentelemetry-go-contrib packages #11923

Closed

1 task

github-actions bot added the Stale label Jan 1, 2025

codeboten removed the Stale label Jan 6, 2025

jaronoff97 mentioned this pull request Jan 6, 2025

Mismatch in resource attributes between metrics and everything else #12021

Closed

fix test

d742afd

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

codeboten force-pushed the codeboten/update-config-dependency branch from d314b4d to d742afd Compare January 22, 2025 20:28

more lint

c5ca4ea

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

Merge branch 'main' into codeboten/update-config-dependency

7220af9

mx-psi reviewed Jan 23, 2025

View reviewed changes

service/service.go Show resolved Hide resolved

service/service.go Show resolved Hide resolved

service/service.go Show resolved Hide resolved

service/service.go Show resolved Hide resolved

Merge branch 'main' into codeboten/update-config-dependency

055d800

mx-psi approved these changes Jan 23, 2025

View reviewed changes

codeboten added 2 commits January 23, 2025 17:19

ensure default options are set

48a3953

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

Merge branch 'main' into codeboten/update-config-dependency

e30a30f

songy23 approved these changes Jan 24, 2025

View reviewed changes

andrzej-stencel approved these changes Jan 24, 2025

View reviewed changes

evan-bradley approved these changes Jan 24, 2025

View reviewed changes

Merge branch 'main' into codeboten/update-config-dependency

f2cddc3

codeboten enabled auto-merge January 24, 2025 17:37

codeboten added this pull request to the merge queue Jan 24, 2025

Merged via the queue into open-telemetry:main with commit 4edaacd Jan 24, 2025

codeboten deleted the codeboten/update-config-dependency branch January 24, 2025 18:06

joschi mentioned this pull request Feb 10, 2025

Breaking change: periodic exporter otlp.protocol #12332

Closed

dmitryax reviewed Feb 13, 2025

View reviewed changes

dmitryax mentioned this pull request Feb 13, 2025

New internal metric added promhttp_metric_handler_errors_total #12383

Open

jade-guiton-dd mentioned this pull request Feb 21, 2025

[service] Internal counter metrics now have a _total suffix in Prometheus #12458

Closed

codeboten mentioned this pull request Apr 23, 2025

[service/internal/graph] Measure telemetry as it is passed between pipeline components #12812

Merged

Conversation

codeboten commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjnowen commented Nov 12, 2024

Uh oh!

github-actions bot commented Nov 27, 2024

Uh oh!

mjnowen commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codeboten commented Dec 10, 2024

tl;dr there's a backwards incompatible change in how headers are passed in and I wanted to support users transitioning

Uh oh!

dmitryax commented Dec 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpkrohling left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mx-psi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 1, 2025

Uh oh!

codeboten commented Jan 22, 2025

Uh oh!

codeboten commented Jan 23, 2025

Uh oh!

mx-psi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mx-psi left a comment

Choose a reason for hiding this comment

Uh oh!

codeboten commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codeboten commented Jan 23, 2025

Uh oh!

songy23 commented Jan 23, 2025

Uh oh!

codeboten commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codeboten commented Nov 5, 2024 •

edited

Loading

mjnowen commented Nov 27, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

jpkrohling left a comment •

edited

Loading

codeboten commented Jan 23, 2025 •

edited

Loading