feat: adds support for isNoop check on spans. #181
Conversation
Span.IsNoop is specially usefull to avoid expensive operations when the tags or logs are not being recorded e.g. if you are about to read the body and process it to obtain certain tags, the check would save that computation as no changes will take effect.
| @@ -39,6 +39,9 @@ func spanName(rti *stats.RPCTagInfo) string { | |||
|
|
|||
| func handleRPC(ctx context.Context, rs stats.RPCStats) { | |||
| span := zipkin.SpanFromContext(ctx) | |||
| if span.IsNoop() { | |||
| return | |||
There was a problem hiding this comment.
in go I guess we don't need to apply the context (to ensure sampled false is carried down stream)?
There was a problem hiding this comment.
That is a good question. My idea was that even if sampled=false is propagated we still get a trace ID and client could choose to ignore that and continue the trace? Not sure this is a valid use case but secondary sampler came to my mind.
There was a problem hiding this comment.
we had problems in brave where we early exited instead of making the span part of the context on noop. I suppose here is not responsible for making the span.. something else did, hence you are checking isNoop... iotw I think you already addressed this concern.
|
another way to solve could be a package level method testing if it is a noop span (or non existent) and not add it to the Span interface. you could internally in the package test if the actual noopSpan implementation is in play. |
|
I see @basvanbeek, something like fun |
…basvanbeek's suggestion IsNoop is more of the concern of the tracer and not necessarily of the span's concern, hence turning this method into a function.
|
PTAL @basvanbeek |
Span.IsNoop is specially usefull to avoid expensive operations when the tags or logs are not being recorded e.g. if you are about to read the body and process it to obtain certain tags, the check would save that computation as no changes will take effect.
References:
Ping @adriancole @basvanbeek