loadbalancingexporter: narrow down critical area of ConsumeTraces #1798

ZhengHe-MD · 2020-12-10T11:46:38Z

Description:

quick fix on the problem that rolling updates of next tier collectors will stop dns resolver from periodical checking.

Link to tracking Issue:

#1690

Testing:

Add a test simulate rolling updates.

Documentation:

None

jpkrohling

The fix looks great, the test needs some attention.

jpkrohling · 2020-12-10T11:50:33Z

exporter/loadbalancingexporter/exporter_test.go

+	}()
+
+	// wait until rolling updates finish
+	time.Sleep(100 * time.Millisecond)


Instead of waiting a specific time, which might lead to intermittent CI failures, it would be better to create a WaitGroup and Add/Wait based on conditions.

jpkrohling · 2020-12-10T11:52:01Z

exporter/loadbalancingexporter/exporter_test.go

+	})
+
+	// keep consuming traces every 2ms
+	go func() {


Even if it's just a test, you'd need a way to break out of this loop. Perhaps add a channel that is closed after the test is finished (currently, the time.Sleep on line 615)?

jpkrohling · 2020-12-10T11:52:53Z

exporter/loadbalancingexporter/exporter_test.go

+		for {
+			select {
+			case <- ticker.C:
+				go p.ConsumeTraces(context.Background(), randomTraces())


Do you need this to be another go routine? With this change, this shouldn't block long anymore, right?

yes, thx, you're right. I should remove the go.

ah. after a deep thought, I found that it does need the go. because I use time.Sleep in ConsumeTraces to simulate unreachable backends, if I don't use go routine, this loop will stuck there and stop consuming traces.

ZhengHe-MD · 2020-12-10T12:14:23Z

@jpkrohling thx for reviewing my codes. I will certainly pay attention to the test. and tell you when it's ready.

…ments

codecov · 2020-12-11T00:43:16Z

Codecov Report

Merging #1798 (f9e8dd9) into master (a045ffc) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1798      +/-   ##
==========================================
- Coverage   89.84%   89.82%   -0.03%     
==========================================
  Files         378      378              
  Lines       18213    18213              
==========================================
- Hits        16363    16359       -4     
- Misses       1387     1389       +2     
- Partials      463      465       +2

Flag	Coverage Δ
integration	`69.77% <ø> (ø)`
unit	`88.52% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
exporter/loadbalancingexporter/exporter.go	`92.23% <100.00%> (ø)`
receiver/collectdreceiver/receiver.go	`68.49% <0.00%> (-2.74%)`	⬇️
receiver/k8sclusterreceiver/watcher.go	`95.29% <0.00%> (-2.36%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a045ffc...f9e8dd9. Read the comment docs.

ZhengHe-MD · 2020-12-11T01:17:40Z

@jpkrohling the PR is ready for review, please take a look when you have time

pkositsyn · 2020-12-11T17:54:47Z

exporter/loadbalancingexporter/exporter_test.go

+	go func() {
+		time.Sleep(50 * time.Millisecond)
+		resolverCh <- 1
+	}()
+
+	<-resolverCh


You write to the channel twice (second time in onLookupIPAddr) and read once. If I'm not mistaken, it should be just close of the channel in this goroutine (actually without goroutine) and read in onLookupIPAddr. This way no future calls to onLookupIPAddr will block (yay, the goroutine isn't leaked). Correct me if I'm wrong, but now I see that the possible execution order is that read in 635 line corresponds to write in onLookupIPAddr, which makes this goroutine have no effect.

Btw for channels without value I would use chan struct{} to make it obvious that you don't need the value. While reading, I was keeping the thought in mind like "there should be the reason why int channel is used, let's look what effect have these values"

I see now, that it is done to avoid blocking in tests. Then I would just suggest making a buffered channel (size 1 is enough) to avoid goroutine leak

thx, chan int does look confusing, will use chan struct{}, and also a size 1 buffered channel

pkositsyn · 2020-12-11T17:58:38Z

exporter/loadbalancingexporter/exporter_test.go

+	// ensure using default exporters
+	p.updateLock.Lock()
+	p.exporters = defaultExporters
+	p.updateLock.Unlock()
+	p.res.onChange(func(endpoints []string) {
+		p.updateLock.Lock()
+		p.exporters = defaultExporters
+		p.updateLock.Unlock()
+	})


Nit: onChange can be no-op function.

yes! you're right! and I just know this after you pointed out. but that's a hidden fact for this specific test case and I would rather keep this for the sake of completeness and readability. if I change the test case to simulate the following process:

["127.0.0.1", "127.0.0.2"] -> ["127.0.0.1", "127.0.0.2", "127.0.0.3"] -> ["127.0.0.2", "127.0.0.3"] -> ["127.0.0.2", "127.0.0.3", "127.0.0.4"] -> ["127.0.0.3", "127.0.0.4"]

the onChange can't be a no-op function then.

The test can later be updated then ;-)

ZhengHe-MD · 2020-12-12T04:36:01Z

@pkositsyn thx for reviewing this, and I've modify the test according to your suggestions. please see if there is any other issue when you have time. 😬

jpkrohling · 2020-12-14T09:54:56Z

exporter/loadbalancingexporter/exporter_test.go

+	// ensure using default exporters
+	p.updateLock.Lock()
+	p.exporters = defaultExporters
+	p.updateLock.Unlock()
+	p.res.onChange(func(endpoints []string) {
+		p.updateLock.Lock()
+		p.exporters = defaultExporters
+		p.updateLock.Unlock()
+	})


The test can later be updated then ;-)

exporter/loadbalancingexporter/exporter_test.go

bogdandrutu · 2020-12-24T16:01:49Z

@jpkrohling friendly ping, is there any comment you have left?

jpkrohling · 2020-12-28T15:09:52Z

From an earlier comment from this PR:

Yes, I wanted to come up with a proposal using WaitGroup constructs before going out for PTO but couldn't. Would it be OK to wait for me to return (Jan 11)? Otherwise, please open an issue and ping me there, so that we can address it later.

github-actions · 2021-01-05T05:45:16Z

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

ZhengHe-MD · 2021-01-09T00:01:00Z

@jpkrohling This PR is declared stale now and I don't see any change you've made in the new branch. If you still have some doubts with the test, maybe I can remove the test first.

jpkrohling · 2021-01-12T14:35:00Z

It was marked automatically as stale by a GitHub bot. I added a link to a gist with what I had in mind, let me know what you think.

… the type (#1798) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* Change NewSplitDriver paramater and initialization * Update CHANGELOG.md Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Update CHANGELOG.md Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Move splitdriver option into options.go and rename * Update CHANGELOG.md Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Change option name and nil test to snapshots * Update exporters/otlp/protocoldriver.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/protocoldriver.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/options.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/options.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/options.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update exporters/otlp/options.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Change SplitDriverOption to match spec * Update changelog entry Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

loadbalancingexporter: narrow down critical area of ConsumeTraces

f9d41a5

ZhengHe-MD requested review from jpkrohling and a team as code owners December 10, 2020 11:46

github-actions bot assigned james-bebbington Dec 10, 2020

jpkrohling reviewed Dec 10, 2020

View reviewed changes

ZhengHe-MD added 2 commits December 11, 2020 00:20

loadbalancingexporter: refactor rolling updates test according to com…

e6c7222

…ments

loadbalancingexporter: fix race conditions in test

f4e351b

pkositsyn reviewed Dec 11, 2020

View reviewed changes

loadbalancingexporter: avoid goroutine leaks in test

72e8171

ZhengHe-MD requested a review from jpkrohling December 12, 2020 14:02

jpkrohling reviewed Dec 14, 2020

View reviewed changes

loadbalancingexporter: fix typo in comment

f9e8dd9

ZhengHe-MD requested a review from jpkrohling December 16, 2020 05:20

github-actions bot added the Stale label Jan 5, 2021

github-actions bot removed the Stale label Jan 9, 2021

jpkrohling approved these changes Jan 13, 2021

View reviewed changes

bogdandrutu merged commit 8345baf into open-telemetry:master Jan 19, 2021

dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021

Remove SetType from configmodels, ensure all registered factories set…

3796e60

… the type (#1798) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loadbalancingexporter: narrow down critical area of ConsumeTraces #1798

loadbalancingexporter: narrow down critical area of ConsumeTraces #1798

ZhengHe-MD commented Dec 10, 2020

jpkrohling left a comment

jpkrohling Dec 10, 2020

jpkrohling Dec 10, 2020

jpkrohling Dec 10, 2020

ZhengHe-MD Dec 10, 2020 •

edited

ZhengHe-MD Dec 10, 2020 •

edited

ZhengHe-MD commented Dec 10, 2020

codecov bot commented Dec 11, 2020 •

edited

ZhengHe-MD commented Dec 11, 2020

pkositsyn Dec 11, 2020 •

edited

pkositsyn Dec 11, 2020

ZhengHe-MD Dec 12, 2020

pkositsyn Dec 11, 2020

ZhengHe-MD Dec 12, 2020 •

edited

jpkrohling Dec 14, 2020

ZhengHe-MD commented Dec 12, 2020

jpkrohling Dec 14, 2020

bogdandrutu commented Dec 24, 2020

jpkrohling commented Dec 28, 2020

github-actions bot commented Jan 5, 2021

ZhengHe-MD commented Jan 9, 2021 •

edited

jpkrohling commented Jan 12, 2021

loadbalancingexporter: narrow down critical area of ConsumeTraces #1798

loadbalancingexporter: narrow down critical area of ConsumeTraces #1798

Conversation

ZhengHe-MD commented Dec 10, 2020

jpkrohling left a 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

ZhengHe-MD Dec 10, 2020 • edited

Choose a reason for hiding this comment

ZhengHe-MD Dec 10, 2020 • edited

Choose a reason for hiding this comment

ZhengHe-MD commented Dec 10, 2020

codecov bot commented Dec 11, 2020 • edited

Codecov Report

ZhengHe-MD commented Dec 11, 2020

pkositsyn Dec 11, 2020 • 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

ZhengHe-MD Dec 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhengHe-MD commented Dec 12, 2020

Choose a reason for hiding this comment

bogdandrutu commented Dec 24, 2020

jpkrohling commented Dec 28, 2020

github-actions bot commented Jan 5, 2021

ZhengHe-MD commented Jan 9, 2021 • edited

jpkrohling commented Jan 12, 2021

ZhengHe-MD Dec 10, 2020 •

edited

ZhengHe-MD Dec 10, 2020 •

edited

codecov bot commented Dec 11, 2020 •

edited

pkositsyn Dec 11, 2020 •

edited

ZhengHe-MD Dec 12, 2020 •

edited

ZhengHe-MD commented Jan 9, 2021 •

edited