feat: add new attributes for entity caching metrics#1486
Open
SkArchon wants to merge 14 commits into
Open
Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
dkorittki,
jensneuse and
ysmolski
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a planner flag to force entity key hashing, extends cache-analytics events with timestamps, field-paths, and data-source attribution, updates visitor/schema logic to mark value-type fields reachable from ancestor entities, and threads field-path/data-source through resolvers, loader cache, and tests. ChangesCache analytics + global hashing override
Sequence Diagram(s)sequenceDiagram
participant Planner
participant Visitor
participant Resolver
participant Loader
participant CacheAnalyticsCollector
Planner->>Visitor: read Configuration.ForceHashAnalyticsKeys
Visitor->>Visitor: analyze schema (hasEntityAncestor) / annotate FieldInfo
Visitor-->>Planner: annotated FieldInfo.CacheAnalyticsHash
Resolver->>Resolver: maintain currentEntityFieldPath & currentEntityDataSource
Resolver->>CacheAnalyticsCollector: HashFieldValue(entityType, field, fieldPath, dataSource)
Loader->>CacheAnalyticsCollector: RecordEntitySource(entityType, keyJSON, dataSource, source)
CacheAnalyticsCollector-->>CacheAnalyticsCollector: stamp Timestamp and store FieldPath/DataSource
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Adds a Configuration.ForceHashAnalyticsKeys planner flag. When set, the plan visitor ORs it onto the per-entity HashAnalyticsKeys setting so a true value can never be downgraded by per-entity SDL config. Use case: router operators need a guarantee that no raw entity key ever leaves the engine via analytics (PII / GDPR), regardless of what each subgraph SDL declares.
Adds Timestamp time.Time as the leading field on every analytics event struct (CacheKeyEvent, CacheWriteEvent, FetchTimingEvent, SubgraphErrorEvent, EntityFieldHash, ShadowComparisonEvent, MutationEvent, CacheOperationError, HeaderImpactEvent). Record* methods auto-stamp time.Now() when the field is zero, so callers don't have to remember to set it but may pre-stamp at the actual capture site (e.g. fetch-start time vs record time).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/pkg/engine/resolve/loader_cache.go (2)
2491-2507:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShadow-mode cached hashes still skip nested leaf paths.
This loop only visits
info.ProvidesData.Fieldsand always passesnilforfieldPath. After the newFieldPathsupport, fresh resolution emits hashes for nested leaves likeprofile.location.street, but the shadow-cached side here only records top-level fields or whole nested objects. Those events no longer line up for value-type traversal. Please recurse throughProvidesDataand hash scalar leaves with the accumulated path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/loader_cache.go` around lines 2491 - 2507, The shadow-cache path only iterates info.ProvidesData.Fields and always passes nil for fieldPath, so nested scalar leaves (e.g., profile.location.street) are never hashed; update the loop that visits info.ProvidesData.Fields (in the block using entry.cachedValue, buildEntityKeyJSON, res.cacheConfig.KeyFields) to recursively traverse ProvidesData's field tree, accumulate the FieldPath for each nested leaf, and call l.ctx.cacheAnalytics.HashFieldValue with that accumulated fieldPath (instead of nil) only when encountering scalar/leaf fields; preserve existing args (entityType, fieldName, fieldBytes, keyRaw, 0, FieldSourceShadowCached, dataSource) and reuse the existing buildEntityKeyJSON and cached value access logic.
1521-1526:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive entity-source keys before alias denormalization.
At these call sites,
FromCachemay already have been renamed back to response aliases.buildEntityKeyJSONandwalkCachedResponseForSourcesstill look up schemaKeyFields, so aliased keys likeuserId: idstop matching and noentitySourceRecordgets emitted. That makesEntitySource/EntityDataSourcefall back to defaults on cached alias reads, which undercuts the new datasource attribution. Use the pre-denormalized cache value, or compute the key/source records before denormalizing.Also applies to: 1659-1660
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/loader_cache.go` around lines 1521 - 1526, The entity-source key computation is happening after alias denormalization so buildEntityKeyJSON and walkCachedResponseForSources don't match schema KeyFields (aliased keys), causing missing entitySourceRecord entries; change the two call sites that pass res.l1CacheKeys[i].FromCache (the blocks that append to res.l2EntitySources and where walkCachedResponseForSources is invoked) to use the original pre-denormalized cached value (or compute key/source records before running alias denormalization) so buildEntityKeyJSON and walkCachedResponseForSources see the schema field names and emit entitySourceRecord correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/resolve/cache_analytics_test.go`:
- Around line 1675-1681: The test subcase "empty field path is stored as nil for
direct entity scalars" currently passes nil for the fieldPath and thus doesn't
verify empty-slice normalization; update the call to
CacheAnalyticsCollector.HashFieldValue (in the subtest using
NewCacheAnalyticsCollector, HashFieldValue, Snapshot, FieldHashes, FieldPath) to
pass an actual empty slice (e.g. []string{}) instead of nil so the collector
must canonicalize an empty []string{} to nil and the assert on
snap.FieldHashes[0].FieldPath will properly exercise the new contract.
---
Outside diff comments:
In `@v2/pkg/engine/resolve/loader_cache.go`:
- Around line 2491-2507: The shadow-cache path only iterates
info.ProvidesData.Fields and always passes nil for fieldPath, so nested scalar
leaves (e.g., profile.location.street) are never hashed; update the loop that
visits info.ProvidesData.Fields (in the block using entry.cachedValue,
buildEntityKeyJSON, res.cacheConfig.KeyFields) to recursively traverse
ProvidesData's field tree, accumulate the FieldPath for each nested leaf, and
call l.ctx.cacheAnalytics.HashFieldValue with that accumulated fieldPath
(instead of nil) only when encountering scalar/leaf fields; preserve existing
args (entityType, fieldName, fieldBytes, keyRaw, 0, FieldSourceShadowCached,
dataSource) and reuse the existing buildEntityKeyJSON and cached value access
logic.
- Around line 1521-1526: The entity-source key computation is happening after
alias denormalization so buildEntityKeyJSON and walkCachedResponseForSources
don't match schema KeyFields (aliased keys), causing missing entitySourceRecord
entries; change the two call sites that pass res.l1CacheKeys[i].FromCache (the
blocks that append to res.l2EntitySources and where walkCachedResponseForSources
is invoked) to use the original pre-denormalized cached value (or compute
key/source records before running alias denormalization) so buildEntityKeyJSON
and walkCachedResponseForSources see the schema field names and emit
entitySourceRecord correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4783286c-7b6b-47d7-b853-8ad17773b174
📒 Files selected for processing (8)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/plan/visitor_cache_analytics_test.gov2/pkg/engine/resolve/cache_analytics.gov2/pkg/engine/resolve/cache_analytics_test.gov2/pkg/engine/resolve/loader_cache.gov2/pkg/engine/resolve/resolvable.go
SkArchon
force-pushed
the
milinda/entity-intelligence
branch
from
5cdd3d7 to
1e37134
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/cache_analytics_test.go (1)
1658-1673: 💤 Low valueStatic analysis: ineffectual assignment to
scratch.The
scratch = append(scratch, "more")assignment on line 1668 is flagged byineffassignbecause the result is never used. While the test intent (demonstrating defensive copying) is clear, the append is redundant — only thescratch[0] = "MUTATED"mutation is needed to prove the slice backing array isn't shared with the collector.Consider removing the unnecessary append to silence the linter:
Proposed fix
// Caller mutates the scratch slice after the call. scratch[0] = "MUTATED" - scratch = append(scratch, "more")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/cache_analytics_test.go` around lines 1658 - 1673, The test "field path is copied so caller scratch buffers can be reused" uses a scratch slice to verify defensive copying but contains an unnecessary append whose result is never used; remove the redundant `scratch = append(scratch, "more")` line so only the in-place mutation `scratch[0] = "MUTATED"` remains, leaving the rest of the test (NewCacheAnalyticsCollector, c.HashFieldValue, Snapshot, and assertions on snap.FieldHashes[0].FieldPath) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/plan/visitor.go`:
- Around line 539-577: hasEntityAncestor() currently skips non-object nodes and
continues scanning upward; change it to stop scanning when encountering abstract
types (interfaces or unions) so an earlier entity above a polymorphic boundary
isn't treated as an enclosing entity. In the loop inside hasEntityAncestor(),
when node.Kind is ast.NodeKindInterfaceTypeDefinition or
ast.NodeKindUnionTypeDefinition, treat it as a non-entity stop (break/return
false) instead of continue; keep the existing object-type handling and the
analytics lookup (v.caching.entityCacheAnalytics(typeName)) for object nodes
unchanged.
---
Nitpick comments:
In `@v2/pkg/engine/resolve/cache_analytics_test.go`:
- Around line 1658-1673: The test "field path is copied so caller scratch
buffers can be reused" uses a scratch slice to verify defensive copying but
contains an unnecessary append whose result is never used; remove the redundant
`scratch = append(scratch, "more")` line so only the in-place mutation
`scratch[0] = "MUTATED"` remains, leaving the rest of the test
(NewCacheAnalyticsCollector, c.HashFieldValue, Snapshot, and assertions on
snap.FieldHashes[0].FieldPath) unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bb5142a-59fc-4047-a582-e29a3450eeb0
📒 Files selected for processing (9)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/plan/caching_planner_state.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/plan/visitor_cache_analytics_test.gov2/pkg/engine/resolve/cache_analytics.gov2/pkg/engine/resolve/cache_analytics_test.gov2/pkg/engine/resolve/loader_cache.gov2/pkg/engine/resolve/resolvable.go
✅ Files skipped from review due to trivial changes (2)
- v2/pkg/engine/plan/visitor_cache_analytics_test.go
- v2/pkg/engine/plan/configuration.go
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
- v2/pkg/engine/resolve/resolvable.go
- v2/pkg/engine/resolve/cache_analytics.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/cache_analytics_test.go (1)
1675-1682:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe test passes
nilbut should pass[]string{}to verify normalization.The subtest name indicates "empty field path", but the code passes
nilas thefieldPathargument. This doesn't verify that the collector canonicalizes an actual empty slice[]string{}tonil, which is the contract being tested. Update the call to pass[]string{}instead.🔧 Proposed fix
t.Run("empty field path is stored as nil for direct entity scalars", func(t *testing.T) { c := NewCacheAnalyticsCollector() - c.HashFieldValue("User", "email", nil, + c.HashFieldValue("User", "email", []string{}, []byte(`"a@b"`), `{"id":"1"}`, 0, FieldSourceSubgraph, "") snap := c.Snapshot() require.Equal(t, 1, len(snap.FieldHashes)) assert.Nil(t, snap.FieldHashes[0].FieldPath, "direct entity scalars should record nil FieldPath") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/cache_analytics_test.go` around lines 1675 - 1682, The test currently passes nil for the fieldPath to HashFieldValue, but it should pass an actual empty slice to verify normalization; update the call to c.HashFieldValue in the subtest (using NewCacheAnalyticsCollector, HashFieldValue, Snapshot and inspecting snap.FieldHashes[*].FieldPath) to pass []string{} instead of nil so the collector's canonicalization of an empty slice to nil is tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@v2/pkg/engine/resolve/cache_analytics_test.go`:
- Around line 1675-1682: The test currently passes nil for the fieldPath to
HashFieldValue, but it should pass an actual empty slice to verify
normalization; update the call to c.HashFieldValue in the subtest (using
NewCacheAnalyticsCollector, HashFieldValue, Snapshot and inspecting
snap.FieldHashes[*].FieldPath) to pass []string{} instead of nil so the
collector's canonicalization of an empty slice to nil is tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45bfc773-6491-40e7-9e87-a5b92c660561
📒 Files selected for processing (1)
v2/pkg/engine/resolve/cache_analytics_test.go
Plumbs the owning subgraph name through every analytics event so a single entity instance resolved by multiple subgraphs can be attributed correctly in dashboards (a User id=42 may have N L2 cache entries — one per subgraph that contributes fields — and EntityType alone cannot disambiguate them). - DataSource string field on EntityFieldHash, MutationEvent, entitySourceRecord - HashFieldValue, RecordEntitySource, walkCachedResponseForSources signatures gain a dataSource parameter - New EntityDataSource(entityType, keyJSON) lookup mirrors EntitySource - Resolvable.currentEntityDataSource is saved/restored across nested entities; resolved from the loader-phase entitySources record, falling back to obj.SourceName for cold subgraph fetches that never hit L1/L2
Extends entity field-hash analytics to scalars reached from an entity through pure value-type traversal (e.g. User.address.street where Address has no @key). Hashes attribute to the nearest enclosing entity for cache identity; the new FieldPath dimension disambiguates leaves at arbitrary nesting depth (User.address.street vs User.profile.location.street). - EntityFieldHash gains FieldPath []string (schema names, no array indices) - HashFieldValue takes a fieldPath param and defensively copies the slice so callers can reuse a scratch buffer - plan/visitor.go marks scalars under value-type ancestors via a new hasEntityAncestor() walk over Walker.TypeDefinitions - resolvable.go tracks currentEntityFieldPath as a push/pop stack across walkObject; reset to nil at every entity boundary, restored on exit - renderFieldValue eligibility check trusts the plan-time CacheAnalyticsHash flag and falls back to a polymorphic interface-on-entity match
Review comments addressed:
- hasEntityAncestor stops at Interface/Union ancestors instead of
scanning past, so Entity → Interface → ValueType correctly defers
to the polymorphic runtime fallback.
- Shadow-mode hashing recurses through ProvidesData per scalar leaf
(with accumulated FieldPath) instead of emitting one whole-subobject
hash; nested value-type leaves now line up with the fresh-side
emission for staleness detection. __typename is skipped.
- L2 entity-source records are emitted from the schema-shape cached
value before alias denormalization, so aliased keys like userId: id
no longer drop the entitySourceRecord.
- Empty-slice canonicalization test passes []string{} (verifies the
collector normalizes to nil); the redundant scratch append is gone.
Other fixes:
- HeaderImpactEvent dedup now keys on a Timestamp-less struct; the
auto-stamped wall-clock made identical events look distinct under
whole-struct equality.
- Resolvable suppresses value-type-traversal hashes when the enclosing
entity has no key fields selected (KeyRaw == "" / "{}" with zero
KeyHash) — only affects value-type leaves; direct entity scalars
always hash even with aliased keys.
- Normalize helpers in v2 and execution/engine zero auto-stamped
Timestamps and (in e2e) the DataSource / FieldPath dimensions on
FieldHashes/MutationEvents (covered exhaustively by unit tests
in cache_analytics_test.go).
New tests:
- TestHashShadowCachedLeaves covers nested per-leaf emission, the
__typename skip, and array-of-objects path accumulation.
- E2E fixtures updated for the new value-type traversal hashes
(Product.body via Review.body) and new DataSource attribution.
SkArchon
force-pushed
the
milinda/entity-intelligence
branch
from
1d6d574 to
b0f1376
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
execution/engine/federation_caching_helpers_test.go (1)
1021-1026:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize empty
CacheOpErrorsslices too.This helper now clears
CacheOpErrorstimestamps above, but it still leaves[]versusnildifferences unnormalized here. Snapshots with no cache-op errors can still failassert.Equalfor that reason alone.Suggested fix
if len(snap.MutationEvents) == 0 { snap.MutationEvents = nil } if len(snap.HeaderImpactEvents) == 0 { snap.HeaderImpactEvents = nil } + if len(snap.CacheOpErrors) == 0 { + snap.CacheOpErrors = nil + } return snap }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/federation_caching_helpers_test.go` around lines 1021 - 1026, The helper currently normalizes empty MutationEvents and HeaderImpactEvents to nil but misses doing the same for CacheOpErrors, causing [] vs nil mismatches; update the same normalization block (the code that checks len(snap.MutationEvents) and len(snap.HeaderImpactEvents)) to also check len(snap.CacheOpErrors) and set snap.CacheOpErrors = nil when zero so snapshots with no cache-op errors are normalized for assertion comparisons.
🧹 Nitpick comments (1)
execution/engine/federation_caching_analytics_test.go (1)
662-673: ⚡ Quick winMake
Review.bodyexpectations consistently assertFieldPath.
Review.bodyhere is a value-type traversal underProduct, but unlike Line 69/Line 79, these entries don’t assertFieldPath: []string{"reviews"}. That weakens regression coverage for path attribution in shadow/header scenarios.♻️ Suggested test expectation tightening
- {EntityType: "Product", FieldName: "body", FieldHash: hashReviewBodyTop1, KeyRaw: entityKeyProductTop1, Source: resolve.FieldSourceSubgraph}, - {EntityType: "Product", FieldName: "body", FieldHash: hashReviewBodyTop2, KeyRaw: entityKeyProductTop2, Source: resolve.FieldSourceSubgraph}, + {EntityType: "Product", FieldPath: []string{"reviews"}, FieldName: "body", FieldHash: hashReviewBodyTop1, KeyRaw: entityKeyProductTop1, Source: resolve.FieldSourceSubgraph}, + {EntityType: "Product", FieldPath: []string{"reviews"}, FieldName: "body", FieldHash: hashReviewBodyTop2, KeyRaw: entityKeyProductTop2, Source: resolve.FieldSourceSubgraph},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/federation_caching_analytics_test.go` around lines 662 - 673, The test expectations in the slices fieldHashes and fieldHashesL2 are missing FieldPath assertions for the value-type traversal entries {EntityType: "Product", FieldName: "body", ...} (these represent Review.body reached under Product); update those entries in both fieldHashes and fieldHashesL2 to include FieldPath: []string{"reviews"} so the test asserts the traversal path (look for the fieldHashes and fieldHashesL2 variables and the Product:body entries to modify).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@execution/engine/federation_caching_analytics_test.go`:
- Line 38: CI reports a gci import/formatting failure in this test file; run the
repository formatter/import-sorter (e.g., gci or the project's preconfigured
formatter) on execution/engine/federation_caching_analytics_test.go, fix the
import order/formatting and re-commit the updated file so the gci check passes
(the change will touch the same file containing symbol hashProductNameTrilby).
In `@execution/engine/federation_caching_helpers_test.go`:
- Around line 837-840: The test helper currently clears
snap.FieldHashes[i].DataSource unconditionally (in the loop touching
FieldHashes.Timestamp/DataSource/FieldPath), which hides subgraph attribution
errors; stop blanking FieldHashes.DataSource globally and instead create two
normalizers: a generic normalizer used by most tests that leaves
FieldHashes.DataSource intact, and a stricter attribution normalizer (e.g.,
NormalizeForAttribution or NormalizeSnapshotForAttribution) that explicitly
clears DataSource when an attribution-insensitive comparison is desired; update
federation_caching_source_test.go to call the attribution-specific normalizer
while other tests keep the generic one so the new source-attribution assertions
will fail when the resolver reports the wrong subgraph.
In `@v2/pkg/engine/resolve/cache_load_test.go`:
- Around line 1329-1361: The normalization loop zeroes timestamps for
snap.MutationEvents, snap.HeaderImpactEvents, and snap.CacheOpErrors but does
not collapse empty slices to nil, leaving potential differences between nil and
[] in asserts; update the helper so after zeroing timestamps it sets each of
snap.MutationEvents, snap.HeaderImpactEvents, and snap.CacheOpErrors to nil when
len(...) == 0 (same approach used for the other event slices) to ensure
consistent nil-versus-empty-slice normalization.
In `@v2/pkg/engine/resolve/loader_cache.go`:
- Around line 2568-2650: The shadow-side hashes lose correct KeyRaw when
entry.cachedValue has already been alias-denormalized; capture the entity key
JSON from the schema-shaped cached value before denormalization and use that
preserved value for shadow hashes. Add a pre-denorm key field to the shadow
cache entry (e.g., shadowCacheEntry.preKeyRaw or similar) when the entry is
created (populate it using buildEntityKeyJSON against the original schema-shaped
cachedValue), change calls that invoke Loader.hashShadowCachedLeaves to
pass/consume that preserved key instead of the current keyRaw derived from
denormalized cachedValue, and update all HashFieldValue calls inside
hashShadowCachedLeaves to use the preserved pre-denorm key for
FieldSourceShadowCached emissions.
---
Outside diff comments:
In `@execution/engine/federation_caching_helpers_test.go`:
- Around line 1021-1026: The helper currently normalizes empty MutationEvents
and HeaderImpactEvents to nil but misses doing the same for CacheOpErrors,
causing [] vs nil mismatches; update the same normalization block (the code that
checks len(snap.MutationEvents) and len(snap.HeaderImpactEvents)) to also check
len(snap.CacheOpErrors) and set snap.CacheOpErrors = nil when zero so snapshots
with no cache-op errors are normalized for assertion comparisons.
---
Nitpick comments:
In `@execution/engine/federation_caching_analytics_test.go`:
- Around line 662-673: The test expectations in the slices fieldHashes and
fieldHashesL2 are missing FieldPath assertions for the value-type traversal
entries {EntityType: "Product", FieldName: "body", ...} (these represent
Review.body reached under Product); update those entries in both fieldHashes and
fieldHashesL2 to include FieldPath: []string{"reviews"} so the test asserts the
traversal path (look for the fieldHashes and fieldHashesL2 variables and the
Product:body entries to modify).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2457d9ba-e5e7-4142-be0d-54ea356f5190
📒 Files selected for processing (11)
execution/engine/federation_caching_analytics_test.goexecution/engine/federation_caching_helpers_test.goexecution/engine/federation_caching_source_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/resolve/cache_analytics.gov2/pkg/engine/resolve/cache_analytics_test.gov2/pkg/engine/resolve/cache_load_test.gov2/pkg/engine/resolve/extensions_cache_invalidation_test.gov2/pkg/engine/resolve/loader_cache.gov2/pkg/engine/resolve/resolvable.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/resolve/cache_analytics_test.go
- v2/pkg/engine/resolve/cache_analytics.go
- Fix gci formatting on federation_caching_analytics_test.go (column alignment after adding wider uint64 const entries). - Normalize empty MutationEvents/HeaderImpactEvents/CacheOpErrors slices to nil in normalizeCacheAnalyticsSnapshot so assert.Equal stops failing on []-vs-nil for snapshots without those events. - Split the e2e helper into normalizeSnapshot (generic; strips DataSource/FieldPath from FieldHashes/MutationEvents) and normalizeSnapshotPreservingAttribution (strict; keeps subgraph attribution intact). Source-attribution tests use the strict variant so a regression that misattributes a hash now fails the assertion. - TestCacheWriteEventSource_MutationL2Write now uses the strict variant and asserts DataSource="reviews" on the User.username FieldHash — User attribution flows through reviews (authorWithoutProvides) before accounts joins username.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
execution/engine/federation_caching_helpers_test.go (2)
1009-1040:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize empty
CacheOpErrorstoniltoo.This helper now canonicalizes the other event slices, but
CacheOpErrorsis still left out. After unmarshalling, an emptycache_op_errorsarray will stay[], so equality checks can still fail on[]vsnilin no-error cases.Suggested fix
if len(snap.HeaderImpactEvents) == 0 { snap.HeaderImpactEvents = nil } + if len(snap.CacheOpErrors) == 0 { + snap.CacheOpErrors = nil + } return snap }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/federation_caching_helpers_test.go` around lines 1009 - 1040, The test helper currently canonicalizes many empty event slices but omits CacheOpErrors; add the same normalization for snap.CacheOpErrors by checking if len(snap.CacheOpErrors) == 0 and setting snap.CacheOpErrors = nil (matching the pattern used for L1Reads, L2Reads, EntityTypes, FieldHashes, ErrorEvents, ShadowComparisons, MutationEvents, HeaderImpactEvents) so unmarshalled empty arrays compare equal to nil.
938-987:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComplete the strict-mode sort keys.
normalizeSnapshotPreservingAttributionkeeps attribution onFieldHashesandMutationEvents, but these comparators still ignore that preserved data. If two events only differ byDataSource/FieldPath, they still compare equal here, so attribution-focused snapshot tests can remain order-sensitive and flaky.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/federation_caching_helpers_test.go` around lines 938 - 987, The sort comparators inside normalizeSnapshotPreservingAttribution are ignoring attribution fields so tests can be flaky; update the FieldHashes comparator (snap.FieldHashes / resolve.EntityFieldHash) to additionally compare DataSource and FieldPath (after existing EntityType, FieldName, KeyRaw, KeyHash and before FieldHash) and update the MutationEvents comparator (snap.MutationEvents / resolve.MutationEvent) to include DataSource and FieldPath in the comparison keys (insert them into the tie-breaker chain so two events that differ only by attribution no longer compare equal).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@execution/engine/federation_caching_helpers_test.go`:
- Around line 1009-1040: The test helper currently canonicalizes many empty
event slices but omits CacheOpErrors; add the same normalization for
snap.CacheOpErrors by checking if len(snap.CacheOpErrors) == 0 and setting
snap.CacheOpErrors = nil (matching the pattern used for L1Reads, L2Reads,
EntityTypes, FieldHashes, ErrorEvents, ShadowComparisons, MutationEvents,
HeaderImpactEvents) so unmarshalled empty arrays compare equal to nil.
- Around line 938-987: The sort comparators inside
normalizeSnapshotPreservingAttribution are ignoring attribution fields so tests
can be flaky; update the FieldHashes comparator (snap.FieldHashes /
resolve.EntityFieldHash) to additionally compare DataSource and FieldPath (after
existing EntityType, FieldName, KeyRaw, KeyHash and before FieldHash) and update
the MutationEvents comparator (snap.MutationEvents / resolve.MutationEvent) to
include DataSource and FieldPath in the comparison keys (insert them into the
tie-breaker chain so two events that differ only by attribution no longer
compare equal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 186a6305-51c7-4660-ab2d-e75befb6565a
📒 Files selected for processing (4)
execution/engine/federation_caching_analytics_test.goexecution/engine/federation_caching_helpers_test.goexecution/engine/federation_caching_source_test.gov2/pkg/engine/resolve/cache_load_test.go
Shadow-mode FieldSourceShadowCached hashes were re-extracting the entity key from entry.cachedValue inside compareShadowValues, but that value has already been alias-denormalized at the L2 hit site. For aliased @key fields (e.g. `userId: id`), buildEntityKeyJSON couldn't find the schema names and the emitted hashes lost their entity-key correlation — the same aliasing problem the L2 entitySourceRecord path was already fixed for. Capture the keyRaw from the schema-shape value before denormalization and persist it on shadowCacheEntry so compareShadowValues uses the correct key without re-extracting from the denormalized value. The preDenormKeyJSON computation gate is widened from analyticsEnabled-only to (analyticsEnabled || shadowMode) since shadow-mode now relies on it.
…s normalization normalizeSnapshotPreservingAttribution preserves DataSource on FieldHashes / MutationEvents and FieldPath on FieldHashes, but the sort comparators didn't include these as tiebreakers, so attribution-distinct events were order-equivalent and could flake the assertion. Extend the comparators: - FieldHashes: append FieldPath then DataSource as tiebreakers - MutationEvents: append DataSource as tiebreaker Also normalize empty CacheOpErrors slices to nil — the existing block canonicalized every other event slice but missed this one, so snapshots with no cache-op errors could fail [] vs nil equality. Adds a small compareStringSlice helper for the FieldPath tiebreaker.
jensneuse
reviewed
May 6, 2026
| // will attribute the hash to currentEntityTypeName (the nearest | ||
| // enclosing entity preserved by walkObject's save/restore) with | ||
| // ExactParentTypeName recorded separately for disambiguation. | ||
| fieldInfo.CacheAnalyticsHash = true |
Member
There was a problem hiding this comment.
let's have a more in depth discussion on this
…ilinda/entity-intelligence
…y and emit field-hash hits
Root-field caches that delegate via EntityKeyMappings (e.g. @openfed__queryCache on
Query.employee → Employee) now record analytics under the underlying entity type
instead of the operation root ("Query"). Hub / Studio per-entity dashboards group
cache events by EntityType, so tagging Query.employee L2 reads as "Query" left the
per-Employee panels empty even when the cache served every request.
Also synthesize per-(typeName,fieldName) field-hash events at every root-field L2
hit site so the hub heatmap's per-field cache-hit tint lights up for Query.<f> /
Mutation.<f>. The response walk emits FieldHashes for leaves under an entity
scope, but root fields have no enclosing entity and were therefore invisible.
Events are accumulated per-result and merged on the main thread, mirroring the
existing L2 key-event pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y scope currentEntityDataSource is the entry-point subgraph for the entity scope, not the subgraph that resolved any given leaf. For @Shareable / overridden fields that's wrong — the directive surfaces in downstream analytics under the entity owner instead of the field resolver. Introduce fieldDataSource() which prefers FieldInfo.Source.Names[0] (planner's preferred resolver) and falls back to the entity scope when per-field info is missing (root-field walks, shadow-cached recursion). Wire into renderFieldValue (fresh emission) and hashShadowCachedLeaves (shadow-cached recursion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y scope
Today field_hash events emit one row per resolved scalar leaf. For
`Employee { details { forename } }` the engine produces a single row
attributed to Employee with FieldName=forename and FieldPath=[details] —
the accessor itself is never emitted. Downstream coverage analytics
cannot distinguish "operator selected the accessor" from "only leaves
are visible," so for a family-scoped recommendation on Employee, the
analytics either over-reports (Details leaves attributed to Employee
inflate observed-field counts beyond the subgraph's declared set) or
under-reports (declared accessor `details` shows as never read).
Add a new EntityFieldSelection event and a RecordFieldSelection method
on CacheAnalyticsCollector. The walker emits one event per Object/Array
accessor entered inside an entity scope BEFORE descending — so the
parent-entity attribution stays correct when crossing into a nested
entity. Same gating as HashFieldValue (isOnCurrentEntity check with
polymorphic ParentTypeNames fallback, hasEntityIdentity guard) plus a
new CachingOptions.EmitFieldSelections sub-flag (default false) so the
emission stays opt-in until consumers are ready to read the new rows.
ChildTypeName carries FieldInfo.NamedType — the unwrapped accessor
return type. For list accessors that's the item type (Hobby for
[Hobby!]!); for interface/union accessors it's the abstract type name
(concrete __typename info already lives on the leaf rows under it).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds new attributes for entity caching metrics. Namely
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.