sentry-javascript
sentry-javascript copied to clipboard
getsentry
ref(node): Improve span flushing
Follow up to https://github.com/getsentry/sentry-javascript/pull/16416, slightly moving things around and making some small improvements to performance and logic (some not related to that PR but another improvement we noticed when BYK looked int this):
- Actually debounce the flushing properly, including a maxWait of 100ms. Previously, it was technically possible to debounce flushing forever, if a span was flushed every ms. Now, at least after 100ms it should always flush.
- Simplify overhead by avoid checking for the 5min timeout of sent spans. As this check
isSpanAlreadySenthas been called for every single span that ended, it is quite high impact, and meant a little bit of additional work (although it should not necessarily run too often, but still). Instead, we now simply check formap.has(id)which should be good enough for what we want to achieve, IMHO. We still clean up the sent spans when flushing, so old stuff should still go away eventually. - Made stuff that is not needed as public API private on the span exporter class. this is technically breaking but I think it is OK, this should not be public API surface as it does not need to be called from outside.
For this I moved the already existing debounce function from replay to core. We re-implement this in replay with a different setTimeout impl. which is needed for some angular stuff.
size-limit report 📦
| Path | Size | % Change | Change |
|---|---|---|---|
| @sentry/browser | 23.99 kB | - | - |
| @sentry/browser - with treeshaking flags | 23.76 kB | - | - |
| @sentry/browser (incl. Tracing) | 38.79 kB | - | - |
| @sentry/browser (incl. Tracing, Replay) | 76.92 kB | +0.04% | +28 B 🔺 |
| @sentry/browser (incl. Tracing, Replay) - with treeshaking flags | 70 kB | +0.05% | +30 B 🔺 |
| @sentry/browser (incl. Tracing, Replay with Canvas) | 81.68 kB | +0.04% | +28 B 🔺 |
| @sentry/browser (incl. Tracing, Replay, Feedback) | 93.75 kB | +0.03% | +28 B 🔺 |
| @sentry/browser (incl. Feedback) | 40.73 kB | - | - |
| @sentry/browser (incl. sendFeedback) | 28.7 kB | - | - |
| @sentry/browser (incl. FeedbackAsync) | 33.59 kB | - | - |
| @sentry/react | 25.76 kB | - | - |
| @sentry/react (incl. Tracing) | 40.78 kB | - | - |
| @sentry/vue | 28.36 kB | - | - |
| @sentry/vue (incl. Tracing) | 40.66 kB | - | - |
| @sentry/svelte | 24.01 kB | - | - |
| CDN Bundle | 25.48 kB | - | - |
| CDN Bundle (incl. Tracing) | 38.96 kB | - | - |
| CDN Bundle (incl. Tracing, Replay) | 74.81 kB | +0.05% | +33 B 🔺 |
| CDN Bundle (incl. Tracing, Replay, Feedback) | 80.23 kB | +0.05% | +34 B 🔺 |
| CDN Bundle - uncompressed | 74.48 kB | - | - |
| CDN Bundle (incl. Tracing) - uncompressed | 115.3 kB | - | - |
| CDN Bundle (incl. Tracing, Replay) - uncompressed | 229.35 kB | +0.04% | +84 B 🔺 |
| CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 242.18 kB | +0.04% | +84 B 🔺 |
| @sentry/nextjs (client) | 42.44 kB | - | - |
| @sentry/sveltekit (client) | 39.28 kB | - | - |
| @sentry/node | 150.76 kB | +0.08% | +109 B 🔺 |
| @sentry/node - without tracing | 98.52 kB | +0.12% | +113 B 🔺 |
| @sentry/aws-serverless | 124.27 kB | +0.1% | +112 B 🔺 |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I added some more code comments based on @BYK's comments, to make it (hopefully) easier to follow what the exporter does! 🙏
It was actually a great pointer from @BYK that we should really actually profile/measure if the for loop is faster than the flatMap, instead of just assuming so. I ran a simple benchmark here: https://jsbench.me/84mc07ydz3/1
I re-ran it a few times, the difference is not huge and it also changed from time to time which was faster. So overall likely not worth the change!