feat(trace-tree-node): BaseNode implementation changes to achieve feature parity #101873
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Frontend
Abdkhan14
commented
Oct 21, 2025
| for (const error of value.errors) { | ||
| this.parent.errors.add(error); | ||
| } | ||
| const closestEAPTransaction = this.findParentEapTransaction(); |
Contributor
Author
There was a problem hiding this comment.
We were propagating issues to the closest parent and not the closest transaction parent, like in the legacy trace tree node implementation.
Abdkhan14
commented
Oct 21, 2025
| value.start_timestamp * 1e3, | ||
| (value.timestamp - value.start_timestamp) * 1e3, | ||
| ]; | ||
| if ( |
Contributor
Author
There was a problem hiding this comment.
Adds the validation for value.timestamp used to initialize TransactioNode.space[1]. Safer, helps pass a bunch of tests in trace.spec.tsx
Abdkhan14
commented
Oct 21, 2025
| transaction.parent = parent; | ||
| } | ||
|
|
||
| // Invalidating the node and its children to ensure the tree is rebuilt correctly, |
Contributor
Author
There was a problem hiding this comment.
This block should run after all of the reparenting is done.
- Invalidates at the end, ensures that node depths and connectors lazily calculated at render, are accurate.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
gggritso
reviewed
Oct 27, 2025
Member
gggritso
left a comment
gggritso
left a comment
There was a problem hiding this comment.
Makes sense, just a few questions!
| return ( | ||
| <TraceSpanRow | ||
| {...props} | ||
| // Won't need this cast once we use BaseNode type for props.node |
Contributor
Author
There was a problem hiding this comment.
Spot on, I'll remove it.
| get projectSlug(): string { | ||
| // The span value does not have a project slug, so we need to find a parent that has one. | ||
| // If we don't find one, we return the default project slug. | ||
| return this.findParent(p => !!p.projectSlug)?.projectSlug ?? 'default'; |
Member
There was a problem hiding this comment.
Is there guaranteed to be a parent with a project slug? I'm wondering if it makes sense to fetch project slugs from ProjectsStore instead?
Contributor
Author
There was a problem hiding this comment.
@gggritso Yeah, we always expect spans to be embedded under a transaction in the waterfall. Instead of looking for a transaction with a projectSlug, I look for a parent here to be more generic.
I don't fully get the ProjectsStore route 🤔 , re: a trace can have multiple projects, there's no projectID attached to spans to look for a specific project by
Comment on lines
+456
to
+463
| findParentTransaction(): TransactionNode | null { | ||
| return this.findParent<TransactionNode>(p => isTransactionNode(p)); | ||
| } | ||
|
|
||
| findParentEapTransaction(): EapSpanNode | null { | ||
| return this.findParent<EapSpanNode>(p => isEAPTransactionNode(p)); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Is this possible to do without type guards?
Contributor
Author
There was a problem hiding this comment.
@gggritso, I think that this is a good use case for a type guard. We are looking for a specific node in the base node class. The problem initially was that such type guards were scattered all over the place, we mitigated that quite a lot.
We can add is_transaction, is_eap_transaction, is_span... attrs to the baseNode classe but I don't really think that is any cleaner 🤔
We need methods like findParentTransaction if we are looking for a specific property that only exists in a TransactionNode type, like transaction id. The usage of the guard is now embedded in the baseNode instance so we can just do node.findParentTransaction instead of node.find(p => isTransactionNode(p)) which is an improvement.
Open to suggestions, lmk!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| this.transactionId === id || | ||
| hasMatchingErrors || | ||
| hasMatchingOccurrences | ||
| ); |
Contributor
There was a problem hiding this comment.
Bug: Tree Traversal: A Performance Bottleneck
The matchById method calls this.transactionId, which triggers parent tree traversal via findParentTransaction() and findParentEapTransaction() on every invocation. When matchById is called repeatedly during tree searches, this creates expensive redundant parent traversals for nodes that don't override matchById, significantly impacting performance on large trace trees.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes the changes required in BaseNode and it's subclasses to achieve feature parity as we replace
TraceTreeNodewithBaseNodein the stacked PR: feat(trace-tree-node): Replacing legacy TraceTreeNode usage with new BaseNode class #101874Added tests for new BaseNode methods
Got rid of all typescript errors form BaseNode and its subclasses
Added in line PR comments for blocks that aren't trivial