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.

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

Jaeger: ThriftSender unit test #1162

Merged
merged 8 commits into from
Jan 3, 2022

Conversation

esigo
Copy link
Member

@esigo esigo commented Dec 22, 2021

Adds unit test to cover the memory leak issue reported in #1160

Changes

Mocks Transport so that we can reach ThriftSender::Append when JaegerExporter::Export was called.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #1162 (11eee93) into main (09fb4e0) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
+ Coverage   93.25%   93.26%   +0.02%     
==========================================
  Files         173      173              
  Lines        6362     6362              
==========================================
+ Hits         5932     5933       +1     
+ Misses        430      429       -1     
Impacted Files Coverage Δ
...include/opentelemetry/sdk/common/circular_buffer.h 100.00% <0.00%> (+2.09%) ⬆️

@esigo esigo changed the title [WIP] Jaeger: ThriftSender unit test Jaeger: ThriftSender unit test Dec 22, 2021
@esigo esigo marked this pull request as ready for review December 22, 2021 18:52
@esigo esigo requested a review from a team December 22, 2021 18:52
@lalitb
Copy link
Member

lalitb commented Dec 27, 2021

LGTM. Shouldn't this be successful now that #1160 is merged? cc @bjosv

@esigo
Copy link
Member Author

esigo commented Dec 27, 2021

LGTM. Shouldn't this be successful now that #1160 is merged? cc @bjosv

Seems there was a timeout in batch_span_processor_test:
//sdk/test/trace:batch_span_processor_test TIMEOUT in 300.1s

Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry for late response.

@ThomsonTan ThomsonTan merged commit 4bffa63 into open-telemetry:main Jan 3, 2022
@esigo esigo deleted the Jaeger-ThriftSender-unit-test branch January 3, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants