feat(node-core,node,bun): Export processSessionIntegration from node-core and add it to bun #18852
Conversation
…core and add it to bun
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
We might need to re-export it manually from some meta framework packages if the consistent exports e2e test fails
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| contextLinesIntegration(), | ||
| nodeContextIntegration(), | ||
| modulesIntegration(), | ||
| processSessionIntegration(), |
There was a problem hiding this comment.
Missing tests for new Bun integration feature
Low Severity · Bugbot Rules
This is a feat PR that adds processSessionIntegration to Bun's default integrations, but doesn't include any integration or E2E tests. Per the review rules file, feat PRs are expected to include at least one integration or E2E test. The existing test file packages/bun/test/init.test.ts tests integration setup patterns and could be extended to verify ProcessSession is included in the default integrations when initializing the Bun SDK.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Nice, thank you!
I tested this locally and Bun now shows failure rates and crash free sessions 🎉
…s comes out to two, locally to one...
size-limit report 📦
|
| contextLinesIntegration(), | ||
| nodeContextIntegration(), | ||
| modulesIntegration(), | ||
| processSessionIntegration(), |
There was a problem hiding this comment.
Bug: The processSessionIntegration is added to Bun's default integrations but will not work with Bun.serve() because the beforeExit event it relies on never fires.
Severity: HIGH
Suggested Fix
Conditionally exclude processSessionIntegration from the default integrations when the SDK is used in a Bun environment, or implement an alternative mechanism to end sessions that is compatible with Bun's lifecycle, such as using the bunServerIntegration to hook into the server's shutdown process.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/bun/src/sdk.ts#L51
Potential issue: The `processSessionIntegration` is being added to the default
integrations for the Bun SDK. This integration relies on the Node.js
`process.on('beforeExit')` event to correctly terminate sessions. However, in Bun
environments using `Bun.serve()`, the `beforeExit` event is never fired. As a result,
for any Bun application utilizing the standard web server, Sentry sessions will not be
properly closed, leading to inaccurate session health and release health metrics. The
integration lacks a fallback mechanism for environments where `beforeExit` is not
supported.
Did we get this right? 👍 / 👎 to inform future reviews.
Closes: #18516