-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
022a598
to
cb82655
Compare
size-limit report
|
c8a4edb
to
fe53418
Compare
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.
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).
2cdd29f
to
812f317
Compare
packages/node/test/manual/release-health/application-mode-session.js
Outdated
Show resolved
Hide resolved
packages/node/test/manual/release-health/application-mode-session.js
Outdated
Show resolved
Hide resolved
388220d
to
f5c89f6
Compare
packages/node/test/manual/release-health/application-mode-session.js
Outdated
Show resolved
Hide resolved
packages/node/test/manual/release-health/application-mode-session.js
Outdated
Show resolved
Hide resolved
packages/node/test/manual/release-health/application-mode-session.js
Outdated
Show resolved
Hide resolved
f5c89f6
to
9ad7925
Compare
130f666
to
0024d2e
Compare
0024d2e
to
fe3c0cf
Compare
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.
- 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
close() { | ||
return Promise.resolve({ | ||
status: 'success', | ||
}); | ||
} |
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.
Wrong type here, this is the signature of close
:
public close(timeout?: number): PromiseLike<boolean>
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.
Findings so far:
process.on('exit')
will not function correctly becauseendSession
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
- https://stackoverflow.com/questions/40574218/how-to-perform-an-async-operation-on-exit
- Or using
beforeExit
event, howeverbeforeExit
has certain limitations
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
568b4d1
to
38a5058
Compare
38a5058
to
9aacf37
Compare
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 |
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.
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.
9c3b6be
to
c004c2c
Compare
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.
Sentry.init
we start a sessionautoSessionTracking
is now a global option in both@sentry/browser
&@sentry/node
and istrue
by defaultrelease
we do not send any sessionscrashed
if an error/promise was captured with the global handlers andhandled: false
errored
if an error/promise was captured withhandled: true
fix(node): Sets mechanism handled to false when it is a crash #3493beforeExit
we capture it -> sending it to SentrySee getsentry/develop#323 for more info