Skip to content
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

Duplicate Sentry Reports for Native Client Crashes #1045

Open
3 tasks done
Atul767 opened this issue Jan 3, 2025 · 35 comments
Open
3 tasks done

Duplicate Sentry Reports for Native Client Crashes #1045

Atul767 opened this issue Jan 3, 2025 · 35 comments

Comments

@Atul767
Copy link

Atul767 commented Jan 3, 2025

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Electron SDK Version

4.22.0

Electron Version

27.2.20

What platform are you using?

None

Link to Sentry event

https://jupiter-ct.sentry.io/issues/5977075889/events/d8cb5ff92ddd4fbb9e7bd0380f77df83/?project=1430058 , https://jupiter-ct.sentry.io/issues/5105499169/events/49718a83fe6142b4a727520fb6ce24d8/?project=4506692758077440

Steps to Reproduce

  1. Launch the Native Client from the parent application
  2. Trigger a crash in the Native Client.
  3. Observe the crash being reported in both Sentry projects (one under the parent application, one under the Native Client).

We have observed an issue where the same crash is being reported in two separate Sentry projects:

The parent application's Sentry project (e.g., Jupiter).
The Native Client's Sentry project.
This duplication occurs when the Native Client is launched from the parent application. Both projects log the same crash as separate issues.

Example:
Here are two examples of duplicate issues for the same crash:

Parent Application Sentry Project: [MTKView draw]: Fatal Error: EXC_BREAKPOINT / EXC_ARM_BREAKPOINT / 0x100742e00 — Project: jupiter-ct.
Native Client Sentry Project: EXC_BREAKPOINT: height — Project: ringcentral-video-nc.

Expected Result

  • Prevent the parent application from sending Sentry reports for crashes that originate in the Native Client.
  • Ensure that Native Client crashes are reported only in the Native Client's Sentry project

Actual Result

This duplication occurs when the Native Client is launched from the parent application. Both projects log the same crash as separate issues.

@timfish
Copy link
Collaborator

timfish commented Jan 3, 2025

So the parent app is the Electron app, or the native app? Is the native app using the Sentry Native SDK?

Does this only occur on macOS or all platforms?

@Atul767
Copy link
Author

Atul767 commented Jan 3, 2025

Hi @timfish ,

So the parent app is the Electron app, or the native app?
Answer: The parent app is the Electron app.

Is the native app using the Sentry Native SDK?
Answer: Yes,

Does this only occur on macOS or all platforms?
Answer: Based on the Sentry logs, I am currently seeing this issue on macOS devices. I’m not yet certain about other platforms.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 3, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 3, 2025

The Sentry Electron SDK uses the Electron crashReporter API to capture native crashes and it states:

Note that if the crash reporter is started in the main process, it will automatically monitor child processes

There's no option to configure it so I don't think there's anything we can do to stop this. You may be able to custom build Electron and disable this but that would be a lot of work.

When starting your native code from Electron, you could set an environment variable that your native uses to disable crash handling?

@DamonYu6
Copy link

DamonYu6 commented Jan 6, 2025

Hi @timfish, If we call process.crashReporter.addExtraParameter('ignore', 'true') in the child_process and then filter out such crashes in beforeSend hook, would this approach be feasible? Additionally, I’m concerned that even if we manage to filter out these events from being uploaded, would it still impact the parent project’s Crash Free Session Rate?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 6, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 6, 2025

If we call process.crashReporter.addExtraParameter('ignore', 'true') in the child_process and then filter out such crashes in beforeSend hook, would this approach be feasible?

The beforeSend hook in the Electron main process will not be able to access anything like that set in the child processes. Native crashes are captured in minidumps which are passed through as attachments. Without parsing the minidump, there's no way to know what caused it or to access the child process metadata.

would it still impact the parent project’s Crash Free Session Rate?

Yes, even if you could filter in beforeSend I think these would still impact sessions.

You could switch the crash reporter off in your child process? Are you trying to avoid doing that because then all crashes will then go to the Electron app and you'd like them going to two different Sentry projects depending on the source?

@DamonYu6
Copy link

DamonYu6 commented Jan 7, 2025

You could switch the crash reporter off in your child process?

No, we cannot disable crash reporting for the child process because the native app is maintained by a separate development team. They have already enabled the Sentry Native SDK within the native app to report crashes to their own project.

Are you trying to avoid doing that because then all crashes will then go to the Electron app and you'd like them going to two different Sentry projects depending on the source?

Since the native app runs as a child process of the Electron app, its crashes are reported by the Sentry Electron SDK to the Electron project's Sentry instance as well, inflating the project's session crash rate — something we want to avoid. Do you have any suggestions?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 7, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 7, 2025

Both the Electron crashReporter and the Sentry Native SDK use crashpad to capture native crashes and it looks like by default it reports crashes from all child processes.

Do you have any suggestions?

We could add an option to ignore child process crashes from Electron but this would not be 100% reliable. It would have both false positives and false negatives.

