feat(trace-tree-node): Mitigating SpanNode type guard usage #104481
Conversation
github-actions
bot
added
the
Scope: Frontend
…e-tree-node-span-node-guards
| const {projects} = useProjects(); | ||
| const previous = node.previous; | ||
|
|
||
| if (isEAPSpanNode(node.previous)) { |
There was a problem hiding this comment.
Combined the two MissingInstrumentationNodeDetails to one.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
|
|
||
| if (isEAPSpanNode(node.previous)) { | ||
| return <EAPMissingInstrumentationNodeDetails {...props} projects={projects} />; | ||
| } | ||
|
|
||
| if (isSpanNode(node.previous)) { | ||
| const event = node.previous.event; | ||
| const project = projects.find(proj => proj.slug === event?.projectSlug); | ||
| const profileMeta = getProfileMeta(event) || ''; | ||
| const profileContext = event?.contexts?.profile ?? {}; | ||
|
|
||
| return ( | ||
| <BaseMissingInstrumentationNodeDetails | ||
| {...props} | ||
| profileMeta={profileMeta} | ||
| project={project} | ||
| event={event} | ||
| profileId={profileContext.profile_id} | ||
| profilerId={profileContext.profiler_id} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function EAPMissingInstrumentationNodeDetails({ | ||
| projects, | ||
| ...props | ||
| }: TraceTreeNodeDetailsProps<NoInstrumentationNode> & { | ||
| projects: Project[]; | ||
| }) { | ||
| const {node} = props; | ||
| const previous = node.previous as EapSpanNode; | ||
|
|
||
| const { | ||
| data: eventTransaction = null, | ||
| isLoading: isEventTransactionLoading, | ||
| isError: isEventTransactionError, | ||
| } = useTransaction({ | ||
| event_id: previous.value.transaction_id, | ||
| const {data: eventTransaction = null} = useTransaction({ | ||
| event_id: previous.transactionId ?? '', | ||
| organization: props.organization, | ||
| project_slug: previous.value.project_slug, | ||
| project_slug: previous.projectSlug ?? '', |
There was a problem hiding this comment.
Bug: Refactoring removed type guards, causing SpanNodes to pass an empty project_slug to useTransaction due to incorrect property access.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The refactoring removed type guards, causing SpanNodes to be incorrectly processed. The new unified code project_slug: previous.projectSlug ?? '' relies on BaseNode.projectSlug getter. For SpanNodes, this.value (of type TraceTree.Span) lacks a project_slug property, causing BaseNode.projectSlug to return undefined. This undefined is then coerced to an empty string ''. Consequently, the useTransaction hook receives an empty project_slug, preventing correct data fetching and breaking the missing instrumentation feature for SpanNodes.
💡 Suggested Fix
Reintroduce type-specific logic to correctly extract project_slug for SpanNodes, likely by accessing node.previous.event?.projectSlug or adjusting the BaseNode.projectSlug getter to handle SpanNode's structure.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
static/app/views/performance/newTraceDetails/traceDrawer/details/missingInstrumentation.tsx#L23-L27
Potential issue: The refactoring removed type guards, causing `SpanNode`s to be
incorrectly processed. The new unified code `project_slug: previous.projectSlug ?? ''`
relies on `BaseNode.projectSlug` getter. For `SpanNode`s, `this.value` (of type
`TraceTree.Span`) lacks a `project_slug` property, causing `BaseNode.projectSlug` to
return `undefined`. This `undefined` is then coerced to an empty string `''`.
Consequently, the `useTransaction` hook receives an empty `project_slug`, preventing
correct data fetching and breaking the missing instrumentation feature for `SpanNode`s.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6321181
There was a problem hiding this comment.
We assign the parent's projectSlug for spanNodes.
gggritso
left a comment
gggritso
left a comment
There was a problem hiding this comment.
Makes sense, just some questions about code re-use
There was a problem hiding this comment.
👀 what's the difference between this and TraceSpanRow? Is TraceSpanRow still in use somewhere?
There was a problem hiding this comment.
It looks like a lot of code between this, TraceSpanRow and TraceUptimeCheckTimingNode is shared, is that true?
measurementsgetter to the baseNode class and used it to remove SpanNode guards