feat(source-map-debug): Connect source map debug modal to backend #56257
Conversation
github-actions
bot
added
the
Scope: Frontend
There was a problem hiding this comment.
Maybe this is already handled in the modal code (didn't review the original PR) but do we show the wizard snippet for all of the JS SDKs? I think it's generally fine but for SDKs like Nextjs, Remix and Sveltekit, it might not be optimal.
Also, (probably more important) do we show the DebugId tab for Next and Sveltekit?
Yep, we show it for all SDKs. Afaik the wizard can always be used - worst case people need to fall back to CLI.
Dang almost forgot to implement this. So right now we will at least not show the "(recommended)" label for these platforms. Lemme push something along the lines of "you are using an SDK that officially doesn't support debug IDs. Please use the release process instead.". I would still like to show the tab. By default they will probably put onto the release tab anyhow. |
| import { | ||
| mapSourceMapDebuggerFrameInformation, | ||
| useSourceMapDebuggerBlueThunder, | ||
| } from 'sentry/components/events/interfaces/crashContent/exception/useSourceMapDebuggerBlueThunder'; |
There was a problem hiding this comment.
m: the name is cool, but let's just remove blue thunder - otherwise we have to remember to come back and change it.
There was a problem hiding this comment.
We have a useSourceMapDebug already 😢 What do I do?
There was a problem hiding this comment.
As it returns sourceMapDebuggerData maybe call it useSourceMapDebuggerData?
| debuggerFrame.debug_id_process.uploaded_source_map_with_correct_debug_id, | ||
| sdkVersion: sourceMapDebuggerData.sdk_version, | ||
| matchingSourceMapName: | ||
| debuggerFrame.release_process?.matching_source_map_name ?? null, |
There was a problem hiding this comment.
l: If this is our mapped data, can we just avoid using null? undefined should just work for our use case right?
There was a problem hiding this comment.
Lemme think about this. The endpoint returns null in many cases so we would have to map it to undefined.
There was a problem hiding this comment.
I took a shot at implementing this by widening the type to accept null | undefined but it complicates the logic downstream so I would not do this right now. Does that make sense?
| refetchOnWindowFocus: false, | ||
| } | ||
| ); | ||
| return sourceMapDebuggerData; |
There was a problem hiding this comment.
l: shouldn't we also return isLoading and isError here?
There was a problem hiding this comment.
Since we display the button to open the as a tag on the stack frames we probably don't want a loading indicator so we don't care about isLoading.
If the request errors out we probably also don't care.
| export function frameIsFullyResolvedBasedOnDebuggerData( | ||
| frameDebuggerData: FrameSourceMapDebuggerData | ||
| ): boolean { | ||
| const {debugIdProgressPercent} = getDebugIdProgress(frameDebuggerData); |
There was a problem hiding this comment.
l: I don't like that we have to run the getters to decide if we have to render the component, and then run it again when the component renders.
Can we lift this logic out of the SourceMapsDebuggerModal and inject in the progress percent as props?
There's a bunch of props to inject so we could also inject it all as a single object.
There was a problem hiding this comment.
The progress could already be calculated in mapSourceMapDebuggerFrameInformation.
Even though it then does a bit more than mapping 🤷
| const frameSourceMapDebuggerData = sourceMapDebuggerData?.exceptions[ | ||
| excIdx | ||
| ].frames.map(debuggerFrame => | ||
| mapSourceMapDebuggerFrameInformation(sourceMapDebuggerData, debuggerFrame) | ||
| ); |
There was a problem hiding this comment.
Use useMemo so the array is not re-created every time.
There was a problem hiding this comment.
Do you see an easy way of doing this? I am asking because we're already one .map() deep which means I can't just simply useMemo here without majorly refactoring this component :/ (I believe)
Since this component basically re-renders very rarely anyhow I would just keep it like this for now. Thoughts?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This connects the source map debugger modal to the current version of our source maps debug endpoint.
Additionally, we show a pill on stack frames that aren't symbolicated.
The "self hosting" tab is currently disabled as we are waiting on work in symbolicator to forward data needed for this tab.
This feature is currently behind a feature flag.
Ref: #55662