Skip to content

feat(node): Application mode sessions - #3423

Merged
ahmedetefy merged 1 commit into
masterfrom
application-mode-session
May 5, 2021
Merged

feat(node): Application mode sessions#3423
ahmedetefy merged 1 commit into
masterfrom
application-mode-session

Conversation

@ahmedetefy

@ahmedetefy ahmedetefy commented Apr 20, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread packages/node/src/client.ts Outdated
@github-actions

github-actions Bot commented Apr 20, 2021

Copy link
Copy Markdown
Contributor

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

@rhcarvalho rhcarvalho left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread packages/node/package.json Outdated
Comment thread packages/node/test/manual/release-health/application-mode-session.js Outdated
Comment thread packages/node/test/manual/release-health/application-mode-session.js Outdated
@ahmedetefy
ahmedetefy force-pushed the application-mode-session branch 5 times, most recently from 388220d to f5c89f6 Compare April 26, 2021 11:33
Comment thread packages/node/test/manual/release-health/application-mode-session.js Outdated
Comment thread packages/node/test/manual/release-health/application-mode-session.js Outdated
Comment thread packages/node/test/manual/release-health/application-mode-session.js Outdated
@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

@rhcarvalho rhcarvalho left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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>

@ahmedetefy ahmedetefy Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

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 HazAT and rhcarvalho 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
Comment thread packages/node/src/sdk.ts Outdated
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
Copy Markdown
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