Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Remote reporter flush fix #59

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

isaachier
Copy link
Contributor

Guarantee a flush occurs on destruction of RemoteReporter. Add more noexcept specifiers for reporter methods.

Signed-off-by: Isaac Hier <isaachier@gmail.com>
return;
}
_running = false;
flush();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most important line in this review.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnburn suggested that flush() should only run - or at least only block - on an explicit call to Tracer.Close().

@codecov
Copy link

codecov bot commented Feb 9, 2018

Codecov Report

Merging #59 into master will increase coverage by 0.04%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   88.46%   88.51%   +0.04%     
==========================================
  Files          93       93              
  Lines        2246     2246              
==========================================
+ Hits         1987     1988       +1     
+ Misses        259      258       -1
Impacted Files Coverage Δ
src/jaegertracing/reporters/RemoteReporter.h 100% <ø> (ø) ⬆️
src/jaegertracing/reporters/Reporter.h 100% <ø> (ø) ⬆️
src/jaegertracing/reporters/CompositeReporter.h 100% <100%> (ø) ⬆️
src/jaegertracing/reporters/LoggingReporter.cpp 100% <100%> (ø) ⬆️
src/jaegertracing/reporters/InMemoryReporter.h 100% <100%> (ø) ⬆️
src/jaegertracing/reporters/NullReporter.h 100% <100%> (ø) ⬆️
src/jaegertracing/reporters/LoggingReporter.h 100% <100%> (ø) ⬆️
src/jaegertracing/reporters/RemoteReporter.cpp 73.33% <78.57%> (ø) ⬆️
src/jaegertracing/UDPTransport.cpp 93.87% <0%> (+2.04%) ⬆️

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 c36adcf...cdf9a1d. Read the comment docs.

Signed-off-by: Isaac Hier <isaachier@gmail.com>
@ringerc
Copy link
Contributor

ringerc commented Feb 12, 2018

Tested and works for me here, passes the shortlived test proc.

Thanks very much.

@isaachier isaachier merged commit acfb850 into jaegertracing:master Feb 12, 2018
@isaachier isaachier deleted the remote-reporter-flush-fix branch February 12, 2018 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants