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 11 commits into
Conversation
926afe8 to
04d255e
Compare
|
bugbot run |
size-limit report 📦
|
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
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.
There was a problem hiding this comment.
I think the serialization of requests (see: packages/cloudflare/src/client.ts line 149) is adding to this. 256 might be fine, but we might find that less is needed once that's addressed.
7119f01 to
bc89835
Compare
bc89835 to
3216594
Compare
3216594 to
f92b7ba
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>
ed8d910 to
51d50d8
Compare
|
👋 @mydea, @nicohrubec — Please review this PR when you get a chance! |
|
👋 @logaretm, @andreiborza, @getsentry/team-javascript-sdks, @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
|
👋 @mydea, @nicohrubec — Please review this PR when you get a chance! |
|
👋 @logaretm, @andreiborza, @getsentry/team-javascript-sdks, @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
isaacs
left a comment
There was a problem hiding this comment.
This is a pretty big set of changes. I haven't checked the stated perf gains (some kind of benchmark would maybe be good for that?) but the correctness wins in fixing the reported issues would make it worthwhile even if it's a slight perf regression, imo, so as long as it's not making things worse, that's fine.
Re @andreiborza's comment, it is definitely a lot of extra code. The cacheClient: false escape hatch guarantees both delivery pathways live in the tree indefinitely: the flush lock, the span-tracking promise machinery, dispose(), and their tests all stay, and now a second set maintained alongside them. Nothing exercises the false path, so that can potentially rot.
We can delete a lot of the new machinery, though. Some parts are redundant or collapse if the eager drains are gated on flushPointReached the same way span delivery already is. That is imo the highest-value simplification.
Maybe we could either drop the cacheClient: false option now, or file the v12 removal issue and link it from the option's JSDoc so the dual pathway has an expiration date.
We could also push the concept into core, not Cloudflare. The issue is that Client has no notion of an invocation lifetime when it outlives a single request. Vercel Edge and Deno Deploy need the same thing, so a core "delivery scope" could replace InvocationState, the symbol-on-scope trick, and the narrow flushTraceSpans hook. That might be too big for now, but could be a good follow-up PR to simplify things further.
| tunnel: 'http://localhost:3031/', | ||
| // Do not cache this client, as locally there is only one instance | ||
| // And when this handler is getting cached, then the workflow will reuse this very client | ||
| cacheClient: false, |
There was a problem hiding this comment.
I'm a little confused by this.
It looks like packages/cloudflare/src/workflows.ts is the only isolation-scope entry point in the package that wasn't given a setInvocationState call or changed to withInvocationIsolationScope.
The consequence would be:
getInvocationState()returnsundefined, soflush()never setsflushPointReached, so eager span delivery never fires._registerWithInvocationWaitUntil(inclient.ts) falls through tothis._invocationContext, which is whatever the lastinit()in the isolate set. If there's concurrency, then that's the wrongwaitUntil.workflows.ts:234passesenableDedupe: false, which the cached branch discards entirely. Then the workflow gets a dedupe-enabled client.
If I'm reading this right, then it seems like eager delivery never fires there and enableDedupe: false is discarded. And if so, this line in the test would be covering up the problem. Either we should wire workflows into the invocation-state mechanism, or make initWithDefaultIntegrations refuse the cache when the caller asks for options the cached client can't support.
|
|
||
| const client = initAndBind(CloudflareClient, clientOptions) as CloudflareClient; | ||
|
|
||
| if (cacheEnabled && client && options.dsn) { |
There was a problem hiding this comment.
If we don't have a DSN, then it looks like this will leak a client and a global console.log instrumentation.
Because we set cacheClient unconditionally on line 120, if options.dsn is falsey here (eg, preview deploys or just a missing env), then:
- builds a fresh client (nothing is cached),
- gets
isCachedClient === true, sopackages/cloudflare/src/flush.tsline 146 skipsdispose(), - and
dispose()is what runs the cleanup callbacks registered byconsoleIntegration(packages/core/src/integrations/console.tsline 56), which pushes into the module-local handler list inpackages/core/src/instrument/console.tsline 28.
I think we can fix it by either removing the options.dsn check here, or making line 120 be cacheClient: cacheEnabled && !!options.dsn, so that it's setting based on whether the client will actually be cached here.
| const cacheEnabled = options.cacheClient !== false; | ||
|
|
||
| if (cacheEnabled) { | ||
| const cached = getCachedClient(); | ||
| if (cached?.getTransport()) { | ||
| getCurrentScope().update(options.initialScope); | ||
| setCurrentClient(cached); | ||
| cached.setExecutionContext(options.ctx); | ||
| return cached; | ||
| } | ||
| } |
There was a problem hiding this comment.
A later init() with no DSN (or a different DSN) is silently ignored here. The cache enabled branch returns before any DSN comparison. A worker that deliberately disables Sentry for one route/binding (optionsCallback returning no dsn) keeps reporting through the first DSN. A multi-tenant worker with a per-tenant DSN sends every tenant's events to whichever tenant hit the isolate first.
That could be fine, but I think at least we should document it on the option, and emit a DEBUG_BUILD warning if a later init() passes in a different DSN.
| expect(result).toBe('timeout'); | ||
| }); | ||
|
|
||
| it('cacheClient: true - dedupe drops the same error across invocations', async ({ signal }) => { |
There was a problem hiding this comment.
Do we actually want to dedupe errors across invocations?
Two different users hitting the same bug in the same isolate now yield one event, which changes event counts, times_seen, and anything downstream that reasons about volume. dedupeIntegration seems like it's more for one runtime's duplicate reporting of one error, not for cross-request deduping.
If that is the goal, then it probably belongs in the changelog as a behavior change.
| // flush. RPC sub-invocations in Durable Objects never reach a flush point, | ||
| // so their spans batch one envelope per trace here. | ||
| if (!invocationState?.flushPointReached) { | ||
| return; |
There was a problem hiding this comment.
I think we can simplify a few things by gating on flushPointReached. We're doing it here for spans, but maybe we ought to do it for envelopes and logs/metrics as well, in the function below this one.
Before the flush point, nothing needs eager delivery: flush() emits 'flush', which drains the log and metric buffers (packages/core/src/client.ts line 160) and then drains the transport (packages/core/src/client.ts line 475).
If we gated both on flushPointReached, it would:
- restore in-invocation batching (one log envelope per invocation, not one per microtask), fixing the issue with log and metric envelope counts, described below on line 330.
- make the serialized
eagerFlushPromisechain (lines 249-260) and theflush()await of it (lines 160-162) unnecessary, because there are no in-flight eager drains at the boundary. - make
_INTERNAL_flushLogsBufferand_INTERNAL_flushMetricsBufferinflush()redundant
There was a problem hiding this comment.
Oh, caveat to this, though, it would require that packages/cloudflare/src/workflows.ts be updated to call setInvocationState, or else it'll never set flushPointReached, and we'd lose logs and metrics from workflows. But if it did call that, then this becomes
| /** Returns the one cached Cloudflare client for this isolate. */ | ||
| export function getCachedClient(): CloudflareClient | undefined { | ||
| return (GLOBAL_OBJ as GlobalWithCloudflareClient)[GLOBAL_CLIENT_KEY]; | ||
| } |
There was a problem hiding this comment.
The PR description says "A cached client whose transport is gone is evicted rather than returned." It is not evicted; packages/cloudflare/src/baseSdk.ts line 99 just falls through, and the stale reference is only overwritten if the new init has a DSN.
Combined with the DSN issue in the comment on packages/cloudflare/src/baseSdk.ts line 120, a disposed client can stay pinned on GLOBAL_OBJ for the isolate's life.
I'd suggest evicting as the PR description says if the transport is gone.
| * The flush point is per invocation. In Durable Objects it lands at RPC-method | ||
| * settle, so RPC spans (which end before it) keep batching one envelope per | ||
| * trace — flushing them per call would turn a fan-out trace into one envelope | ||
| * per RPC — while detached continuations inheriting that invocation's state | ||
| * flush eagerly. |
There was a problem hiding this comment.
I think the parenthetical here is backwards. RPC spans end after the flush point, so the outcome the comment rules out is the one that happens.
Ordering in wrapMethodWithSentry: onFulfilled runs inside the startSpan callback (packages/cloudflare/src/wrapMethodWithSentry.ts:208-213), and it calls waitUntil?.(teardown()) (wrapMethodWithSentry.ts line 149).
Evaluating that argument runs teardown() synchronously into flushAndDispose (packages/cloudflare/src/flush.ts line 140) and then into CloudflareClient.flush(), whose first statement sets flushPointReached = true (lines 137-140). Only after onFulfilled returns, then startSpan ends the span. So the span's afterSpanEnd fires with flushPointReached already true, and the eager per-trace flush runs.
Because SpanBuffer.flush(traceId) deletes the bucket (packages/core/src/tracing/spans/spanBuffer.ts line 185), the next RPC call starts a fresh bucket and flushes it again. With RPC trace propagation on, that is N envelopes for one trace, precisely the fragmentation the comment says the design avoids.
Note also that this comment contradicts the one below it, on lines 288-290. "RPC sub-invocations in Durable Objects never reach a flush point". They do, DurableObjectState.waitUntil exists
(@cloudflare/workers-types, DurableObjectState), so waitUntil?.(teardown()) is not short-circuited.
I don't see how this would result in data loss, but it will be more envelopes and more subrequests per DO fan-out, and a comment that contradicts the code. If we want the batching property the flush point must be set after the span ends, not before, for example by moving the flushPointReached write out of flush() and into the wrappers, after their span has ended.
| .unordered() | ||
| .start(signal); | ||
|
|
||
| await runner.makeRequest('get', '/burst?n=1&id=fanout'); |
There was a problem hiding this comment.
I think n should be higher here, or else we're not really "bursting" per se, right? It seems like the purpose here is to verify that they all get into the same envelope, but that's trivially true if there's only one span.
| // writes the propagation context to the *current* scope, so the forked | ||
| // isolation scope's propagation context carries a different trace id and | ||
| // flushing by it silently no-ops. | ||
| const traceId = span.spanContext().traceId; |
There was a problem hiding this comment.
minor/nit: we just calculated the traceId a few lines up, on 295 and again on 298. Slightly more efficient to stash it in a variable at the top and then use that each time.
| } | ||
|
|
||
| const DEFAULT_TRANSPORT_BUFFER_SIZE = 30; | ||
| const DEFAULT_TRANSPORT_BUFFER_SIZE = 256; |
There was a problem hiding this comment.
I think the serialization of requests (see: packages/cloudflare/src/client.ts line 149) is adding to this. 256 might be fine, but we might find that less is needed once that's addressed.
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.