Update core to latest & fix impacted components #3442

gramidt · 2021-05-20T19:08:08Z

Description:
Updated core and updated impacted components with the latest Client Authentication Extension addition.

Impacted components:

Exporters:

lokiexporter (I'm the codeowner)
dynatraceexporter
sumologicexporter
influxdbexporter
signalfxexporter
f5cloudexporter (I'm the codeowner)
awsprometheusremotewriteexporter
humioexporter
uptraceexporter

Receivers:

nginxreceiver

Extensions:

httpforwarderextension

I have split up each component into their own commit and ensured all changes to that component remained in their corresponding commit.

Link to tracking Issue:
#3438

Testing:
Unit tests

Documentation:
Unit tests

Notes
This one was a fun one. PLEASE TAKE EXTRA CARE AND ASSUME I MISSED SOMETHING when reviewing your components in this PR, as some were straightforward others required more effort.

gramidt · 2021-05-20T21:39:22Z

The first round of the build and test errors have been sorted out. I am now reviewing all components that use the confighelpers to see if there's a clash in settings ( "auth" field). I know 100% this is an issue with the f5cloudexporter.

gramidt · 2021-05-20T21:53:43Z

Confirmed list of components that use the impacted confighelpers and have a config setting field named "auth" at the top level of the config (with and without 'squash'):

f5cloudexporter (Codeowner: gramidt)

gramidt · 2021-05-20T21:54:16Z

Since I am the codeowner of the f5cloudexporter I will update it in this PR to avoid issues.

gramidt · 2021-05-20T22:40:40Z

For some reason the humioexporter didn't show up in earlier failures. Working on it now...

gramidt · 2021-05-20T23:52:13Z

Now working on the uptrace exporter...

gramidt · 2021-05-21T01:52:21Z

This one was a fun one. PLEASE TAKE EXTRA CARE AND ASSUME I MISSED SOMETHING when reviewing your components in this PR, as some were straightforward others required more effort.

I have split up each component into their own commit and ensured all changes to that component remained in their corresponding commit.

gramidt · 2021-05-21T01:54:47Z

cc @tigrannajaryan @jpkrohling @pavankrish123

gramidt · 2021-05-21T02:53:19Z

[UPDATE] SHOULD BE RESOLVED - PENDING A FEW TEST RUNS

There appears to be a flaky test for the signalfx exporter. It has failed twice now and has passed other times. I will dig into this ASAP.

[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x8d1814]

goroutine 19 [running]:
net/http.(*Client).deadline(0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/http/client.go:189 +0x34
net/http.(*Client).do(0x0, 0xc00022a500, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/http/client.go:604 +0xbd
net/http.(*Client).Do(...)
	/usr/local/go/src/net/http/client.go:586
github.com/signalfx/signalfx-agent/pkg/apm/requests.sendRequest(0x0, 0xc00022a500, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/circleci/go/pkg/mod/github.com/signalfx/signalfx-agent/pkg/apm@v0.0.0-20201202163743-65b4fa925fc8/requests/sender.go:116 +0x8f
github.com/signalfx/signalfx-agent/pkg/apm/requests.(*ReqSender).sendRequest(0xc0000ce780, 0xc00022a500, 0x1, 0x1)
	/home/circleci/go/pkg/mod/github.com/signalfx/signalfx-agent/pkg/apm@v0.0.0-20201202163743-65b4fa925fc8/requests/sender.go:72 +0x79
github.com/signalfx/signalfx-agent/pkg/apm/requests.(*ReqSender).processRequests(0xc0000ce780)
	/home/circleci/go/pkg/mod/github.com/signalfx/signalfx-agent/pkg/apm@v0.0.0-20201202163743-65b4fa925fc8/requests/sender.go:62 +0x256
created by github.com/signalfx/signalfx-agent/pkg/apm/requests.(*ReqSender).Send
	/home/circleci/go/pkg/mod/github.com/signalfx/signalfx-agent/pkg/apm@v0.0.0-20201202163743-65b4fa925fc8/requests/sender.go:44 +0x11a
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/correlation	0.047s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/dimensions	20.081s	coverage: 85.4% of statements
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/hostmetadata	0.089s	coverage: 100.0% of statements
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/translation	0.189s	coverage: 96.9% of statements
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/translation/dpfilters	0.056s	coverage: 100.0% of statements
FAIL

tigrannajaryan · 2021-05-21T14:01:03Z

There appears to be a flaky test for the signalfx exporter. It has failed twice now and has passed other times. I will dig into this ASAP.

Please file a bug so that we fix the flaky test.

tigrannajaryan

@gramidt thank you for working on this.

jpkrohling

I'm approving the components that I own and the general semantics of the auth changes. Other component owners still need to give their approvals.

jpkrohling · 2021-05-21T15:01:34Z

exporter/awsprometheusremotewriteexporter/auth_test.go

@@ -109,7 +110,7 @@ func TestLeakingBody(t *testing.T) {
 			return newSigningRoundTripperWithCredentials(authConfig, awsCreds, next, sdkInformation)
 		},
 	}
-	client, _ := setting.ToClient()
+	client, _ := setting.ToClient(componenttest.NewNopHost().GetExtensions())


If you prefer, you can use something like map[config.ComponentID]component.Extension{} instead.

That's also a great option, @jpkrohling! Which way do you believe is the correct convention for us to use moving forward? I know we typically use componentest when possible, so I thought following that pattern made the most sense at the time of this PR.

exporter/f5cloudexporter/README.md

gramidt · 2021-05-21T16:17:27Z

@gramidt thank you for working on this.

Anytime, @tigrannajaryan! It's all about the community :)

gramidt · 2021-05-21T16:18:56Z

It seems that "Publish-2" had an issue. I'll re-kick this off when we're ready.

https://app.circleci.com/pipelines/github/open-telemetry/opentelemetry-collector-contrib/14123/workflows/48ca9007-bc9e-4aac-a634-4626559eaeec/jobs/121072

gramidt · 2021-05-21T18:19:56Z

There appears to be a flaky test for the signalfx exporter. It has failed twice now and has passed other times. I will dig into this ASAP.

Please file a bug so that we fix the flaky test.

I believe it was caused by me and I fixed it. I have been running the entire suite locally without seeing that issue again. Once everyone has had the opportunity to review, I will re-kick the tests a few times to be sure. If I see it again, I will file an issue (assuming I didn't cause it with these changes).

pavankrish123 · 2021-05-21T18:30:40Z

Thank you @gramidt

gramidt · 2021-05-21T21:49:26Z

Just rebased to get latest changes.

gramidt · 2021-05-24T11:56:10Z

Friendly ping CODEOWNERS.

This PR is critical since it's needed before we can pull any further updates from the core collector.

bogdandrutu

There are too many changes that break the components. We need to rollback the change in core, and make PRs to move the http client initialization in Start as a separate PR.

bogdandrutu · 2021-05-24T18:46:34Z

exporter/dynatraceexporter/metrics_exporter.go

@@ -232,6 +227,18 @@ func (e *exporter) sendBatch(ctx context.Context, lines []string) (int, error) {
 	return 0, nil
 }

+// start starts the exporter
+func (e *exporter) start(_ context.Context, host component.Host) (err error) {


This is not used in the factory as "WithStart" so it will not be called.

bogdandrutu · 2021-05-24T18:47:42Z

exporter/humioexporter/traces_exporter.go

@@ -235,6 +258,18 @@ func tagFromSpan(evt *HumioStructuredEvent, strategy Tagger) string {
 	}
 }

+// start starts the exporter
+func (e *humioTracesExporter) start(_ context.Context, host component.Host) (err error) {


Same, This is not used in the factory as "WithStart" so it will not be called.

gramidt · 2021-05-24T19:00:28Z

@jpkrohling @pavankrish123 - Please see @bogdandrutu comments above. Sadly, I over extended myself to work on this PR to help out, but I do not have the necessary bandwidth to rollback and adjust this effort in the near future. Would either of you be able to work with @bogdandrutu on this?

bogdandrutu · 2021-05-24T21:36:00Z

@gramidt I think you can start splitting from this PR, smaller PRs that simply change where ToClient is called (in start). Then when the change to the API is rolledback in core we can simply just add the extensions to the call.

gramidt · 2021-05-24T21:42:36Z

I should be able to manage that, @bogdandrutu! I will start now and see how far I get with my limited time today.

gramidt · 2021-05-24T21:43:37Z

Closing this in favor of @bogdandrutu approach.

gramidt marked this pull request as ready for review May 21, 2021 01:49

gramidt requested review from alolita, anuraaga, arminru, djaglowski, dmitryax, jmacd, jpkrohling, jrcamp, kbrockhoff, mx-psi and owais as code owners May 21, 2021 01:49

gramidt requested a review from a team May 21, 2021 01:49

github-actions bot assigned bogdandrutu May 21, 2021

tigrannajaryan reviewed May 21, 2021

View reviewed changes

jpkrohling approved these changes May 21, 2021

View reviewed changes

gramidt added 15 commits May 21, 2021 15:39

updated core to latest

830e4bd

fixed Loki exporter

0e189f6

fixed httpforwarder extension

f9f6ff7

fixed dynatrace exporter

e3282b6

fixed sumologic exporter

9ba5170

fixed influxdb exporter

2574f07

fixed signalfx exporter

6da1b8b

fixed f5cloud exporter duplicate auth field

140c65e

fixed awsprometheusremotewrite tests

46f88f0

fixed humio exporter

2e8fd69

fixed upstrace exporter

9c3ccfc

fixed nginx receiver

7697ce0

requeuing build

59a4134

requeue tests

f01a070

requeueing tests

216bfcc

Aneurysm9 approved these changes May 21, 2021

View reviewed changes

bogdandrutu requested changes May 24, 2021

View reviewed changes

pavankrish123 mentioned this pull request May 24, 2021

Revert "Support for Custom Exporter Authenticators as Extensions (#3128)" open-telemetry/opentelemetry-collector#3281

Closed

gramidt closed this May 24, 2021

gramidt mentioned this pull request May 24, 2021

Prepare components for the Client Authentication Extensions #3507

Closed

pavankrish123 mentioned this pull request May 25, 2021

Prepare components for the Client Authentication Extensions open-telemetry/opentelemetry-collector#3287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update core to latest & fix impacted components #3442

Update core to latest & fix impacted components #3442

gramidt commented May 20, 2021 •

edited

Loading

gramidt commented May 20, 2021

gramidt commented May 20, 2021 •

edited

Loading

gramidt commented May 20, 2021

gramidt commented May 20, 2021

gramidt commented May 20, 2021

gramidt commented May 21, 2021 •

edited

Loading

gramidt commented May 21, 2021

gramidt commented May 21, 2021 •

edited

Loading

tigrannajaryan commented May 21, 2021

tigrannajaryan left a comment

jpkrohling left a comment

jpkrohling May 21, 2021

gramidt May 21, 2021

gramidt commented May 21, 2021

gramidt commented May 21, 2021

gramidt commented May 21, 2021

pavankrish123 commented May 21, 2021

gramidt commented May 21, 2021

gramidt commented May 24, 2021

bogdandrutu left a comment

bogdandrutu May 24, 2021

bogdandrutu May 24, 2021

gramidt commented May 24, 2021

bogdandrutu commented May 24, 2021

gramidt commented May 24, 2021

gramidt commented May 24, 2021

Update core to latest & fix impacted components #3442

Update core to latest & fix impacted components #3442

Conversation

gramidt commented May 20, 2021 • edited Loading

gramidt commented May 20, 2021

gramidt commented May 20, 2021 • edited Loading

gramidt commented May 20, 2021

gramidt commented May 20, 2021

gramidt commented May 20, 2021

gramidt commented May 21, 2021 • edited Loading

gramidt commented May 21, 2021

gramidt commented May 21, 2021 • edited Loading

tigrannajaryan commented May 21, 2021

tigrannajaryan left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling May 21, 2021

Choose a reason for hiding this comment

gramidt May 21, 2021

Choose a reason for hiding this comment

gramidt commented May 21, 2021

gramidt commented May 21, 2021

gramidt commented May 21, 2021

pavankrish123 commented May 21, 2021

gramidt commented May 21, 2021

gramidt commented May 24, 2021

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu May 24, 2021

Choose a reason for hiding this comment

bogdandrutu May 24, 2021

Choose a reason for hiding this comment

gramidt commented May 24, 2021

bogdandrutu commented May 24, 2021

gramidt commented May 24, 2021

gramidt commented May 24, 2021

gramidt commented May 20, 2021 •

edited

Loading

gramidt commented May 20, 2021 •

edited

Loading

gramidt commented May 21, 2021 •

edited

Loading

gramidt commented May 21, 2021 •

edited

Loading