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.

Sign up for GitHub

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

Jump to bottom

cmd/trace-agent: fix trace-agent stop behaviour #2980

Merged
merged 1 commit into from
Feb 18, 2019
Merged

cmd/trace-agent: fix trace-agent stop behaviour #2980

merged 1 commit into from
Feb 18, 2019

Conversation

apoorv007
Copy link
Contributor

@apoorv007 apoorv007 commented Feb 1, 2019 •
edited
Loading

What does this PR do?

When Stop() is called on trace-agent, this PR

  1. fixes goroutine leaks
  2. prevents 100% cpu usage due to infinite spin-loop in writer/stats.go

@apoorv007 apoorv007 requested a review from a team as a code owner February 1, 2019 16:11
@gbbr gbbr changed the title fix trace-agent stop behaviour cmd/trace-agent: fix trace-agent stop behaviour Feb 1, 2019
@gbbr gbbr added the team/agent-apm trace-agent label Feb 1, 2019
@gbbr gbbr added this to the 6.10.0 milestone Feb 1, 2019
gbbr
gbbr reviewed Feb 1, 2019
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for this! We will review it soon.

releasenotes/notes/fix-trace-agent-goroutine-leak-c3be040113bbd6d2.yaml Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 1, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #2980 into master will increase coverage by 0.02%.
The diff coverage is 35.23%.

@@            Coverage Diff             @@
##           master    #2980      +/-   ##
==========================================
+ Coverage   53.29%   53.32%   +0.02%     
==========================================
  Files         532      532              
  Lines       37710    37755      +45     
==========================================
+ Hits        20098    20131      +33     
- Misses      16414    16427      +13     
+ Partials     1198     1197       -1
Impacted Files Coverage Δ
pkg/trace/writer/stats.go 90.76% <100%> (+0.04%) ⬆️
pkg/trace/api/api.go 75.74% <42.85%> (+1.36%) ⬆️
pkg/trace/agent/sampler.go 27.84% <6.25%> (-1.39%) ⬇️
pkg/trace/api/listener.go 71.15% <85.71%> (+13.25%) ⬆️
pkg/forwarder/worker.go 90% <0%> (-2.5%) ⬇️
pkg/forwarder/transaction.go 82.9% <0%> (ø) ⬆️

@apoorv007 apoorv007 force-pushed the master branch 2 times, most recently from e41aae6 to d74cfff Compare February 1, 2019 21:46
gbbr
gbbr reviewed Feb 4, 2019
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

This is great work (and very much needed). Can you please consider my improvement recommendations?

cmd/trace-agent/sampler.go Outdated Show resolved Hide resolved
pkg/trace/api/api.go Outdated Show resolved Hide resolved
pkg/trace/api/api.go Outdated Show resolved Hide resolved
releasenotes/notes/fix-trace-agent-goroutine-leak-c3be040113bbd6d2.yaml Outdated Show resolved Hide resolved
@apoorv007
Copy link
Contributor Author

Hi. Please let me know if you have any more comments on this.

gbbr
gbbr approved these changes Feb 18, 2019
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I was on vacation. This is great! Thank you very much! We are currently in code freeze for the 6.10.0 release but will merge this as soon as that's lifted. 🙇

pkg/trace/writer/stats.go
@@ -256,7 +256,7 @@ func (w *StatsWriter) monitor() {
select {
case e, ok := <-monC:
if !ok {
break
return
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

pkg/trace/api/api.go
r.exit <- struct{}{}
<-r.exit

r.PreSampler.Stop()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@gbbr
Copy link
Contributor

gbbr commented Feb 18, 2019

Seems like it's still ok to merge this, so I'm merging it. Thanks again!

@gbbr gbbr merged commit c9b7561 into DataDog:master Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbbr gbbr gbbr approved these changes

Assignees
No one assigned
Labels
team/agent-apm trace-agent
Projects
None yet
Milestone
6.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@apoorv007 @codecov-io @gbbr