feat: add cache hit statuses to expr context#2643
Conversation
WalkthroughThis PR extends GraphQL operation observability by introducing five new cache-hit indicator boolean fields to the expression context's Operation struct and propagating cache state from the prehandler and WebSocket layers into the expression context for telemetry and access log evaluation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
📝 Coding Plan
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2643 +/- ##
==========================================
+ Coverage 62.85% 62.88% +0.02%
==========================================
Files 244 244
Lines 25831 25836 +5
==========================================
+ Hits 16237 16246 +9
+ Misses 8259 8255 -4
Partials 1335 1335
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router-tests/structured_logging_test.go (1)
2914-3130: Please add a websocket/subscription case for these new expressions.These assertions only go through the HTTP path via
MakeGraphQLRequest*. The PR also changesrouter/core/websocket.go, and that propagation path is separate enough that it deserves at least one subscription test here—especially forpersistedOperationCacheHitandplanCacheHit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/structured_logging_test.go` around lines 2914 - 3130, Add a websocket/subscription test that mirrors the HTTP tests but uses the WS subscription path so the new propagation in router/core/websocket.go is exercised: create a new t.Run that calls the environment's websocket subscription helper (e.g., xEnv.MakeGraphQLSubscription or xEnv.MakeWebsocketSubscription equivalent), send a persisted operation subscription (and a normal subscription for planCacheHit), then inspect xEnv.Observer().FilterMessage("/graphql") context maps for "persisted_operation_cache_hit" and "plan_cache_hit" (and/or the other cache keys as desired) asserting first request is false (miss) and a subsequent identical subscription is true (hit); reuse the same testenv.Run/testenv.Config setup pattern and Observer assertions used in the existing HTTP tests so the fields are validated over the WS path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/core/websocket.go`:
- Around line 909-925: The websocket expression context is only getting the
three normalization-related cache flags but not PersistedOperationCacheHit and
PlanCacheHit, causing subscriptions to see false even when caches hit; in the
same block where you assign
reqCtx.expressionContext.Request.Operation.NormalizationCacheHit,
VariablesNormalizationCacheHit and VariablesRemappingCacheHit, also set
reqCtx.expressionContext.Request.Operation.PersistedOperationCacheHit =
opContext.persistedOperationCacheHit and
reqCtx.expressionContext.Request.Operation.PlanCacheHit = opContext.planCacheHit
(these names are set earlier in parseAndPlan/operationKit and consulted by
executeSubscription), ensuring all five cache-hit flags mirror into the
websocket expr context.
---
Nitpick comments:
In `@router-tests/structured_logging_test.go`:
- Around line 2914-3130: Add a websocket/subscription test that mirrors the HTTP
tests but uses the WS subscription path so the new propagation in
router/core/websocket.go is exercised: create a new t.Run that calls the
environment's websocket subscription helper (e.g., xEnv.MakeGraphQLSubscription
or xEnv.MakeWebsocketSubscription equivalent), send a persisted operation
subscription (and a normal subscription for planCacheHit), then inspect
xEnv.Observer().FilterMessage("/graphql") context maps for
"persisted_operation_cache_hit" and "plan_cache_hit" (and/or the other cache
keys as desired) asserting first request is false (miss) and a subsequent
identical subscription is true (hit); reuse the same testenv.Run/testenv.Config
setup pattern and Observer assertions used in the existing HTTP tests so the
fields are validated over the WS path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10300af6-3b57-47c1-8cd6-08a79beeee10
📒 Files selected for processing (4)
router-tests/structured_logging_test.gorouter/core/graphql_prehandler.gorouter/core/websocket.gorouter/internal/expr/expr.go
…er-add-cache-metric-hits-to-the-expression-context
…s-to-the-expression-context
This PR adds cache hit statuses to the expr context. Right not they are only available as router response headers, which cannot be added to access logs.
Summary by CodeRabbit
New Features
Tests
Checklist