Make buffered channel to avoid goroutines leak in groupbytraceprocessor #1505
Conversation
|
|
pkositsyn
force-pushed
the
groupbytrace_leak
branch
from
995c48f
to
a0de46b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1505 +/- ##
==========================================
+ Coverage 87.59% 88.93% +1.33%
==========================================
Files 345 346 +1
Lines 16975 17057 +82
==========================================
+ Hits 14870 15170 +300
+ Misses 1648 1420 -228
- Partials 457 467 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I cannot imagine test, which wouldn't use |
|
I know we have a PR on Jaeger making use of |
pkositsyn
force-pushed
the
groupbytrace_leak
branch
from
fcc0550
to
dcba408
Compare
| # /bin/bash | ||
| # Run this script only from current directory! | ||
|
|
||
| go test -c -o tests | ||
| for test in $(go test -list . | grep -E "^(Test|Example)"); | ||
| do ./tests -test.run "^$test\$" &>/dev/null || echo "$test failed"; | ||
| done | ||
| rm tests |
There was a problem hiding this comment.
Took this script from goleak repo. It is extremely convenient to find leaky test. But still, do we need it here and should I copy the license?
There was a problem hiding this comment.
How accurate is it? Would it have reliably found this leak? If we are copying the code, it should come with the same license header.
There was a problem hiding this comment.
That said, would you be able to make it work with all other modules in the repo? Would be a good addition to the repo itself, I believe.
There was a problem hiding this comment.
Generally, as I understood it just compares goroutines running before all tests and after all tests in a module. Now the tests fail with unbuffered channel, so it's really helpful here (also helped to find minor leak in shutdown). I am curious about stability of this method and there is a good point in discussion in Jaeger repo that, unfortunately, the main test must be put in every package manually. From my perspective, it can be helpful in components that use goroutines for servers or async processing. And don't see any point to use it otherwise
Well, the script is copied not entirely, but I admit that we have to put the license
There was a problem hiding this comment.
I think I would prefer to leave it out for now from this PR. Instead, would you open an issue so that we can discuss a better, broader solution? Perhaps there's a linter out there that we can make use of?
pkositsyn
force-pushed
the
groupbytrace_leak
branch
from
3724c00
to
c5f7a99
Compare
|
Seems like I messed up with CLA. Should I make another PR? |
|
Close in order to fix CLA in new PR #1532 |
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
* Update README master branch URL to main * Remove master branch from workflow triggers The master branch has been renamed to main. * Add changes to CHANGELOG * Rename other projects default branch All of OpenTelemetry is moving to rename `master` to `main`, this updates all other URLs for those projects. Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Description: Fixing a bug - goroutines leak
Link to tracking Issue: #1504