Record http.url on cache spans for string and URL keys - #209
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughCache instrumentation now extracts URLs from string keys, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
petyosi
left a comment
There was a problem hiding this comment.
Reviewed the full diff and relevant surrounding code. No significant correctness, failure-mode, compatibility, or test-coverage issues found.
7f5ab2a to
1c19539
Compare
|
Rebased onto current main (79bcac0) and re-verified there. Approved and clean, no conflicts. Recap: Cache methods accept Happy to close it if you would rather not take this one. |
The Cache methods accept
RequestInfo | URL, so the key can be aRequest, a bare string, or aURL.instrumentFunctioninpackages/otel-cf-workers/src/instrumentation/cache.tsread the url asargArray[0].url, which only exists on aRequest, socaches.default.match('https://example.com/x')and theURLform both produced a cache span with nohttp.urlat all. Two of the three documented input shapes lost the attribute.This pulls the url out of whichever shape was passed. I also made
sanitiseURLreturn undefined instead of throwing on something unparseable, because it runs while building the span attributes, before the span exists, so a throw there would have surfaced inside the caller's own cache call rather than as a failed span.There was no test file for the cache bindings, so I added one covering all three key shapes plus an unparseable one. The string and URL cases fail on current main with
expected undefined to be 'https://example.com/...'. Ran the repo preflight andpnpm run check, both green. Patch changeset for@pydantic/otel-cf-workersincluded.quick disclosure: worked through this with Claude Code and read the final diff myself. Freshman still learning, so if you would rather keep the attribute Request-only I am happy to drop it :)