Generalize frequency rules from scalar package_id+campaign_id to arbitrary fcap_keys[]

[bokelley]

Background

The AdCP TMP spec (adcp#3359) defines frequency caps using an fcap_keys[] label model — tenant:dimension:value strings (e.g. buyer-acme:campaign:42, buyer-acme:advertiser:13, buyer-acme:creative:8) — with arbitrary buyer-chosen dimensions.

The current adcp-go/targeting/ impl predates this design and uses scalar fields:

• ExposureEntry { ImpressionID, PackageID, CampaignID, SourceID, Timestamp }
• PackageIdentityConfig.FrequencyRules (per package)
• CampaignFreqConfig.FrequencyRules (per campaign)
• CheckFrequencyRulesMultiLog(logs, filterHash, isCampaign, ...) — boolean isCampaign flag picks between two scalar paths

This means today the impl supports exactly package-level and campaign-level caps; advertiser, creative, line-item, group, or any other dimension a buyer might want to cap on isn't expressible.

What needs to change

Generalize the data model so the dimension is arbitrary:

1. ExposureEntry: carry fcap_keys []string (or pre-hashed fcap_keys [][8]byte for binary log compactness) instead of separate PackageID / CampaignID fields. Or keep PackageID/CampaignID for backward compat and add fcap_keys as an additional field.
2. FrequencyRule: attach to a fcap_key string rather than implicitly to a package or campaign.
3. Engine: filter exposure entries by fcap_key membership in the entry's fcap_keys[], not by scalar match.
4. Binary log format: exposure_binary.go currently has fixed slots for ImpressionHash / PackageHash / CampaignHash / SourceHash. Either widen entries to variable-size (carry a count + N fcap_key hashes) or version-bump to a different layout. Backward compat plan: v1 = current, v2 = arbitrary fcap_keys, decoder handles both.
5. CheckFrequencyRulesMultiLog / CheckFrequencyRulesAggregated: drop the isCampaign bool parameter, take a single filterHash that matches against any fcap_key on the entry.

Backward compatibility

Existing callers using package_id and campaign_id should continue to work. Suggested approach:

• Keep PackageID / CampaignID fields on ExposureEntry (deprecate but don't remove)
• Add FcapKeys []string
• During the transition, treat PackageID and CampaignID as implicit fcap_keys (pkg:<id> and campaign:<id>) so existing rules keep working
• New rules attached via fcap_key directly

Why this matters

Without the generalization, buyers can only cap at package and campaign level. The whole point of the label model (per the spec) is that buyers choose dimensions — most DSPs need advertiser-level, creative-level, and flight-level caps. Today buyers running adcp-go's targeting/ package can't express those.

Out of scope for this issue

• The pre-aggregation optimization in #103 — that's orthogonal and should land first; the generalization extends the same data structures it indexes.
• Wire-spec changes — TMP itself doesn't carry fcap_keys on the wire; they're buyer-internal. No protocol coordination needed.
• Multi-tenant isolation rules (tenant prefix requirement) — those are buyer-side enforcement that lives in the SDK, not in targeting/.

References

• AdCP spec PR: adcontextprotocol/adcp#3359
• fcap_keys label model section: docs/trusted-match/identity-match-implementation.mdx#fcap_keys-label-model
• Spec open question Schema-driven type generation from JSON Schema #1 explicitly tracks this work
• Related upstream perf PR (preaggregation): #103 — should land first; generalization builds on the same indexes.

[bokelley]

Triage

Classification: Feature request
Bucket(s): targeting
Status: deferred — blocked on #103

Blocked-on: #103 — resurfaces on merge

PR #103 (preaggregation perf) is open and touches the same targeting/ data structures this issue proposes to generalize: it introduces CheckFrequencyRulesAggregated keeping the isCampaign bool parameter that issue #104 proposes to drop, and it indexes on PackageHash/CampaignHash — the exact scalar fields this issue wants to generalize away. The issue author correctly sequences them: #103 lands first, then the fcap_keys[] generalization extends the indexed structures.

What to expect on resurfacing: This is a non-trivial breaking change even with the backward-compat shim approach — removing or deprecating exported struct fields (PackageID/CampaignID on ExposureEntry), changing public function signatures (CheckFrequencyRulesMultiLog/CheckFrequencyRulesAggregated losing isCampaign bool), and versioning the binary log format. It will route to human review once the blocker clears; no auto-PR path.

My take: Correct sequencing. Attempting to generalize the data model while #103's aggregation index is still in review would produce a three-way conflict on the same structs and functions. Clean re-triage after merge.

---

Triaged by Claude Code. Session: https://claude.ai/code/session_013sbjV3XU2j83Mz6ifnvPBw

---

Generated by Claude Code

[bokelley]

Pre-launch — no production deployments — so the back-compat section in the issue body is over-engineered. Tightening scope:

Drop the entire back-compat shim.

• ExposureEntry becomes { ImpressionID, FcapKeys [][8]byte, Timestamp }. Remove PackageID / CampaignID / SourceID outright. No deprecation, no implicit-key shim, no pkg:<id> / campaign:<id> translation.
• FrequencyRule attaches to a single fcap_key. No package/campaign-rule carve-outs.
• CheckFrequencyRulesMultiLog / CheckFrequencyRulesAggregated — drop the isCampaign bool parameter, rename to CheckFrequencyRules(logs, fcapKeyHash, rules, nowUnix). Single path, single signature.
• Binary log format — pick one shape and ship it. No version byte, no v1/v2 decoder branch. The design call (fixed-stride with capped N vs. variable-size vs. primary + sidecar) is still load-bearing — just simpler now because there's no v1 to keep working.
• Migration timeline section — delete. Wipe valkey, redeploy.

The "what needs to change" list collapses from 5 items with back-compat caveats to 5 items without them.

Related upstream simplification: the fcap_keys format on the spec side (adcp#3359) is dropping the required tenant prefix — single-tenant-per-buyer deployment model means tenant prefix isn't load-bearing for the protocol. Format becomes dimension:value, not tenant:dimension:value. That removes the wrinkle around the implicit-key shim violating spec conformance — moot now that the shim is gone, but worth noting that the spec conformance bar on key format is also relaxed.

Open follow-up: still need to pick the binary log shape before any code goes in. That's the next decision.

[bokelley]

Acknowledged — scope update noted, defer reasoning refreshed.

What changed:

• Back-compat shim dropped (pre-launch; wipe-valkey-and-redeploy model). ExposureEntry collapses to { ImpressionID, FcapKeys [][8]byte, Timestamp } — PackageID / CampaignID / SourceID removed outright, no implicit-key translation.
• Function path consolidates to CheckFrequencyRules(logs, fcapKeyHash, rules, nowUnix) — isCampaign bool gone, single code path.
• Binary log: single format, no version byte. Design decision (fixed-stride-capped-N vs. variable-size vs. primary+sidecar) remains explicitly open — must be resolved before any code goes in.
• Upstream spec: fcap_keys key format relaxes to dimension:value (no required tenant prefix) per adcp#3359. The implicit-key shim conformance concern is moot now that the shim is gone.

Disposition unchanged: Blocked-on: #103 — resurfaces on merge. PR #103 is still open and introduces CheckFrequencyRulesAggregated (retaining isCampaign bool) while indexing on the PackageHash/CampaignHash scalar fields this issue replaces. Sequencing still holds.

On resurfacing after #103: With back-compat dropped, this is now a clean-break replacement — removing exported struct fields from ExposureEntry, renaming/consolidating public functions. Still routes to Flag for human review (not auto-PR) under the breaking-change rules. The open binary log shape decision must also resolve before code; expect that to be called out for @bokelley at that point.

---

Triaged by Claude Code. Session: https://claude.ai/code/cse_014iFL2UxdZqEC3BWXxAs925

---

Generated by Claude Code

[bokelley]

Bench results — six storage shapes compared against real valkey

Built a bench package (targeting/expbench/, six variants, sanity-test verified equivalence on rolling-window rules) to drive this issue's storage-shape decision empirically. Single-client local valkey 7.2, 200 iterations per measurement.

The headline: the fcap rule shape matters more than the storage shape. AppNexus-style framing — every fcap rule is either bucket-per-period (1/day, 5/day, 1/week, 1/month) or rolling-window — collapses 90% of the work to a different storage model than what this issue originally proposed.

Heavy 30K-impression / 30-day retention, 24h rule

variant	write p50	read1 p50	read100 p50	read1k p50	cleanup	memory
binary (fcap_keys-extended)	56785 µs	8838 µs	7594 µs	8704 µs	46959 µs	3.0 MB
zset-array	423 µs	951 µs	1195 µs	1997 µs	3320 µs	3.5 MB
zset-perkey	781 µs	2778 µs	3170 µs	4021 µs	1595 µs	12.0 MB
zset-perkeyed (one ZSET per (user, fcap_key))	898 µs	305 µs	1996 µs	4145 µs	7045 µs	12.0 MB
bucket-day (SET per (user, day))	299 µs	250 µs	386 µs	482 µs	0 (TTL)	572 KB
bucket-count (HASH per (user, day))	298 µs	789 µs	597 µs	536 µs	0 (TTL)	249 KB

Heavy 30K, 30d rule (worst case for ZSET — no server-side window filter)

variant	write p50	read1 p50	read100 p50	read1k p50
binary	51102 µs	6987 µs	31823 µs	55742 µs
zset-array	585 µs	12956 µs	18972 µs	23378 µs
zset-perkey	685 µs	33237 µs	50658 µs	32777 µs
zset-perkeyed	298 µs	772 µs	19933 µs	41255 µs

(Bucket variants don't apply to a 30d rolling rule — they'd need a bucket-month shape with TTL=30d, structurally identical perf.)

What this changes for #104

The original issue body assumes a single storage path that needs to handle arbitrary frequency rules over a single binary exposure log. The bench shows no single shape wins — the choice depends on rule semantics:

Rule shape	Storage	Why	Heavy-30K perf
singleton:{day,week,month,lifetime}	SET per (user, period), TTL	One-shot SISMEMBER / SMEMBERS+intersect	read1k 482 µs, mem 572 KB
count:{day,week,month} (N>1)	HASH per (user, period), TTL, HINCRBY	Same shape as singleton, count instead of presence	read1k 536 µs, mem 249 KB
rolling:N:Ns	ZSET per (user, fcap_key)	Only place ZSET wins; per-keyed isolates the hot data	read1 305 µs

Implications for the rework scoped in this issue:

1. Binary log goes away entirely. Loses on writes by 100+× under both bucket and ZSET comparisons. The fixed-stride scan advantage doesn't materialize once you account for read-modify-write cost on every impression.
2. PR perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103's preagg index becomes irrelevant for the 90% case. Bucket reads are flat across batch sizes — one SMEMBERS or HGETALL covers a 1000-key batch in a single RT. The whole "preaggregate to amortize per-package scan" optimization only matters in the rolling-window path, which is rare.
3. Schema change required. Each fcap_key declares its rule shape (singleton:day, count:week, rolling:N:Ns, etc.). Buyer commits up-front; we don't try to support arbitrary (MaxCount, Window) combinations.
4. Storage code shrinks. Two thin stores (BucketStore, RollingStore) plus a FrequencyEvaluator that picks based on declared shape. No exposure log, no preagg index, no binary format design call.
5. Constraint we accept: can't express "5 per week" or arbitrary (N, Window) combinations. AppNexus didn't either; production validated this for ~a decade.

Bench code

Committed at targeting/expbench/ on branch bokelley/expbench-fcap-storage (six variants, ~2650 LOC, own go module to keep redis deps isolated). Run with:

docker run -d -p 6380:6379 valkey/valkey:7.2
VALKEY_ADDR=localhost:6380 go test -count=1 -run TestBench$ -v ./targeting/expbench/

Sanity test (TestSanity_Equivalence) confirms all rolling-window variants compute identical eligibility answers; bucket variants are excluded since they implement different rule semantics.

Proposed next step

Update this issue body to replace the "what needs to change" list with the AppNexus-split scope:

1. Schema: add rule_shape field to fcap rule definitions (enum: singleton:{day,week,month,lifetime}, count:{day,week,month}, rolling:N:Ns)
2. Storage: implement BucketStore (SET-or-HASH per period) and RollingStore (ZSET per (user, fcap_key))
3. Engine: FrequencyEvaluator dispatches by declared shape
4. Remove: exposure_binary.go, exposure_aggregate.go, the entire BinaryExposureLog apparatus
5. spec: adcp#3359 documents the rule_shape enum and the bucket-vs-rolling semantics

[bokelley]

Triage

Classification: Feature request
Bucket(s): targeting
Status: ready-for-human — blocking dependency inverted; defer reasoning refreshed

Bench data received. Three things change from the prior engagement:

1. Binary log design decision closed. At 100×+ write latency vs. bucket/ZSET shapes under the 30K-impression workload, the binary-log path is eliminated. The open design call from the last triage ("must resolve before code goes in") is now resolved: no binary log at all.

2. Blocking relationship has inverted. The prior defer rationale was "land perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103 first, then extend its indexed structures." The bench shows Generalize frequency rules from scalar package_id+campaign_id to arbitrary fcap_keys[] #104 in its new form — BucketStore + RollingStore, entire BinaryExposureLog apparatus removed — would supersede perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103's additions. CheckFrequencyRulesAggregated and the preagg-on-binary-log path become dead code the moment Generalize frequency rules from scalar package_id+campaign_id to arbitrary fcap_keys[] #104 lands. Landing perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103 first now creates cleanup work rather than a foundation.

3. Scope is cleaner than the original proposal. Two thin stores + a dispatcher is a smaller, more auditable surface than an extended binary format with a version decoder. TTL-based cleanup removes explicit cleanup code paths. The AppNexus-style rule-shape enum constraint is well-understood operationally and production-validated.

Human decision needed before #104 can proceed:

PR #103 is open and blocked (not approved, not awaiting merge). Three paths:

• (a) Close perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103 — the optimization is irrelevant for the 90% bucket path; the rolling-window path doesn't need preagg either (per-keyed ZSET already isolates hot data). Cleanest.
• (b) Scope perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103 down — strip anything touching BinaryExposureLog / CheckFrequencyRulesAggregated, land only what survives the new model. Unclear if anything worth keeping remains.
• (c) Merge perf(targeting): preaggregate exposure log per filter hash for high-package eligibility #103 as-is, have Generalize frequency rules from scalar package_id+campaign_id to arbitrary fcap_keys[] #104 delete the additions — creates deliberate short-window technical debt. Auditable but untidy.

Option (a) looks right from here, but that's your call, @bokelley.

Once #103 disposition is settled, #104 routes to Flag for human review as before — removing exported fields from ExposureEntry and consolidating function signatures is a breaking change that needs explicit sign-off regardless of pre-launch state.

The proposed issue body update (replace "what needs to change" with the AppNexus-split scope) makes sense. Happy to re-triage after #103 is resolved.

---

Triaged by Claude Code. Session: https://claude.ai/code/cse_01BnNYzGDnXWbw8n6PLmTnF2

---

Generated by Claude Code