Uh oh!
There was an error while loading. Please reload this page.
feat(cloudflare): Instrument sync KV - #21316
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2decb59. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
2decb59 to
091b09bCompare0526d91 to
26e3446Compare| if (prop === 'kv' && original != null && typeof original === 'object') { | ||
| return instrumentDurableObjectSyncKvStorage(original as SyncKvStorage); | ||
| } |
There was a problem hiding this comment.
Bug: Accessing storage.kv repeatedly creates a new proxy object each time instead of reusing a cached one, causing unnecessary performance overhead.
Severity: MEDIUM
Suggested Fix
Use a WeakMap to cache the instrumented proxy for the kv property. On the first access, create the proxy, store it in the map, and return it. On subsequent accesses, return the cached proxy from the WeakMap. This aligns with the existing caching pattern in instrumentEnv.ts and avoids creating multiple proxy objects.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/cloudflare/src/instrumentations/instrumentDurableObjectStorage.ts#L39-L41
Potential issue: The instrumentation for Durable Object storage creates a new proxy for
the `kv` property on every access. In scenarios where code makes multiple sequential
calls like `storage.kv.get(...)` and then `storage.kv.put(...)`, a new proxy object is
unnecessarily created for each call. This leads to increased object allocations and
garbage collection pressure, which can degrade performance in serverless environments.
This behavior is inconsistent with the caching pattern (using `WeakMap`) established in
other parts of the codebase, such as in `instrumentEnv.ts`.
Did we get this right? 👍 / 👎 to inform future reviews.
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
| // reference" errors. | ||
| const original = Reflect.get(target, prop, target); | ||
| if (prop === 'kv' && original != null && typeof original === 'object') { |
There was a problem hiding this comment.
Can this be hardened a bit? typeof object is a bit broad. Maybe you can check for some typical object keys.
Uh oh!
There was an error while loading. Please reload this page.
closes#20830
closes JS-2443
This adds instrumentation for
ctx.storage.kv.*(it only adds a span like with the async KV).Cloudflare itself is having
cloudflare-durable-object-sqlasdb.system.name, that is why we have it here now too.