fix(core): Add a PromiseBuffer for incoming events on the client #18120
Conversation
JPeer264
force-pushed
the
jp/memory-leak
branch
2 times, most recently
from
619a016 to
1695dd4
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| processEvent(event: Event): Event | null | PromiseLike<Event | null> { | ||
| return new Promise(resolve => setTimeout(() => resolve(event), 1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Async Test Integration Missing Event Processor Setup
The AsyncTestIntegration defines a processEvent method but lacks a setupOnce or setup method to register it as an event processor. Without registration, the async processEvent won't execute, causing tests using this integration to pass incorrectly without actually exercising the promise buffer's async event handling logic.
JPeer264
force-pushed
the
jp/memory-leak
branch
from
1695dd4 to
2ee14b4
Compare
size-limit report 📦
|
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.
|
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
To me this feels a bit weird because it means we have two promise buffers in our pipeline, one to manage client processing state, and the other to manage the transport queue state.
The transport still needs a promise buffer to manage inflight requests because we need to make sure we don't saturate the user's network I/O.
I understand conceptually this is the best way to do it so giving it the ✅ (unless we totally overhaul _process, which honestly we might want to as a follow up).
We'll need the transport buffer size to be >= client buffer size, otherwise we risk backpressure issues. I think it's reasonable to assume the time it takes promises to resolve on the client buffer will be shorter than the transport buffer, so we could even make this size 32 instead of 64, or pick Math.ceil(transportOptions.bufferSize / 2)
It would be nice to just do a sanity check benchmark of the client buffer with a basic test app that we blast with load - we can use that to see if the buffer size assumptions hold up.
To be fair I see this Promise buffer not as final solution, but more of mitigating the main issue. Overall there are quite some memory increases once we run into sync code
So once we have code like above only 64 requests are taken and the rest are abandoned and removed so only 64 are going through - always, since the requests will be queued and not really processed further (unless we remove the async integrations). So in theory the second promise buffer doesn't do anything anymore in this scenario (that's just a guess at this point). |
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
So in theory the second promise buffer doesn't do anything anymore in this scenario (that's just a guess at this point).
the 2nd promise buffer tracks I/O instead of a transformed event, so I think it still matters.
Thinking about this more, the 2nd promise buffer probably resolves promises slower too, as it's based on request promises. It becomes the throughput bottleneck. If we size the first promise buffer to be too large, we will always drop events no matter what. So the size of the first promise buffer must be smaller than the second one.
Let's get this merged in and keep iterating.
JPeer264
force-pushed
the
jp/memory-leak
branch
from
9a0c936 to
91f264f
Compare
| client.captureException(new Error('third')); | ||
|
|
||
| expect(client._clearOutcomes()).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Bug: Test expects wrong outcome for sync integrations
The test expects no dropped events when calling captureException three times with a buffer size of 1, but the promise buffer doesn't distinguish between sync and async integrations. With three synchronous calls and a buffer size of 1, the first call adds a promise to the buffer, and the second and third calls are rejected immediately because the buffer is full. The test should expect [{ reason: 'queue_overflow', category: 'error', quantity: 2 }] instead of an empty array.
| this._process( | ||
| () => promisedEvent.then(event => this._captureEvent(event, hintWithEventId, currentScope)), | ||
| isMessage ? 'unknown' : 'error', | ||
| ); |
There was a problem hiding this comment.
Bug: Promise created eagerly in captureMessage
In captureMessage, the promisedEvent is created outside the task producer function passed to _process. This means eventFromMessage or eventFromException is called immediately, even when the promise buffer is full. This defeats the lazy evaluation design of the promise buffer, causing unnecessary work when events should be dropped. The promise creation should be moved inside the task producer function to enable proper lazy evaluation.
JPeer264
force-pushed
the
jp/memory-leak
branch
from
91f264f to
63b4be9
Compare
Problem
Previously, the client would process all incoming events without any limit, which could lead to unbounded growth of pending events/promises in memory. This could cause performance issues and memory pressure in high-throughput scenarios. This occurs when two conditions are met:
processEventare added (e.g.ContextLines, which is a defaultIntegration)Sentry.captureException, are called synchronouslySolution
This PR adds a
PromiseBufferto theClientclass to limit the number of concurrent event processing operations._promiseBufferin theClientclass that limits concurrent event processingDEFAULT_TRANSPORT_BUFFER_SIZE(64) but can be configured viatransportOptions.bufferSizequeue_overflowreason_process()method to:Special 👀 on
transportOptions.bufferSize: Not sure if this is the best technique, but IMO both should have the same size - because if it wouldn't it would be capped at a later stage (asking myself if the transport still needs the promise buffer - as we have it now way earlier in place)_processtakes now aDataCategory. At the time of the process the event type is almost unknown. Not sure if I assumed the categories correctly there, or if there is another technique of getting the type (edit: a comment by Cursor helped a little and I added a helper function)recordDroppedEventis now printing it one after each other - theoretically we can count all occurences and print the count on it. I decided against this one, since it would delay the user feedback - this can be challenged though