feat(#90): adds support for finished span handler. #135
Conversation
| tracer *Tracer | ||
| mustCollect int32 // used as atomic bool (1 = true, 0 = false) | ||
| flushOnFinish bool | ||
| finishedSpanHandler func(*model.SpanModel) bool |
There was a problem hiding this comment.
fyi we have a list of these in java (ex one for metrics, one for dependency graph)
| shouldRecord = s.finishedSpanHandler(&s.SpanModel) | ||
| } | ||
|
|
||
| if shouldRecord && s.flushOnFinish { |
There was a problem hiding this comment.
I assume flushOnFinish is related to sampling bit. if so, sg
There was a problem hiding this comment.
Not sure that is the case. I think it is more for when you want to add more stuff after a span is finished. See https://github.com/openzipkin/zipkin-go/blob/master/span_options.go#L79
There was a problem hiding this comment.
gotcha. yeah I think this will want sampled local support before people start coding to it. Otherwise, later they might get a firehose and not notice. here's the docs from the java side.
/**
* Triggered on each finished span except when spans that are {@link Span#isNoop() no-op}.
*
* <p>{@link TraceContext#sampled() Sampled spans} hit this stage before reporting to Zipkin.
* This means changes to the mutable span will reflect in reported data.
*
* <p>When Zipkin's reporter is {@link zipkin2.reporter.Reporter#NOOP} or the context is
* unsampled, this will still receive spans where {@link TraceContext#sampledLocal()} is true.
*
* @see #alwaysSampleLocal()
*/| tracer, _ := NewTracer( | ||
| rep, | ||
| WithNoopSpan(false), | ||
| WithSampler(AlwaysSample), |
There was a problem hiding this comment.
need a test for never sample, that if the finished span handler needs all data it can still see it.
note in brave we have a flag on finished span handler which is always record or not (sampledLocal) you might need this to properly implement finished span handler.
|
@jcchavezs thanks for taking this on. I will review end of the week. |
|
After a quick glance, this is not ready yet. Promised my wife not to work the next few weeks so this will have to wait a little. |
jcchavezs
force-pushed
the
adds_finished_span_handler
branch
from
a1410cd to
5087c90
Compare
|
any news on this matter? |
This PR adds support for
finishedSpanHandlerin the same way as brave: https://github.com/openzipkin/brave/releases/tag/5.4.1Ping @basvanbeek @adriancole
Closes #90