feat(trace): cover compact and response-cache hits - #301
OnlineChef (ChefGroep) wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCache entries now preserve the ID of the request that populated them. Cache-hit traces record the cache-hit flag, response hash and byte count, and source trace ID when available, without storing the cached response body. Compact Responses requests now run within a trace context. ChangesCache-hit tracing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ResponseCacheMiddleware
participant ResponseCache
participant Server
participant TraceCapture
ResponseCacheMiddleware->>ResponseCache: get cached entry
ResponseCache-->>ResponseCacheMiddleware: return body and sourceTraceId
ResponseCacheMiddleware-->>Server: return CacheHit
Server->>TraceCapture: begin trace for cached request
Server->>TraceCapture: noteTraceCacheHit with body and sourceTraceId
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the change is mergeable after normal checks and the stated trace-store parent dependency is satisfied. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to This expands optional capture of potentially sensitive traffic while retaining existing authentication and caller-isolation checks. No introduced security defect was established, but deployed access and retention assumptions are not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
Summary
Extends the opt-in trace store from #298 to the two remaining HTTP/cache paths that materially affect cache debugging:
/v1/responses/compactThis PR is intentionally stacked on
feat/trace-store/ #298. It does not add WebSocket/live/realtime tracing, a remote reader, Redis, or a new persistence format.Cache-hit trace model
A response-cache entry now keeps the request id that originally populated it. On a later HIT the new request trace records:
cacheHit: truecacheSourceTraceIdwhen knownThe cached response body is not copied again into trace.sqlite. In
redacted/fullmode only the hit request's own inbound body may be stored under its new trace id.The persisted cache source id is allowlisted before it can enter usage/request-log metadata.
Compact coverage
/v1/responses/compactnow uses the same trace lifecycle as the other HTTP data-plane routes:beginTrace -> runWithTrace -> response observation -> addFinalRequestLogThe compact implementation already has a 32 MiB hard response-buffer bound; the trace store still applies its own lower configurable per-body cap.
Verification
Exact clean-tree head
84f105ba56240f31ca57f8e348bcc7290c6cc7bd:bun x tsc --noEmit✅bun run privacy:scan✅git diff --check✅client-artifactbuild/guard tests ✅The earlier dirty-tree full-suite attempt hit only the repository's intentional client-artifact clean-tree guard before this lane was committed. The clean exact-head rerun passed that guard and the complete suite.
Remaining boundary
WebSocket/live/realtime traffic is intentionally not included here. That lifecycle needs per-session/frame bounds and separate finalization semantics rather than being bolted onto the HTTP trace path.
Dependency
Stacked on #298. Merge/rebase only after the trace-store parent lands.
Summary by CodeRabbit
PR-stack (mergen van onder naar boven; elke PR bouwt op de vorige):
Base van deze PR = de branch van de PR erboven in de lijst (#296 =
main).