Fix leak in Jaeger exporter#1160
Conversation
lalitb
left a comment
lalitb
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix.
Agree, Jaeger exporter tests need to be improved.
Codecov Report
@@ Coverage Diff @@
## main #1160 +/- ##
=======================================
Coverage 93.38% 93.38%
=======================================
Files 165 165
Lines 6233 6233
=======================================
Hits 5820 5820
Misses 413 413 |
esigo
left a comment
esigo
left a comment
There was a problem hiding this comment.
thanks for the fix.
LGTM.
I will add more tests without mock to cover Thrift part.
Great! Was thinking of looking into this, but just let me know If I can be of any help. |
|
We can catch the leak in the new test. |
|
@bjosv - can you update the branch to main, to allow this to be merged. |
The jaeger_span was never destroyed after its release from the JaegerRecordable. This change let a `std::unique_ptr` handle the destruction when going out of scope, first allowing the `thrift::Span` to be copied to the `span_buffer_`.
bjosv
force-pushed
the
fix-leak-jaeger-exporter
branch
from
902fdfe to
be83b1d
Compare
|
@lalitb The PR has now been updated to the latest from main. |
|
CI step It doesn't seem to be related to my correction according to the output, but I'm not familiar with the test. |
|
Thanks for catching and fixing. |
4 participants
During our introduction of Jaeger and using the
JaegerExporterwe found this memory leak.There is currently no unittest for the
JaegerExporterin opentelemetry-cpp that could have caught this, but maybe we should add something similar to the ongoingZipkinExportertest (#1155)?Changes
The jaeger_span was never destroyed after its release from the JaegerRecordable.
This change let a
std::unique_ptrhandle the destruction when going out of scope, first allowing thethrift::Spanto be copied to thespan_buffer_.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes