feat(cloudflare): Add cacheClient to reuse the client across invocations - #23151
feat(cloudflare): Add cacheClient to reuse the client across invocations#23151JPeer264 wants to merge 10 commits into
Conversation
926afe8 to
04d255e
Compare
|
bugbot run |
size-limit report 📦
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
d081c44 to
a362f65
Compare
|
bugbot run |
8869481 to
0ba7c00
Compare
Adds tests to check if `enableDedupes` is really disabled for workflows original trigger: #23151 (comment) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
0ba7c00 to
7a1cdd3
Compare
|
bugbot run |
|
bugbot approve |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 71b3f5f. Configure here.
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
This seems like a lot of extra, hard-to-maintain code that's working around the way client flushing works in our SDKs. Are you sure the perf gains are worth it? I'm a bit concerned, but your call.
| * scope, which is shared by every invocation in the isolate. | ||
| */ | ||
| export function setInvocationState(scope: Scope, state: InvocationState): void { | ||
| (scope as ScopeWithInvocationState)[INVOCATION_STATE] = state; |
There was a problem hiding this comment.
m: Let's add a check here to only set this if scope !== getDefaultIsolationScope(), wdyt?
| options: CloudflareOptions, | ||
| getDefaultIntegrationsImpl: (options: CloudflareOptions) => Integration[], | ||
| ): CloudflareClient | undefined { | ||
| const cacheEnabled = options.cacheClient !== false && Boolean(options.dsn); |
There was a problem hiding this comment.
Uff that was a left over from before where I cached multiple clients per isolate. This should be removed
|
|
||
| // If no more pending spans, resolve the completion promise | ||
| if (this._pendingSpans.size === 0 && this._resolveSpanCompletion) { | ||
| DEBUG_BUILD && debug.log('[CloudflareClient] All spans completed, resolving promise'); |
There was a problem hiding this comment.
l: This doesn't seem to add much value to the user, what promise is resolving? I think we prob don't need to log here, wdyt?
There was a problem hiding this comment.
Before, per client we needed a way to know WHEN to flush, as there is no timer. The only way to know when to flush is when potentially no spans are open anymore (this was super hacky, but worked somehow - but ofc not for all usecases - this is why we don't rely on it anymore).
There was a problem hiding this comment.
Sure, but I mean the debug log itself. What would I do with this as a user? 🤔
There was a problem hiding this comment.
Ah true - I can refine that a little. I change it to
| DEBUG_BUILD && debug.log('[CloudflareClient] All spans completed, resolving promise'); | |
| DEBUG_BUILD && debug.log('[CloudflareClient] All spans completed, preparing to flush'); |
| } | ||
|
|
||
| const DEFAULT_TRANSPORT_BUFFER_SIZE = 30; | ||
| const DEFAULT_TRANSPORT_BUFFER_SIZE = 256; |
There was a problem hiding this comment.
Before we had 1 transport per client = 1 client per request - ergo buffer size of 30 per request. Now we have 1 client for the entire lifetime, which mean more requests per client. Therefore the buffer needs to go up. 256 was a number I set based on the load tests I ran. We can also set it to 100 to mirror the promisbuffer.ts
There was a problem hiding this comment.
Hm no it's fine, but maybe a comment explaining how we got to this number would help us in the future.
It is not just because of the performance gains - but it was indeed one of the motivations (I had this in my head for couple of months already). The main trigger was actually #22545 which is impossible with client disposals, as we would need to keep the client open longer in order to retrieve potential events, which might or might not arrive. This is then a clear OOM case as we only have 128MB on workers |
07655c6 to
2781cf5
Compare
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/in/1153895?s=60&v=4){kind=small}
7119f01 to
bc89835
Compare
bc89835 to
3216594
Compare
Building and disposing a client per invocation costs real time on every request, and in a Durable Object it also loses data: there is no `waitUntil` boundary that dependably extends execution, so anything captured after the handler returned went to a client that had already been disposed. Enabled by default, this caches one client per isolate. The first initialization wins for the isolate's lifetime: a later init with different options reuses that client, and a new deployment always starts fresh isolates, so clients are always built from the current version's options. A cached client is flushed but not disposed at an invocation boundary, and it is re-bound to the current scope on every invocation — otherwise `initialScope` would apply only to an isolate's first invocation, and a client disposed by a competing init would keep being handed out. A cached client whose transport is gone is evicted rather than returned. Because a reused client never reaches an end-of-invocation flush, delivery is eager: the new `afterEnvelope` hook on the core client drains the transport buffer as soon as an envelope has been accepted, and logs and metrics drain on a debounced hook so they are batched rather than sent one at a time. Spans that end after the invocation's flush point are delivered through core's `flushTraceSpans` hook, which flushes only that trace's bucket from the span streaming buffer. The per-invocation flush lock and span tracking are skipped, since binding a client that outlives the invocation to one invocation's lock would make later flushes wait on that invocation's work forever. A shared client also shares integration state, so dedupe works across invocations: the same error raised by two separate requests is reported only once. Uncached behavior is unchanged; pass `cacheClient: false` to restore it. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
3216594 to
f92b7ba
Compare
2 participants
closes #23083
closes #22545
closes #21950
What
This PR is reusing the client, instead of creating a new one. This is also only possible because of #22969 (as now we have the correct isolation scopes per request).
To still have an escape hatch and keep the old behavior there is the
cacheClient: falseoption, that just creates a new client per request, as before.Why
There are more and more issues coming in, that
.disposeis leading to errors, which makes sense as in DurableObjects data can flow in after a request happened and it stays alive. Since we created a new client on each request, we also had to clean it up - the best point in time was after a request, which was too early for e.g. #22545. Since there is not a perfect time to dispose the client the only option is to reuse the client and not dispose at all (this is then also aligned with how other SDK machinery works).Issues and how they're solved
Timing
In CF, timers are usually 0 and therefore our 5 second auto flush wouldn't work. In order to still retrieve all the data we need to have point in times (hooks) where it is safe to flush. In our case we have a request and flush after a request, like before. Events that come in a later point in time are then captured with the hooks added in #23136 (most important one is the
afterEnvelope).We start listening to the hooks once
flushPointReachedis set totrue- which is AFTER a request, the time where we have no control anymore about flushing manually otherwise.waitUntil
Keeping the correct
waitUntilis important, as each request needs its ownwaitUntilto properly flush. To still keep the correctwaitUntilthis is now bound onto thescopedirectly. I tried usingsetSDKProcessingMetadataon the scope, but it just didn't work properly on deployed workers. Instead this is bound onto the scope directly with a Symbol - that works like a charm. This is theINVOCATION_STATE#namingishardflushLock
The flush lock would wait for all spans to be finished and then flush. This would just not work, as we only have one client. So we skip this entirely and get rid of that hack. We keep this in order to have the escape hatch
cacheClient: falsein case something goes sideways.Bonuses
Bonus 1
Because we are now reusing the client we are saving valuable CPU cycles per request. With
cacheClient: truewe gain up to ~14-21% per request, which is loads. Also on top of the CPU wins we also retrieve more events, which would have been dropped before.Bonus 2
In v12 (or any other major) we could get rid of all the
flushLockhacks and the rest of hacks we didBonus 3
Dedupe integration works now as intended. Because we created a new client and the
dedupeIntegrationonly deduplicated per integration, which was a new one on every client, we only deduped it per request, not for all requests.Sidenotes
During the implementation I thought about having multiple clients, which are cached in one global map - in case the isolations would get reused from other deployments or other bindings. After some excessive tests it seems that new deployments are getting a fresh isolate, different bindings have their own isolate, only
ExportedHandlers andWorkerEntrypoints share one isolate, which makes sense to some degree, as they're isolated within each request anyways (they're getting a fresh isolate on a new deployment though). Because this is the case only one client is being created instead of checking if the config differs between clients.Clanker description
Building and disposing a client per invocation costs real time on every request, and in a Durable Object it also loses data: there is no
waitUntilboundary that dependably extends execution, so anything captured after the handler returned went to a client that had already been disposed.Enabled by default, this caches one client per isolate. The first initialization wins for the isolate's lifetime: a later init with different options reuses that client, and a new deployment always starts fresh isolates, so clients are always built from the
current version's options. A cached client is flushed but not disposed at an invocation boundary, and it is re-bound to the current scope on every invocation — otherwise
initialScopewould apply only to an isolate's first invocation, and a client disposed by a competing init would keep being handed out. A cached client whose transport is gone is evicted rather than returned.Because a reused client never reaches an end-of-invocation flush, delivery is eager: the new
afterEnvelopehook on the core client drains the transport buffer as soon as an envelope has been accepted, and logs and metrics drain on a debounced hook so they are batched rather than sent one at a time. Spans that end after the invocation's flush point are delivered through core'sflushTraceSpanshook, which flushes only that trace's bucket from the span streaming buffer. The per-invocation flush lock and span tracking are skipped, since binding a client that outlives the invocation to one invocation's lock would make later flushes wait on that invocation's work forever.A shared client also shares integration state, so dedupe works across invocations: the same error raised by two separate requests is reported only once.