-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Fallback to custom protocol #377
Conversation
a9844fe
to
ea4333b
Compare
I haven't yet decided what to do about 🤔
|
Currently v2 and v3-beta add the following contexts: Renderer JavaScript and native crashes "contexts": {
"electron": {
"crashed_process": "WebContents[1]",
"crashed_url": "app:https:///src/index.html"
}
}, Main process JavaScript and native crashes: "contexts": {
"electron": {
"crashed_process": "browser"
}
}, The naming of these I would suggest that we move
Or would When it comes to URLs, we already have these for renderer JavaScript events via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, the tests make everything pretty easy to understand.
Won't work if the renderer crashes before SDK init
I think we shouldn't spend too much time on this. Other platforms have a similar problem - the only solution is if the framework/OS gives us a way to read those crash events afterwards.
If multiple renderers use the same path, we can only link them by the order that they are started which will not be reliable
Is it a common pattern in Electron apps for multiple renderers to use the same path? If so, let's just not worry too much about linking processName
and window.location.href
, maybe we leave it for a future patch and just open an issue to collect feedback.
would event.origin be a more suitable tag for the source process
IMO the 3 tags shown in the table provide adequate cardinality to identify the crash location, don't think we need to change event.origin
.
For renderer native crashes, shall we keep this at contexts.electron.crashed_url
Yup think this is a great idea.
Lmk and I can go over for a final review. Thanks Tim, you a champ!
); | ||
|
||
// We include sentry_key in the URL so these dont end up in fetch breadcrumbs | ||
// https://github.com/getsentry/sentry-javascript/blob/a3f70d8869121183bec037571a3ee78efaf26b0b/packages/browser/src/integrations/breadcrumbs.ts#L240 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda hacky - but a nice solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe a bit fragile. I initially attempted to filter the breadcrumbs when they're sent from renderer to main but realised that scope updates were triggering fetch, which was triggering scope updates, which.... yeah not great!
Might be nice to have a way to disable Sentry fetch breadcrumbs via a magic header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme ask around to see what others think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're pinned to a specific version of @sentry/browser
so this is only prone to breaking on dependency updates. Shall we merge this as it is and I'll open an issue on getsentry/javascript
to track this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please, opening an issue would be great
I imagine so. We've got a production app that has multiple windows that use a single entry point and the UI varies entirely by the startup hash/fragment identifier . If your multiple windows share most of their code, this is much easier than configuring webpack for shared dependency chunks between multiple entry points! Anyway, my hairbrained idea was linking renderers via the |
Closes #376 and #372
Adds support for receiving renderer events and scope via a custom
protocol
.ipcMode: IPCMode
option to main process. DefaultIPCMode.Both
enables both IPC and protocol for best compatibilitygetSessions: () => [session]
option to main process. Defaults to() => [session.defaultSession]
session
s ifdefaultSession
doesn't sufficesession
s for setting up the protocol and preload injectioninit
must be called before appready
andsession
s cannot be accessed until after appready
!ipcMode
isIPCMode.Protocol
orIPCMode.Both
PreloadInjection
integration modified to usegetSessions
option and not to run at all whenipcMode
isIPCMode.Protocol
Also:
contexts.electron.crashed_process
totags['event.process']
so users can filter by process typeAdded Tests
ipcMode: IPCMode.Protocol
ipcMode: IPCMode.Protocol
Content-Security-Policy
<iframe>
within renderer!init
after app ready