feat(open-next): create cloudflare-sentry-tail package#8842
feat(open-next): create cloudflare-sentry-tail package#8842
Conversation
Creates a `cloudflare-sentry-tail` package that enables us to add a tail worker to the open next deployment of the site. This package should be publishable as well so that we can reuse it in the release worker. Signed-off-by: flakey5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Integrates this tail handler into the site’s Cloudflare Reviewed by Cursor Bugbot for commit c0468e2. Bugbot is set up for automated code reviews on this repo. Configure here. |
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/web-infra @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8842 +/- ##
==========================================
- Coverage 73.87% 73.82% -0.05%
==========================================
Files 105 105
Lines 8883 8883
Branches 326 327 +1
==========================================
- Hits 6562 6558 -4
- Misses 2320 2324 +4
Partials 1 1 ☔ View full report in Codecov by Sentry. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Signed-off-by: flakey5 <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c0468e2. Configure here.
| // Allocate space for the elements we're gonna add | ||
| sentryEvent.breadcrumbs.length += | ||
| item.logs.length + | ||
| item.diagnosticsChannelEvents.length + | ||
| item.exceptions.length; |
There was a problem hiding this comment.
Arrays in JavaScript automatically expand as you add elements to them, see:
So I don't think we need to allocate memory for the new elements here 🤔
There was a problem hiding this comment.
Unnecessary yes, however, preallocating makes it so it's only one memory allocation instead of multiple so it's faster and more efficient:
(noteworthy that this is in Node, but ofc both runtimes are v8 based, also benchmark src)
There was a problem hiding this comment.
I see, thanks that's quite interesting 😄
However for a tail worker I don't imagine that having a better performance for the tail worker would be in any way helpful/useful, would it? 🤔
(also I am curious about the number of such events that you'd expect, obviously the less the number the less the gain here)
PS: I'm just asking out of curiosity since these lines seem to introduce unnecessary (even if very minimal of course) complexity for no real gain. But of course they don't cause any harm if left in.
| @@ -0,0 +1,31 @@ | |||
| { | |||
| "name": "@node-core/cloudflare-sentry-tail", | |||
There was a problem hiding this comment.
Can you open an issue in admin for this package?
3 participants
Creates a
cloudflare-sentry-tailpackage that enables us to add a tail worker to the open next deployment of the site.This package should be publishable as well so that we can reuse it in the release worker.
Description
Validation
Related Issues
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.