fix(ds): Emit dynamic sampling outcomes for spans contained in transaction #4569
Conversation
Dav1dde
force-pushed
the
dav1d/ds-spans-in-transaction
branch
3 times, most recently
from
e9e816e to
2f59f50
Compare
Dav1dde
force-pushed
the
dav1d/ds-spans-in-transaction
branch
from
2f59f50 to
dd58c9d
Compare
| // | ||
| // This is yet another case, when the spans have not yet been separated from the transaction | ||
| // also emit dynamic sampling outcomes for the contained spans. | ||
| if !spans_extracted.0 { |
There was a problem hiding this comment.
nit: Assuming top-level extracted spans are in the dropped_items list, isn't it cleaner to read it from there and sum them up?
It's for sure more expensive because we would have to iterate but maybe it's clearer.
There was a problem hiding this comment.
It would be the wrong signal, we have that explicit boolean which makes the decision if the contained spans should be counted or not (it's the spans_extracted here).
In theory it's possible to have a transaction and spans in the same envelope and then it becomes messy, so the boolean is the one thing that should always accurately track that state.
There was a problem hiding this comment.
Yeah, I was not referring to the condition, I was referring to the counting of spans through dropped items of type ItemType::Span.
There was a problem hiding this comment.
dropped_items will not contain any spans if spans_extracted is false.
When changing the span outcomes in #4569, I fixed this test incorrectly making it flaky. When the test runner is fast enough it would never fail (locally) but in CI this was noticed because the tests run slower. This is the correct fix.
Dynamic sampling currently only emits outcomes for the transaction but not the contained spans. With this PR sampled/filtered outcomes are also emitted for contained spans.