Skip to content

feat(node): Application mode sessions #3423

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 1 commit into from
May 5, 2021
Merged

Conversation

ahmedetefy
Copy link
Contributor

@ahmedetefy ahmedetefy commented Apr 20, 2021

This PR adds API to capture application mode sessions associated with a specific release, and send them to Sentry as part of the Release Health functionality.

  • When you call Sentry.init we start a session
  • autoSessionTracking is now a global option in both @sentry/browser & @sentry/node and is true by default
  • If we are not able to detect (or the users sets) a release we do not send any sessions
  • A session is crashed if an error/promise was captured with the global handlers and handled: false
  • A session is errored if an error/promise was captured with handled: true fix(node): Sets mechanism handled to false when it is a crash #3493
  • beforeExit we capture it -> sending it to Sentry
    • if the session state was in a terminal, we do not send it before the process exists because our error handlers captured it before

See getsentry/develop#323 for more info

@ahmedetefy ahmedetefy force-pushed the application-mode-session branch 2 times, most recently from 022a598 to cb82655 Compare April 20, 2021 15:44
@github-actions
Copy link
Contributor

github-actions bot commented Apr 20, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.63 KB (-0.02% 🔽)
@sentry/browser - Webpack 21.5 KB (0%)
@sentry/react - Webpack 21.53 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.92 KB (0%)

@ahmedetefy ahmedetefy force-pushed the application-mode-session branch 7 times, most recently from c8a4edb to fe53418 Compare April 20, 2021 16:44
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good now. Thanks for the hard work! We're missing here a test exercising and showing how a user creates an application mode session (using the @sentry/node SDK).

@ahmedetefy ahmedetefy force-pushed the application-mode-session branch 2 times, most recently from 2cdd29f to 812f317 Compare April 21, 2021 10:12
@ahmedetefy ahmedetefy marked this pull request as ready for review April 21, 2021 10:12
@ahmedetefy ahmedetefy requested a review from kamilogorek as a code owner April 21, 2021 10:12
@ahmedetefy ahmedetefy requested a review from rhcarvalho April 21, 2021 10:12
@ahmedetefy ahmedetefy force-pushed the application-mode-session branch 5 times, most recently from 388220d to f5c89f6 Compare April 26, 2021 11:33
@ahmedetefy ahmedetefy force-pushed the application-mode-session branch from f5c89f6 to 9ad7925 Compare April 26, 2021 13:04
@ahmedetefy ahmedetefy requested a review from rhcarvalho April 26, 2021 13:05
@ahmedetefy ahmedetefy force-pushed the application-mode-session branch from 130f666 to 0024d2e Compare April 27, 2021 07:33
@ahmedetefy ahmedetefy force-pushed the application-mode-session branch from 0024d2e to fe3c0cf Compare April 27, 2021 07:34
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • JS over TS was probably a bad idea; missing linters and type checks.
  • need to be sure process.on('exit', ...) is the right approach
  • we're limited to one test with the current approach

Comment on lines 56 to 60
close() {
return Promise.resolve({
status: 'success',
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong type here, this is the signature of close:

public close(timeout?: number): PromiseLike<boolean>

Copy link
Contributor Author

@ahmedetefy ahmedetefy Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings so far:

  1. process.on('exit') will not function correctly because endSession is asynchronous and according to https://nodejs.org/api/process.html#process_event_exit

Listener functions must only perform synchronous operations. The Node.js process will exit immediately after calling the 'exit' event listeners causing any additional work still queued in the event loop to be abandoned. In the following example, for instance, the timeout will never occur:

Possible Solutions

The 'beforeExit' event is not emitted for conditions causing explicit termination, such as calling process.exit() or uncaught exceptions.

The 'beforeExit' should not be used as an alternative to the 'exit' event unless the intention is to schedule additional work.

https://nodejs.org/api/process.html#process_event_beforeexit

@ahmedetefy ahmedetefy force-pushed the application-mode-session branch 2 times, most recently from 568b4d1 to 38a5058 Compare May 4, 2021 11:31
@ahmedetefy ahmedetefy requested review from rhcarvalho and HazAT and removed request for kamilogorek May 4, 2021 11:39
@ahmedetefy ahmedetefy force-pushed the application-mode-session branch from 38a5058 to 9aacf37 Compare May 4, 2021 14:18
const session = hub.getScope()?.getSession();
const terminalStates = [SessionStatus.Exited, SessionStatus.Crashed];
// Only call endSession, if the Session exists on Scope and SessionStatus is not a
// Terminal Status i.e. Exited or Crashed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's explain why we do this here.
If I understood correctly it's because if a session is in that state we've already sent the session to sentry and we don't want to do it twice.

Add API to capture application mode sessions associated with a specific
release, and send them to Sentry as part of the Release Health functionality.
@ahmedetefy ahmedetefy force-pushed the application-mode-session branch from 9c3b6be to c004c2c Compare May 5, 2021 07:25
@ahmedetefy ahmedetefy enabled auto-merge (squash) May 5, 2021 08:18
@ahmedetefy ahmedetefy disabled auto-merge May 5, 2021 08:19
@ahmedetefy ahmedetefy merged commit cca6fcf into master May 5, 2021
@ahmedetefy ahmedetefy deleted the application-mode-session branch May 5, 2021 08:19
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