Just to confirm, HandlePriorityTrace always returns false? Also, _keys seems to have unbounded growth unless I'm mistaken?

Regarding the unbounded keys, we said last time that as we already have clients with more than 1000 resources it could be worth it to limit the size of the set indeed (as it is done in Java IIRC what Zach had answered).

Just to confirm, HandlePriorityTrace always returns false?

Yes, according to the agent logic we should return false when the trace has a positive sampling priority.

Also, _keys seems to have unbounded growth unless I'm mistaken?

Yes, I didn't fix this in the last PR but now would be a good time to implement this. I'll think on the implementation and get back to this point.

Given the rare sampler is the only one with state, had we may as well just return immediately if this is true? If so, then FWIW, you could short-circuit here if preferable for readability:

    return _rareSampler.Sample(trace)
        || _prioritySampler.Sample(trace)
        || _errorSampler.Sample(trace)
        || _analyticsEventSampler.Sample(trace);

What you propose has really great readability. I'll try to do something similar in my coming changes, but note that the RareSampler must always run first (unless disabled), but its result is used as the very last fallback

OK I made a refactor similar to what you suggest in 69b3115 . The difference is that the last OR operand is the result of the _rareSampler.Sample(trace), which was run earlier in the method. The reason for this is documented in the code, so hopefully that's clear

OK I made a refactor similar to what you suggest in 69b3115
Cool.

The reason for this is documented in the code, so hopefully that's clear

The reasoning there is clear, thanks. And I understand keeping it like this for consistency with other traceers, but...

but note that the RareSampler must always run first (unless disabled), but its result is used as the very last fallback

This makes sense to me if the other samplers have side-effects. But as far as I can see, they don't. so if the rareSpanFound is true, the result is always true, and running the other samplers is unnecessary?

I'm not passionate about it, especially as if the other samplers do have side effects (or might in the future), then the order is important

Oh I see what you're saying about side effects, I wasn't fully understanding what you were saying that earlier. Yes since the other samplers have no side effects (right now) then we could possibly return early. I'll keep this as-is for now since we might eventually want to make more robust samplers (closer to what the agent has) that would need to keep some state

As it returns a boolean, I believe it would be better to call this ShouldKeepTrace

Done in 03e94ef

Maybe we could split the RareSampler in 2 parts:

• Span? Dectect(ArraySegment) that will register keys and return the first rare span if any
• bool Sample(Span?) that will add the tag if there's a span

The sampling part would be called only after all other samplers have been called.
I believe that would simplify the RareSampler code as you wouldn't have to handle differently spans with positive or negative priorities.

The sampling part would be called only after all other samplers have been called.

So after a bit of reading the agent code and testing, this isn't quite true. Here's the algorithm we need:

• Run the RareSampler first. If the sampling priority is > 0, the RareSampler will not mark any spans with "_dd.rare = 1". Otherwise, the first rare span in the chunk that's determined to be rare is marked as "_dd.rare = 1"
• If no other samplers decide to sample the trace chunk, return the result of the RareSampler as the decision for the trace

In commit 69b3115 I updated the RareSampler slightly. I documented the reasons for the algorithm so the algorithm itself didn't change too much, but I tried to refactor and add comments so it's clearer. Let me know what you think

Run the RareSampler first. If the sampling priority is > 0, the RareSampler will not mark any spans with "_dd.rare = 1". Otherwise, the first rare span in the chunk that's determined to be rare is marked as "_dd.rare = 1"
If no other samplers decide to sample the trace chunk, return the result of the RareSampler as the decision for the trace

So to make sure IUC, the difference in behaviour with what I suggested is the fact that a span can be marked as _dd.rare = 1 even though it is sampled by another sampler (eg we have a span with prio <=0, it is rare, we add the tag, but it also contains an error, so it is returned anyway). Whereas in my suggestion, we wouldn't add the _dd.rare = 1 in that case. Right?

Regarding the unbounded keys, we said last time that as we already have clients with more than 1000 resources it could be worth it to limit the size of the set indeed (as it is done in Java IIRC what Zach had answered).

Should we add a helper method like MarkedAsKept (not the best name but you see what I mean) that would contain that code
trace.Array[trace.Offset].Context.TraceContext.SamplingPriority is int p && p > 0;

Implemented in 45c2227 with name IsKeptBySamplingPriority