Repository navigation
feat(cloudflare): instrument Workers KV bindings - #25488
faizansaiyed123 wants to merge 10 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
bf253b0 to
c23d2c8
Compare
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Thanks a lot for creating the PR. The code looks great overall. Just found couple of small things - once they are addressed we can ship it
| expect(spans).toEqual( | ||
| expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| attributes: expect.objectContaining({ |
There was a problem hiding this comment.
It'd be nice if you could be more explicit about what attributes are in there. Our /write-tests skill can help out here in case you have an agent.
| }) | ||
| .start(signal); | ||
|
|
||
| await runner.makeRequest('get', '/kv/list'); |
There was a problem hiding this comment.
Could you add a test where you call at least 2 endpoints? Not that something is off, but would be nice to just double check that two requests are for sure sent in separate spans
| MY_KV: KVNamespace; | ||
| } | ||
|
|
||
| export default Sentry.withSentry( |
There was a problem hiding this comment.
Optional, since we haven't migrated most of it yet, but I would suggest to rewrite it so Vite is being used. For that you only need to create a vite.config.mts and a instrument.server.ts - there should be other integration tests lying around that have the same pattern.
| const client = getClient(); | ||
|
|
||
| return { | ||
| name: client && hasSpanStreamingEnabled(client) ? cacheOperation : keys?.join(', ') || operation, |
There was a problem hiding this comment.
Here we don't need to check for streamed spans anymore (we just had to add this for instrumentations that were there before we added span streaming)
| name: client && hasSpanStreamingEnabled(client) ? cacheOperation : keys?.join(', ') || operation, | |
| name: cacheOperation, |
|
bugbot run |
|
This PR is too large for Bugbot to review. It changes 86,197 lines and 3,466,497 characters. Split the change into smaller pull requests to get a review. |
be68e22 to
3bdf95a
Compare
e5147de to
f89897e
Compare
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 f89897e. Configure here.
|
Hi @JPeer264, I've updated the branch with the latest develop changes and added the KV test coverage you requested. The GitHub Actions workflows are still awaiting maintainer approval, so the updated CI hasn't run yet. Could you approve them when you get a chance? Thanks! |
|
👋 @mydea — Please review this PR when you get a chance! |
This branch has not been deployed
2 participants
Fixes #24037
Implements the Workers KV portion of #24037.
isKVNamespacedetection usingget,put,list, andgetWithMetadata, excluding JSRPC proxiesget,getWithMetadata,put,delete, andlistVectorize and Analytics Engine remain separate follow-ups.