Track dropped spans #11
Conversation
zipkin/src/tracer.cc
Outdated
| @@ -111,4 +112,51 @@ void Tracer::reportSpan(Span &&span) { | |||
| void Tracer::setReporter(ReporterPtr reporter) { | |||
| reporter_ = std::move(reporter); | |||
| } | |||
|
|
|||
| uint64_t Tracer::getDroppedSpanCount() const { | |||
There was a problem hiding this comment.
If we want to expose these as part of the Reporter, we wouldn't have to cast anything.
There was a problem hiding this comment.
Yes, that would be better.
Even if you want to cast, reinterpret_cast isn't the right one to use.
| if (spans_.addSpan(span)) { | ||
| num_spans_reported_++; | ||
| } else { | ||
| dropped_span_count_++; |
There was a problem hiding this comment.
This might be able to have loser memory ordering because of the mutex, but that might be a premature optimization.
zipkin/src/tracer.cc
Outdated
| uint64_t Tracer::getDroppedSpanCount() const { | ||
| ReporterImpl *reporter = reinterpret_cast<ReporterImpl *>(reporter_.get()); | ||
| if (reporter) { | ||
| return reporter->getDroppedSpanCount(); |
There was a problem hiding this comment.
These technically have race conditions. One could set the reporter to a nullptr, which would run the destructor on this pointer. Past that point, accessing the pointer is undefined behavior.
I say let's make it so you can't set the reporter pointer after construction.
|
Instead of naming methods |
zipkin/src/zipkin_reporter_impl.h
Outdated
| private: | ||
| TransporterPtr transporter_; | ||
|
|
||
| std::atomic_uint64_t dropped_span_count_{0}; |
There was a problem hiding this comment.
Can you use std::atomic<uint64_t> so that it matches what's used elsewhere.
KevinMGranger
force-pushed
the
drop_reporting
branch
from
ba577a8
to
4131663
Compare
|
Feedback implemented. |
zipkin/include/zipkin/tracer.h
Outdated
| @@ -123,6 +158,39 @@ class Tracer : public TracerInterface { | |||
| */ | |||
| void setReporter(ReporterPtr reporter); | |||
There was a problem hiding this comment.
Weren't you going to remove the setReporter method?
Co-authored-by: Kevin M Granger <git@kevinmgranger.me>
KevinMGranger
force-pushed
the
drop_reporting
branch
from
4131663
to
7b566b0
Compare
| * @param report_period The new size of the span buffer | ||
| */ | ||
| void setReportPeriod(SteadyClock::duration reporting_period) override { | ||
| reporting_period_ = reporting_period; |
There was a problem hiding this comment.
This has a race condition, no? Since reporting_period_ is read from another thread.
| } | ||
|
|
||
| uint64_t Tracer::getAndResetDroppedSpanCount() { | ||
| return reporter_->getAndResetDroppedSpanCount(); |
There was a problem hiding this comment.
What would you think of adding a reporter accessor method to the Tracer class? Then you can get rid of all of these forwarding methods.
There was a problem hiding this comment.
I think that makes sense.
| /** | ||
| * @return the number of spans that can be stored in the buffer | ||
| */ | ||
| virtual size_t bufferSpanCount() const { return 0; } |
There was a problem hiding this comment.
Could we rename bufferSpanCount to maxBufferedSpans or spanBufferSize? -- the name doesn't seem quite right.
|
@KevinMGranger -- how's this going? it looked like there was only a few issues left. |
|
Sorry for the holdup, I've been busy with some other work. I've also been trying to figure out a good testing strategy for this, unless you think the changes are simple enough that we don't need to worry about it. |
The second half of the work from #9.
This is a bigger change, so I'd appreciate feedback.
I'd like to add in more unit testing for this stuff, but haven't yet.