refactor(targeting): remove RecordExposure path#68
refactor(targeting): remove RecordExposure path#68ohalushchak-exadel wants to merge 1 commit intoohalushchak-exadel/evaluate-identity-qfrom
Conversation
Drops ExposeRequest/ExposeResponse types, Engine.RecordExposure, and the sorted-set Store methods (ZAdd/ZCount/ZExpire/ZRemRangeByScore) that were only used by the write path. Exposure eligibility still reads from the binary exposure log via EvaluateIdentityResolved. Also removes the ExposureRecorded metric and adapts affected tests to seed exposures directly through Store.SetUserExposures / AddExposure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
|
||
| // ExposureEntry represents a single ad impression recorded against a user. | ||
| type ExposureEntry struct { | ||
| ImpressionID string `json:"id"` |
There was a problem hiding this comment.
Is this now universally, a universally unique impression ID across sellers? This is not the direct source provided impression ID.
| type ExposureEntry struct { | ||
| ImpressionID string `json:"id"` | ||
| PackageID string `json:"pkg"` | ||
| AdvertiserID string `json:"adv,omitempty"` |
There was a problem hiding this comment.
How does the edge map advertiser campaign creative down to what's coming in from the campaign, I mean, from the upstream source? Because the upstream source won't know what any of these things are. We have to speak in terms of eligible packages, right?
| Exists(ctx context.Context, key string) (bool, error) | ||
|
|
||
| // ZAdd adds a member with the given score to a sorted set. | ||
| ZAdd(ctx context.Context, key string, score float64, member string) error |
There was a problem hiding this comment.
Don't we want to store all the exposure data in a sorted set that automatically expires after 30 days? That way we don't have to look through the whole set, right?
To do a merge or whatever else, we can just increment until we find what we need. I guess it's a question of, where's this dedupe happen? In the set.
And now that you've split out the identities from impressions, I guess we're saying we have three totally separate sets now with three separate identities. I think we still want that sorted. Well, I guess it depends if we want to try to dedupe before it comes back to us?
bokelley
left a comment
There was a problem hiding this comment.
Broader technical read on top of the three inline questions above. Requesting changes because a couple of these look blocking to me.
Blocking
The write path is gone, but the read path stays. EvaluateIdentityResolved in targeting/engine.go still reads user:exposures:<hash>, but after this PR nothing in production writes that key — only MockStore.SetUserExposures / MockStore.AddExposure do. We'd be merging a read-only system in main. If the intent is that exposure writes move elsewhere, let's land that elsewhere first (or in the same PR) so we aren't shipping a broken flow.
Pruning is silently dropped. The old RecordExposure called TruncateBinaryLog + applied the 30-day cutoff on every write. The new MockStore.AddExposure calls MergeBinaryLogs with no cutoff, no truncation, and no TTL on store.Set. The one test that asserted old entries were pruned (TestSystem_RollingWindowExpiry) was deleted and replaced by one that drops that assertion. Unbounded growth in the binary log is now unverified — this ties directly to my Q3 above about where dedupe/expiry should live.
Undeclared breaking change to the on-disk format. exposure_binary.go bumps binary version v2 → v3 and ValidateBinaryLog now rejects v2 with ErrBinaryUnknownVersion. No migration, not mentioned in the PR body. If anyone has persisted v2 keys in a deployed Valkey, they'll start erroring on read. Either add a v2 reader for one release, or call this out explicitly and gate behind a config switch.
Observability regression. Metrics.ExposureRecorded and targeting_exposure_recorded_total are removed with no replacement counter. Even if exposure writes are relocating, a counter for exposure-volume should land alongside — losing it across the refactor makes any incident during the transition much harder to triage.
Worth addressing
- Scope exceeds the title. This PR also renames
PackageID→CreativeIDacrossExposureEntry+ the binary format, addsAdvertiserID/CreativeIDtoPackageIdentityConfig, and bumps the binary version. None of that is hinted at in the title or body — each of those is a standalone decision worth splitting out or at least calling out in the description. (Relates to my Q2: the newCreativeIDfield on the config still has no resolution logic showing how upstream package IDs map to it.) - Silent skip of frequency rules.
engine.goappliesidCfg.FrequencyRulesonly whenidCfg.CreativeID != "". A config with frequency rules but no creative ID now silently evaluates as uncapped. I'd prefer a config-load error over a silent skip. - Coverage gap on source/identity semantics. The three
TestIdentity_Source*tests that covered source-hash provenance and cross-source dedupe were deleted outright with no replacement. That's exactly the area my Q1 (impression-ID uniqueness) touches — if those invariants still hold, they need tests against the new path; if they don't, the removal deserves discussion. - Test-to-implementation drift. Most remaining identity tests now seed state directly via
store.SetUserExposures/store.AddExposurerather than exercising any public API. That moves the tests away from the spec and toward verifying internal plumbing.
Nits
maxExposureEntriesinexposure_binary.goappears to be orphaned onceRecordExposureis gone.exposure_binary.gov3 entry doc comment doesn't note that v2 is now rejected.PruneExpiredinexposure.gois still exported but has no caller in production code after this PR.valkeystore/integration_test.godropsmr.FastForward(25 * time.Hour)in favor of advancing onlyengine.Now. Valkey TTL is no longer exercised — consistent with the refactor, but the test namedSlidingWindowExpirynow tests only engine-side cutoff arithmetic.
Bigger-picture concern
None of my three inline questions above are resolved anywhere in the diff. Combined with the blocking items, this refactor deletes the primitives (sorted sets, native TTL, pipelined writes) that would answer Q3 about where dedupe/expiry lives — and those are the same primitives PR #11 is trying to codify as the Store abstraction in the reference identity agent. I'd like us to align on the target design for exposure storage before we land a PR that removes that machinery from the production path.
Replaced by #75
Summary
ExposeRequest,ExposeResponse,ValidateExposeRequest, andEngine.RecordExposure; eligibility still reads the binary exposure log viaEvaluateIdentityResolved.ZAdd/ZCount/ZExpire/ZRemRangeByScore) from theStoreinterface and both implementations; drop the key-level expiry map inMockStorethat was only populated byZExpire.Metrics.ExposureRecorded+ its prommetrics registration.Store.SetUserExposures/AddExposure(targeting, valkeystore, e2e).Test plan
go build ./...go vet ./...go test -count=1 ./...across root,targeting,targeting/valkeystore,targeting/prommetrics,e2e, reference/bench modules🤖 Generated with Claude Code