Skip to content

feat(node): Add abnormal session support for ANR #9268

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

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Oct 16, 2023

This PR:

  • Adds the abnormal_mechanism property to session types
  • Passes the latest session from main process to ANR process
  • If ANR is triggered and there's an active session, send the abnormal session
  • Adds a test to check for the session update

This PR makes no attempt to suppress further session updates from the main app process. How does Sentry backend treat conflicting session updates?

Ref: getsentry/sentry-electron#774

@AbhiPrasad
Copy link
Member

How does Sentry backend treat conflicting session updates

After the session hits a terminal state it cannot be updated anymore, so I think we are fine.


const testScriptPath = path.resolve(__dirname, 'basic-session.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
@romtsn
Copy link
Member

romtsn commented Oct 16, 2023

How does Sentry backend treat conflicting session updates

After the session hits a terminal state it cannot be updated anymore, so I think we are fine.

Sorry for chiming in, this is correct^, but this should be enforced client-side iirc. On the server we can't really do that, because we have aggregates and don't keep reference on a single session, so there's no way to know whether it was terminated already when a new update comes in.

We've already faced this on Android once, when were sending multiple Crashed updates for a single session which resulted in an incorrect crash-free rate.

@romtsn
Copy link
Member

romtsn commented Oct 16, 2023

Also, note that it's currently hardcoded for android-only and behind a feature flag, we'd have to relax those conditions (maybe even remove the flag altogether, it's been out for almost a year, but need to check with @shruthilayaj)

@timfish
Copy link
Collaborator Author

timfish commented Oct 16, 2023

I've modified this PR so the ANR child process now notifies the main process that an ANR has been triggered and we clear any session from the scope.

This is probably not completely foolproof since we may have lost contact with the main process.

@AbhiPrasad AbhiPrasad merged commit df08e8f into getsentry:develop Oct 17, 2023
@timfish timfish deleted the feat/anr-session-support branch October 17, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants