ref(transactions): Ensure that every span's parent exists #4661
Conversation
In the span buffer we rely on the parent_span_id to cluster spans back into transactions/segments. If we send out transactions that contain "dangling" spans, then flatten those transactions into individual spans, we won't be able to build the same segment back together.
| .filter_map(|span| span.value()) | ||
| .filter_map(|span| span.span_id.value()) | ||
| .chain(Some(root_span_id)) | ||
| .cloned() |
There was a problem hiding this comment.
I really dislike this. In this expression we access span_id, in the for-loop we only access parent_span_id. In theory those are mutually exclusive accesses, and should be able to be borrowed at the same time. But it's tricky to convince the compiler of that, and so I have to clone here.
It would be better to make SpanId a u16 anyway...
There was a problem hiding this comment.
Either this or we serialize like in ProjectKey which supports as_str and as_ref::<str>:
relay/relay-base-schema/src/project.rs
Lines 85 to 90 in cbf72c2
There was a problem hiding this comment.
tried to change this but too much stuff has to be adjusted to make it work since .0 is pub. let's just ship as-is
There was a problem hiding this comment.
Right, part of this would be refactoring all usage to remove access to the field.
Co-authored-by: Jan Michael Auer <[email protected]>
| if event.ty.value() == Some(&EventType::Transaction) { | ||
| span::fix_trees::fix_trees(event); | ||
| } |
There was a problem hiding this comment.
Should we not put this inside the conditional block below?
| /// | ||
| /// This is to avoid any nasty surprises in the span buffer specifically. Other readers of spans | ||
| /// (such as the frontend's tree component) were already able to deal with detached spans. | ||
| pub fn fix_trees(event: &mut Event) { |
There was a problem hiding this comment.
Maybe a name along the lines of reparent_spans_to_root is more expressive?
In the span buffer we rely on the parent_span_id to cluster spans back
into transactions/segments. If we send out transactions that contain
"dangling" spans, then flatten those transactions into individual spans,
we won't be able to build the same segment back together.