The SDK gets events from Electron for renderer and child process crashes and we assume any minidumps found after those events correspond to the event type. If we find any minidumps at startup, we assume they are for the Electron main process because it exits immediately on crash. However, the assumptions we make are not always correct. For example, your event above that was sent from Electron has event.process: browser which means it was found at startup and we assumed it was from the main (browser) process. From what you've said, this was not the case and it came from a child process so this categorisation was wrong.

With Electron, minidumps are written by an external crashpad process and Electron notifies us but another minidump may be written by the time we read the disk. There are race conditions that we can only attempt to mitigate against.

If you look at all the native events in your Electron project that you'd like to filter out, what do they have for the event.process tag? Are they all browser or do they differ?

You could parse the minidumps and determine which process it came from. If I had to do this it would be via a native module using the minidump Rust crate and napi-rs to expose this to JavaScript. The minidump crate is able to access the main_module which should give the name of the executable. If you used this logic with beforeSend it would still impact crash free session stats but we could add another callback to help work around this.

@DamonYu6
Copy link

DamonYu6 commented Jan 9, 2025

If you look at all the native events in your Electron project that you'd like to filter out, what do they have for the event.process tag? Are they all browser or do they differ?

Although most of the events have event.process marked as "browser," there are still a few events where event.process is marked as "renderer."
https://jupiter-ct.sentry.io/issues/5977075889/events/db3cc9a2f424430cbba863f217bd2c7f/?project=1430058

If you used this logic with beforeSend it would still impact crash free session stats but we could add another callback to help work around this.

Could you explain in detail how to add these callbacks to work around the impact on the CFSR? It seems this is the only way we have currently. Thanks

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 9, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 9, 2025

I've been working on #1049 which should allow us to better determine which process generated the minidump. I need to test this with the Sentry Native SDK but I'm hoping you'll be able to set process_type in the Crashpad metadata which can be picked up here.

With this I can then add an option to the Electron SDK that will allow these to be ignored while not impacting CSFR.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 19, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 19, 2025

If I configure ignoredProcesses to include node, does that mean all crashes from processes categorized as node will be completely ignored and will not be sent to Sentry at all?

Yes, that is correct. They will be ignored and the mindumps deleted by the Electron SDK. It's worth noting that node processes are only from child_process.fork.

@Atul767
Copy link
Author

Atul767 commented Jan 19, 2025

Hi @timfish I wanted to clarify that the native client binary in our setup is being initialized using child_process.spawn, not fork any suggestion ?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 19, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 19, 2025

in our setup is being initialized using child_process.spawn, not fork

I have not tested this but I suspect child_process.spawn will have the same results as child_process.exec.

You will only get the process listed as node if you use child_process.fork as this results in a forked Node process.

@Atul767
Copy link
Author

Atul767 commented Jan 19, 2025

Hi @timfish

If I configure ignoredProcesses to include node, does that mean all crashes from processes categorized as node will be completely ignored and will not be sent to Sentry at all?

Yes, that is correct. They will be ignored and the mindumps deleted by the Electron SDK. It's worth noting that node processes are only from child_process.fork.

In our scenario, we launch a native client from our Electron app using child_process.spawn. Based on my understanding, the crashes from this native client would not be categorized as node in the minidump metadata, and therefore, filtering with ignoredProcesses: ['node'] won't apply. Even after #1050

Given this, we will still face a challenge in distinguishing or ignoring crashes specifically from this native client. Do you have any suggestions on how we can proceed?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 19, 2025
@timfish
Copy link
Collaborator

timfish commented Jan 19, 2025

Given this, we will still face a challenge in distinguishing or ignoring crashes specifically from this native client. Do you have any suggestions on how we can proceed?

I that case, crashes from your native client will be classified as unknown so you will be able to filter them out that way.

@Atul767
Copy link
Author

Atul767 commented Jan 20, 2025

Okay, great! Let’s try it out once the PR is merged and see how it integrates.

@timfish
Copy link
Collaborator

timfish commented Jan 20, 2025

The latest release (5.10.0) already reads the process type from the minidump and populates the event.process tag which can be used for filtering in `beforeSend. The only thing that #1050 adds is the ability to ignore these events while not impacting CFSR.

@timfish
Copy link
Collaborator

timfish commented Jan 26, 2025

Through some changes in @sentry/core, this PR fixes beforeSend handling so it now doesn't impact release health if you return null:

@timfish
Copy link
Collaborator

timfish commented Jan 28, 2025

v5.11.0 has been released.

This includes a fix for beforeSend so it should now not impact release health if you return null!

@Atul767
Copy link
Author

Atul767 commented Jan 28, 2025

Hi @timfish,

Thank you for the update! I wanted to confirm if simply upgrading the Sentry Electron SDK and applying the following change would be sufficient to resolve my issue:

Sentry.init({ dsn: '__DSN__', beforeSend: (event) => { return event?.tags?.['process.type'] === 'unknown' ? null : event; }, });
also its it ['process.type'] or ['event.type']
Would this approach effectively address the problem, or are there additional steps I need to take?

@timfish
Copy link
Collaborator

timfish commented Jan 30, 2025

Yes it's process.type and that code should fix your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

6 participants