feat(attribution): index facts in memory and publish them per type - #286
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds collection-delete attribution facts with selectors and UID coverage, introduces a bounded in-memory fact index with asynchronous stream following and waiter coordination, and batches webhook-derived facts by stream for publication with new telemetry and tests. ChangesAttribution fact pipeline
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant AuditHandler
participant AuthorFactFromEvent
participant AuditFactPublisher
participant FactIndex
participant WatchResolver
AuditHandler->>AuthorFactFromEvent: convert accepted audit events
AuthorFactFromEvent-->>AuditHandler: return grouped author facts
AuditHandler->>AuditFactPublisher: publish facts by stream
AuditFactPublisher->>FactIndex: deliver fact entries
WatchResolver->>FactIndex: await attribution query
FactIndex-->>WatchResolver: return tiered attribution result
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The layer that sits above the fact transport seam and never learns which transport it has: the four match structures the join reads, the reference-counted subscription set, the waiter registry a blocked resolver parks on, and the follower loop that fills all three. The route leads every key. The index is one per process while the streams are one per (route, group/resource), so an index keyed on the type alone would pool two clusters' facts in one map and hand a watch event on cluster B an author from cluster A. The rv-only tier is where that bites hardest, because a resourceVersion is opaque and not unique across clusters, and the collection tier is where it bites most quietly, because a namespace name says nothing about which cluster it is in. Entries apply in delivery order, because the latest tier is last-writer-wins. Each carries its insertion time, so expiry is decided on read and the sweep is memory reclaim rather than correctness. Each type is capped, with oldest-first eviction that is COUNTED: an attribution dropped because the index was full has to look different in the metrics from one that was never published. The waiter registry registers THEN checks, never the reverse. A fact applied in the gap between the two must signal a waiter that is already listening; registering after the check loses exactly that fact, which is the race the poll loop papers over by looking again. AuthorFact gains the collection fields the index reads — the selector the request URI expressed, and the optional uid set — so this layer is complete on its own. Nothing above it changes: the resolver still polls, the audit handler is untouched, and the v1 fact keys are still the path in use. Numbers picked here: 4096 facts per type as the primary cap, 65536 in total, a 30s collection window, and a 30s sweep. All configurable on FactIndexConfig.
The audit receiver stops writing per event and accumulates instead: each accepted event is reduced to an AuthorFact, the facts are grouped by (audit route, group/resource) across the whole request, and each group is appended once. An apiserver batch of 400 events over three types becomes three appends. The batching that causes the delivery delay attribution waits out is the same batching that makes publishing nearly free. A deletecollection is published as ONE fact describing the collection rather than expanded into one fact per object, so the "no resolvable name" rule becomes "no name and not a collection verb". The fact carries what the audit event actually said — actor, verb, type, namespace, stage timestamp, the selector from the request URI — plus the uids the response body listed, reduced at the receiver and dropped past a cap with the degradation counted. The selector is the better evidence: it is the intent the actor expressed, and it is there even when a truncating production cluster sends no body at all. Only facts that would have been stored are published. An event with no objectRef or no user produces nothing, or waiters wake for facts that can name nobody. The v1 keys are still written and still read. The resolver is not switched over here, so behaviour is unchanged and e2e attribution keeps passing on the path it always used. The uid-set cap is 10000 — a few hundred kilobytes of uids against a response body for the same request that runs to tens of megabytes.
89551ca to
4079d5c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/queue/fact_index_store.go (2)
317-347: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
compactis O(refs × collections) while holding the index mutex.
livescanss.collectionslinearly for every collection ref, andcompactcallslivefor every ref inorder.SweepholdsFactIndex.mufor the whole call, so a scope near the per-type cap makes every lookup on the watch shard's blocking path wait on a quadratic scan. Building the live collection seqs once per compact makes it linear.♻️ Proposed fix: hoist the collection seq set out of the per-ref scan
func (s *scopeFacts) compact() { + collectionSeqs := make(map[uint64]struct{}, len(s.collections)) + for _, entry := range s.collections { + collectionSeqs[entry.seq] = struct{}{} + } kept := s.order[:0] for _, ref := range s.order { - if s.live(ref) { + if s.liveWith(ref, collectionSeqs) { kept = append(kept, ref) } } s.order = kept }with
liveWithtaking the precomputed set for thefactKindCollectioncase and delegating the map lookups for the other three kinds.🤖 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 `@internal/queue/fact_index_store.go` around lines 317 - 347, Update scopeFacts.compact to build a set of live collection sequence numbers once before iterating over s.order, then use a liveWith helper that accepts this precomputed set for factKindCollection while preserving the existing map-based checks for the other reference kinds. Replace the per-reference live call in compact with liveWith so compaction remains linear in the number of references and collections.
83-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
invalidSelectorhas no test.The branch decides that a collection fact is never scope-matched — an over-attribution guard — and nothing in
fact_index_test.gopublishes a fact with an unparseableLabelSelector. A case assertingAttributionAbsentfor a fact carrying e.g."!!!"would pin the behaviour.As per coding guidelines, "Cover new code with positive and negative tests".
🤖 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 `@internal/queue/fact_index_store.go` around lines 83 - 90, Add a negative test in fact_index_test.go that publishes a collection fact with an unparseable LabelSelector such as "!!!", then assert its attribution result is AttributionAbsent. Exercise the invalidSelector branch in the fact indexing flow and preserve existing valid-selector behavior.Source: Coding guidelines
internal/webhook/audit_fact_publish_test.go (1)
49-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for
FactRecorder+FactPublisherconfigured together.The
FactPublisherdoc comment states it is "additive toFactRecorder" — both are expected to run side by side in production during this transition period. Every test here wires only one or the other (newPublishingHandlersets onlyFactPublisher;TestAuditHandler_NoPublisherPublishesNothingsets onlyFactRecorder), so there's no coverage confirming both fire correctly for the same event batch.♻️ Suggested additional test
func TestAuditHandler_RecorderAndPublisherBothFire(t *testing.T) { recorder := &fakeFactRecorder{} publisher := &fakeFactPublisher{} handler, err := NewAuditHandler(AuditHandlerConfig{FactRecorder: recorder, FactPublisher: publisher}) require.NoError(t, err) body := eventListBody(writeEvent("a", "apps", "deployments", "web", "alice")) require.Equal(t, http.StatusOK, serveBody(t, handler, http.MethodPost, "/audit-webhook/prod-eu-1", body).Code) require.Equal(t, 1, recorder.len()) require.Len(t, publisher.recorded(), 1) }🤖 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 `@internal/webhook/audit_fact_publish_test.go` around lines 49 - 54, Add a test alongside the existing audit handler tests that configures both FactRecorder and FactPublisher in AuditHandlerConfig, sends one valid event batch through the handler, and verifies a successful response plus exactly one recorded event in each fake. Keep the existing single-component tests 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 `@internal/queue/fact_index_store.go`:
- Around line 100-102: Correct the comment for parseStageTimestamp to state that
falling back to the later insertion time widens the window because inWindow
compares against at.Add(window), rather than claiming it narrows the window.
---
Nitpick comments:
In `@internal/queue/fact_index_store.go`:
- Around line 317-347: Update scopeFacts.compact to build a set of live
collection sequence numbers once before iterating over s.order, then use a
liveWith helper that accepts this precomputed set for factKindCollection while
preserving the existing map-based checks for the other reference kinds. Replace
the per-reference live call in compact with liveWith so compaction remains
linear in the number of references and collections.
- Around line 83-90: Add a negative test in fact_index_test.go that publishes a
collection fact with an unparseable LabelSelector such as "!!!", then assert its
attribution result is AttributionAbsent. Exercise the invalidSelector branch in
the fact indexing flow and preserve existing valid-selector behavior.
In `@internal/webhook/audit_fact_publish_test.go`:
- Around line 49-54: Add a test alongside the existing audit handler tests that
configures both FactRecorder and FactPublisher in AuditHandlerConfig, sends one
valid event batch through the handler, and verifies a successful response plus
exactly one recorded event in each fake. Keep the existing single-component
tests 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a6c4a0c-d936-4547-a691-80936926b4dd
📒 Files selected for processing (12)
.coverage-baselineinternal/queue/attribution_index.gointernal/queue/author_fact.gointernal/queue/author_fact_test.gointernal/queue/fact_index.gointernal/queue/fact_index_store.gointernal/queue/fact_index_test.gointernal/queue/fact_streams.gointernal/queue/fact_waiters.gointernal/telemetry/exporter.gointernal/webhook/audit_fact_publish_test.gointernal/webhook/audit_handler.go
The recreate-on-deletion spec waits for a recovery only the periodic requeue can deliver — this controller runs no control-plane Secret watch — and its budget was exactly two RequeueStreamSettleInterval ticks with no slack. A CI runner busy enough to delay one tick by a second failed the spec on timing alone, twice in a row, on a branch whose diff cannot reach the age-key path. Eventually returns as soon as the Secret is back, so the third tick costs nothing on any run that was going to pass.
1 participant
Stacked on #284 (the transport seam). Base is
feat/attribution-fact-stream, so this PR carries only its own diff.This is PR 2 and PR 3 of the attribution fact stream design together: the transport-agnostic layer above the seam, and the publish side that feeds it. Nothing reads from it yet — the resolver still polls the v1 keys, so behaviour is unchanged and e2e attribution keeps passing on the path it always used.
The transport-agnostic layer
internal/queue/fact_index.goand friends: the four match structures, the reference-counted subscription set, the waiter registry, and the follower loop. Everything here has exactly one implementation and never learns which transport it has.The route leads every key. The index is one per process while the streams are one per
(route, group/resource), so an index keyed on the type alone would pool two clusters' facts in one map and hand a watch event on cluster B an author from cluster A. The rv-only tier is where that bites hardest — a resourceVersion is opaque and not unique across clusters — and the collection tier is where it bites most quietly, because a namespace name says nothing about which cluster it is in.TestFactIndex_RouteIsolatesOtherwiseIdenticalFactsstores identical(group/resource, uid, rv)facts under two routes and resolves each from its own, through both the exact and the rv-only tier.Entries apply in delivery order, because the latest tier is last-writer-wins. Each carries its insertion time, and expiry is decided on READ as well as swept: an aged-out fact is never joined merely because the sweep has not run.
Eviction is counted, never silently absorbed. An attribution dropped because the index was full has to look different in the metrics from one that was never published.
The waiter registry registers THEN checks. A fact applied in the gap between the two must signal a waiter that is already listening; registering after the check loses exactly that fact, which is the race the poll loop papers over by looking again. The signal is buffered, so a waiter that was not listening yet still finds it.
The follower loop applies entries, wakes waiters, and reports trim gaps as a counter plus a log line naming the stream. A transport error is retried rather than returned: a follower that gave up would leave attribution silently dead for the life of the process.
AttributionResolutionsTotalandAttributionResolutionWaitSecondskeep their names and meanings — the wait histogram is how this whole change gets measured. New ininternal/telemetry:attribution_fact_index_evictions_total{reason},attribution_fact_stream_gaps_total{stream},attribution_collection_degraded_total{reason}. They are registered here and first emitted when the index is wired in the switch-over PR, which is also where they get their row indocs/interpreting-metrics.md.FactStreamSetis exposed fortarget_watch.goto drive in a later PR; nothing acquires a reference here.The publish side
internal/webhook/audit_handler.goaccumulates rather than writing per event: each accepted event is reduced to anAuthorFact, the facts are grouped by(route, group/resource)across the whole request, and each group is appended once. A batch of 400 events over three types becomes three appends.A
deletecollectionis published as one fact describing the collection rather than expanded into one fact per object, so the "no resolvable name" rule becomes "no name and not a collection verb". The fact carries the actor, verb, type, namespace, stage timestamp, the selector from the request URI, and the uids the response body listed — reduced at the receiver, dropped past a cap with the degradation counted. The selector is the better evidence of the two: it is the intent the actor expressed, and it is there even when a cluster with--audit-webhook-truncate-enabledsends no body at all.Only facts that would have been stored are published. An event with no
objectRefor no user produces nothing, or waiters wake for facts that can name nobody.Deliberately not in this PR
RecordFact, the fact key builders,RecordDeleteCollectionFacts, and their tests are untouched, andauthor_resolver.gois not touched at all. The v1 keys are still written and still read. The switch-over and the deletions are the next PR; doing them here would produce a diff that cannot be validated in halves.The three open numbers
The design leaves three tuning numbers open. All are fields on
FactIndexConfig/ aqueueconstant, defaulted here; the flag wiring lands with the switch-over, whencmd/main.gofirst constructs an index (a flag whose consumer does not exist yet is a flag nobody can test).(route, group/resource)Validation
task fmt,task generate,task manifests,task vet,task lint,task test(coverage 78.3% → 78.4%, baseline committed),task test-e2e.Summary by CodeRabbit
New Features
Monitoring