Rename or rejig Sampler

[codefromthecrypt]

Sampled returning false in sleuth is different than most instrumentation I've seen. For example, a span is managed regardless of whether it is sampled (i.e. still generates callbacks etc). The Sampled state controls the exportable field, which in turn controls propagation and storage (and a special-case for users of TraceManager.addAnnotation(key, value) as opposed to getCurrentSpan)

I'd suggest 2 paths, which could both be taken.

• Rename Sampler to ShouldExport. Document the propagation and storage aspects there.
• Move the ShouldExport logic to the end of a span. Since we are gathering a Span regardless anyway, we might as well use it in the export decision. In other words, change the invocation from before-the-fact (before a span id is created) until after-the-fact (when a span ends or times out).

If controlling overhead thing is an intended goal of the Sampler, then maybe the before-the-fact is still the right choice. I'd have a few suggestions on that.

• Trim the state needed.. seems we shouldn't be creating callbacks and the like for unsampled spans. That or it should be clear why we do (maybe it is to others).
• Expand guarding to things beyond storage, propagation, TraceManager.setAttribute, or invert the logic. Ex. don't use the Span object at all if not sampled.. use something different (assuming the goal of keeping it around is to coordinate ids or something).

[codefromthecrypt]

here's a related issue with the ruby tracer, which was accidentally gathering state for spans that are tossed openzipkin/zipkin-ruby#40

[marcingrzejszczak]

I think that this is a little bit related to our discussions about the "baggage api". For the upcoming release I'd take the 1st path. However ATM the decission about exporting takes place in the instrumentation logic (e.g.):

• https://github.com/spring-cloud/spring-cloud-sleuth/blob/master/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/AbstractTraceChannelInterceptor.java#L61
• https://github.com/spring-cloud/spring-cloud-sleuth/blob/master/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java#L131
• https://github.com/spring-cloud/spring-cloud-sleuth/blob/master/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/SpanMessageHeaders.java#L59
• https://github.com/spring-cloud/spring-cloud-sleuth/blob/master/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceAsyncClientHttpRequestFactoryWrapper.java#L52

How would you like to move it to the end of the span?

[marcingrzejszczak]

I wouldn't do anything about this for the 1.0.0 release. Let's move it for the next big release.

[codefromthecrypt]

since a year has passed, probably needs another look in case (my) understanding is better now

[marcingrzejszczak]

I actually have an idea that we could leave the sampler as a name but maybe think of moving sampling as far as possible? Actually to the moment when the span is closed? That way users can do adaptive sampling (which is impossible ATM?). WDYT @adriancole ?

[codefromthecrypt]

You can think of adaptive sampling as a combination of parts. first, it is expressed like reservoir sampling, say accepting up to 1k traces/second. That's the target rate. Your actual rate will be different, and need to be corrected (adapted). In easiest case, you never have 1k traces/second, so you never have to correct the rate. How would you know? First, you need to count sample requests over time, like sample requests per minute. Then, you need to count how many sample requests you approve over time. When the latter rate is higher than the target, you need to adapt the sample rate below 100% (you might start with 0%) Commonly, you would "check" your rate at a higher frequency, like once a second. If you are higher than your target, you use some strategy to lower the sample rate. If the latter, you raise the rate. This is the adaptive part. This isn't super-smart because Zipkin doesn't have a capacity in terms of traces because traces can have an arbitrary amount of spans. However, it is smarter than a fixed percentage of traces (because it allows for budget). If you want to get smarter, you can either get smarter about what you keep (trace with error vs normal trace), or get smarter about how much you can keep (system capacity). Getting smarter about what you keep is not possible in sleuth, as it cannot see the result of downstream, and has no means to coordinate a sampling decision. For this, you'd need to do something like send 100% to zipkin-sparkstreaming and move the sample decision there. Getting smarter about how much you can keep is possible. For example, if you read back a metric of spans accepted by a collector, you can do some math to adjust to a target rate based on that. Tricky part is coordination as multiple servers may be sampling (even if it is equivalent nodes in the same cluster). For example, do you split 10k spans/second equally between 10 nodes? what if one dies, or 10 more are added? This gets to rather more complex code, like the zookeeper sampler used by twitter (note this is a collector sampler) https://github.com/openzipkin/zipkin/tree/master/zipkin-zookeeper I don't want to distract the issue here, too much, just think of adaptive as some function and the hard part is modeling the flow of information that can tell you to accept more or less. You can get arbitrarily fancy and the edge cases can become really difficult.

[marcingrzejszczak]

Thanks for the analysis. Makes perfect sense. I simplified the adaptive sampling issue. I think that we should close this one for now.