fix(flamegraph): Fix continuous flamegraph non call order#104582
fix(flamegraph): Fix continuous flamegraph non call order#104582
Conversation
| configSpaceTransform: | ||
| // For continuous flamegraphs, we only want to adjust when the sorting is | ||
| // call order. This is because we need to offset it to align with the | ||
| // specified start/end but when sorting by left heavy or alphabetical, | ||
| // we always align it at 0. | ||
| sorting === 'call order' | ||
| ? getProfileOffset(profile, configSpaceQueryParam[0]) | ||
| : undefined, | ||
| }, | ||
| }); | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // we always align it at 0. | ||
| sorting === 'call order' | ||
| ? getProfileOffset(profile, configSpaceQueryParam[0]) | ||
| : undefined, |
There was a problem hiding this comment.
Bug: Inconsistent offset logic between flamegraph and related chart views
The fix conditionally applies configSpaceTransform only when sorting === 'call order' for the flamegraphView, but the related views (uiFramesView, batteryChartView, cpuChartView, memoryChartView) still unconditionally apply the offset via getProfileOffset. When using alphabetical or left heavy sorting, the flamegraph won't have an offset but the charts will, causing visual misalignment between these synchronized views that share the same X-axis.
Additional Locations (2)
| configSpaceTransform: | ||
| // For continuous flamegraphs, we only want to adjust when the sorting is | ||
| // call order. This is because we need to offset it to align with the | ||
| // specified start/end but when sorting by left heavy or alphabetical, | ||
| // we always align it at 0. | ||
| sorting === 'call order' | ||
| ? getProfileOffset(profile, configSpaceQueryParam[0]) | ||
| : undefined, |
There was a problem hiding this comment.
Bug: Overlay views (uiFramesView, batteryChartView, cpuChartView, memoryChartView) unconditionally apply getProfileOffset() causing misalignment when flamegraphView uses undefined for configSpaceTransform in non-'call order' sorting.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When the flamegraph is sorted by a non-'call order' mode, the main flamegraphView is configured with configSpaceTransform: undefined (which becomes an identity matrix). However, the dependent overlay views (uiFramesView, batteryChartView, cpuChartView, memoryChartView) unconditionally apply getProfileOffset() to their configSpaceTransform. This discrepancy in transformation matrices for views sharing the same canvas but having synchronized configView rects causes them to render at different screen positions, leading to visual misalignment between the flamegraph and its overlaid metrics.
💡 Suggested Fix
Conditionally apply getProfileOffset() to configSpaceTransform for uiFramesView, batteryChartView, cpuChartView, and memoryChartView using the pattern sorting === 'call order' ? getProfileOffset(...) : undefined.
🤖 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/components/profiling/flamegraph/continuousFlamegraph.tsx#L607-L614
Potential issue: When the flamegraph is sorted by a non-'call order' mode, the main
`flamegraphView` is configured with `configSpaceTransform: undefined` (which becomes an
identity matrix). However, the dependent overlay views (`uiFramesView`,
`batteryChartView`, `cpuChartView`, `memoryChartView`) unconditionally apply
`getProfileOffset()` to their `configSpaceTransform`. This discrepancy in transformation
matrices for views sharing the same canvas but having synchronized `configView` rects
causes them to render at different screen positions, leading to visual misalignment
between the flamegraph and its overlaid metrics.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6607484
When using a non call order mode (e.g. alphabetical or left heavy), it's possible the start of the profile is empty. When this happens, the offset can push the actual flamegraph off screen. So don't use an offset in these sortings.