Repository navigation
Associate feature flags with success metrics (exposures + cohort readout) - #1080
Conversation
📝 WalkthroughWalkthroughFeature flags now support optional success metrics, preserve assignment sources during evaluation, record measured exposures through Analytics Engine or D1, aggregate month-to-date cohorts with usage data, and display readouts in web and MCP admin surfaces. Exposure rollups receive migration, account-data, and 90-day retention coverage. ChangesFeature flag success metrics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UserRequest
participant FeatureFlagService
participant ExposureRecorder
participant ReadoutLoader
participant AdminSurface
UserRequest->>FeatureFlagService: evaluate flags with assignment sources
FeatureFlagService->>ExposureRecorder: record measured exposures
ExposureRecorder-->>ReadoutLoader: persisted exposure data
ReadoutLoader->>ReadoutLoader: join exposures with usage events
ReadoutLoader->>AdminSurface: attach on/off metric readout
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/feature-flags/success-metric-readout.ts (1)
325-389: 🚀 Performance & Scalability | 🔵 TrivialNo caching for per-flag external readout queries.
attachFeatureFlagMetricReadoutsfires 2 external Analytics Engine REST calls per measured flag on every/admin/feature-flagsload and everyadmin_feature_flag_listMCP call, uncached. Fine today with a single measured flag, but as more flags adoptsuccessMetric(which the UI explicitly encourages), admin page load time will grow linearly with external API round-trips. Worth a short-TTL cache in front of the readout in a follow-up once adoption grows.🤖 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 `@packages/worker/src/feature-flags/success-metric-readout.ts` around lines 325 - 389, Introduce a short-TTL cache for per-flag readouts used by attachFeatureFlagMetricReadouts, keyed by flag key and success metric so repeated admin and MCP requests reuse recent external results. Apply caching around loadFeatureFlagSuccessMetricReadout while preserving the existing D1/Analytics Engine selection and unavailable fallback behavior.
🤖 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 `@packages/worker/migrations/0117-feature-flag-exposure-rollups.sql`:
- Around line 20-21: Add a dedicated index on the feature_flag_exposure_rollups
day column alongside the existing user_id index so retention pruning can
efficiently filter and order exposures by day.
In `@packages/worker/src/app/retention.ts`:
- Around line 823-844: Update pruneFeatureFlagExposuresForRetention to require a
userId and scope both the retention SELECT and selectAndDeleteByIds deletion to
that user. Add user_id to the selection predicate and bindings, and configure
the deletion predicate/key so rows from other users cannot be removed.
In `@packages/worker/src/feature-flags/registry.ts`:
- Around line 82-95: Update the measured feature-flag readout flow rooted at
measuredFeatureFlagDefinitions and measuredFeatureFlagKeys so every Analytics
Engine and D1 cohort query receives an authorized userId scope before returning
data. Thread userId through the relevant readout functions and add the
caller-user predicate to each read query, preserving existing aggregation
behavior within that user’s scope.
---
Nitpick comments:
In `@packages/worker/src/feature-flags/success-metric-readout.ts`:
- Around line 325-389: Introduce a short-TTL cache for per-flag readouts used by
attachFeatureFlagMetricReadouts, keyed by flag key and success metric so
repeated admin and MCP requests reuse recent external results. Apply caching
around loadFeatureFlagSuccessMetricReadout while preserving the existing
D1/Analytics Engine selection and unavailable fallback behavior.
🪄 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: 7139ece9-794d-4305-9e21-38d12d1f986c
📒 Files selected for processing (35)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/feature-flags.mddocs/contributing/architecture/usage-metering.mde2e/admin-feature-flags.spec.tspackages/worker/client/routes/admin-feature-flags.tsxpackages/worker/migrations/0117-feature-flag-exposure-rollups.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/app/account-retention-dispositions.tspackages/worker/src/app/admin-feature-flags-data.tspackages/worker/src/app/handlers/admin-feature-flags.node.test.tspackages/worker/src/app/handlers/session-handler.node.test.tspackages/worker/src/app/request-feature-flags-cache.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/app/retention.tspackages/worker/src/app/session-info.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/env-schema.tspackages/worker/src/feature-flags/exposure.node.test.tspackages/worker/src/feature-flags/exposure.tspackages/worker/src/feature-flags/registry.tspackages/worker/src/feature-flags/service.node.test.tspackages/worker/src/feature-flags/service.tspackages/worker/src/feature-flags/success-metric-readout.node.test.tspackages/worker/src/feature-flags/success-metric-readout.tspackages/worker/src/feature-flags/types.tspackages/worker/src/mcp/capabilities/access-control.tspackages/worker/src/mcp/capabilities/admin/admin-feature-flag-list.tspackages/worker/src/mcp/capabilities/admin/feature-flag-shared.tspackages/worker/src/usage/aggregate-rollups.tspackages/worker/src/usage/event-types.tspackages/worker/src/usage/record-usage.tspackages/worker/tsconfig-client.jsonpackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonctools/migration-ledger.json
| export async function pruneFeatureFlagExposuresForRetention(input: { | ||
| db: D1Database | ||
| now?: Date | ||
| batchSize?: number | ||
| }) { | ||
| const cutoffDay = cutoffIso( | ||
| input.now ?? new Date(), | ||
| featureFlagExposureRetentionDays, | ||
| ).slice(0, 'YYYY-MM-DD'.length) | ||
| return selectAndDeleteByIds({ | ||
| db: input.db, | ||
| column: 'rowid', | ||
| bindings: [cutoffDay, input.batchSize ?? retentionDefaultBatchSize], | ||
| sql: `SELECT rowid | ||
| FROM feature_flag_exposure_rollups | ||
| WHERE day < ? | ||
| ORDER BY day ASC, rowid ASC | ||
| LIMIT ?`, | ||
| table: 'feature_flag_exposure_rollups', | ||
| idColumn: 'rowid', | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Scope retention reads and deletes by userId.
This user-owned table is selected and deleted solely by day; selectAndDeleteByIds consequently removes rows across every user. Make the retention operation explicitly user-scoped and include user_id in both selection and deletion predicates. As per coding guidelines, “every read and write path must be scoped by userId.”
🤖 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 `@packages/worker/src/app/retention.ts` around lines 823 - 844, Update
pruneFeatureFlagExposuresForRetention to require a userId and scope both the
retention SELECT and selectAndDeleteByIds deletion to that user. Add user_id to
the selection predicate and bindings, and configure the deletion predicate/key
so rows from other users cannot be removed.
Source: Coding guidelines
| /** | ||
| * Registry flags that declare a success metric. Only these flags get | ||
| * exposure recording and admin metric readouts. | ||
| */ | ||
| export const measuredFeatureFlagDefinitions: ReadonlyArray< | ||
| FeatureFlagDefinition & { successMetric: FeatureFlagSuccessMetric } | ||
| > = featureFlagDefinitions.flatMap((definition) => | ||
| 'successMetric' in definition ? [definition] : [], | ||
| ) | ||
|
|
||
| export const measuredFeatureFlagKeys: ReadonlySet<FeatureFlagKey> = new Set( | ||
| measuredFeatureFlagDefinitions.map( | ||
| (definition) => definition.key as FeatureFlagKey, | ||
| ), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope metric readouts by userId.
These measured flags feed the supplied Analytics Engine and D1 readouts, which aggregate every user_id without a caller-user predicate. Thread an authorized userId scope through the readout queries before returning cohort data.
As per coding guidelines, “every read and write path must be scoped by userId.”
🤖 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 `@packages/worker/src/feature-flags/registry.ts` around lines 82 - 95, Update
the measured feature-flag readout flow rooted at measuredFeatureFlagDefinitions
and measuredFeatureFlagKeys so every Analytics Engine and D1 cohort query
receives an authorized userId scope before returning data. Thread userId through
the relevant readout functions and add the caller-user predicate to each read
query, preserving existing aggregation behavior within that user’s scope.
Source: Coding guidelines
4a3e678 to
6580b1b
Compare
|
🔎 Preview deployed: https://kody-pr-1080.kody-a99.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 573d3b6. Configure here.
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 (1)
packages/worker/src/feature-flags/service.ts (1)
297-300: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftScope admin identity reads to the requesting user.
listFeatureFlagsForAdminhas no requesteruserId, yet now returns every override owner’s and updater’sstableUserId. This exposes cross-user identifiers through both web and MCP admin responses. Pass and enforce the requester scope, or keep cross-user identity data in a separately authorized server-only operation and omit it from this response.As per coding guidelines, “every read and write path must be scoped by
userId” and “Cross-user data sharing is a bug.”Also applies to: 309-323, 344-350, 373-379
🤖 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 `@packages/worker/src/feature-flags/service.ts` around lines 297 - 300, Update listFeatureFlagsForAdmin and its related queries/response mappings to require the requesting userId and scope every feature-flag, override-owner, and updater read to that user. Do not return cross-user stableUserId values through web or MCP admin responses; omit them unless a separately authorized server-only operation is used. Propagate the requester scope through all affected call sites and preserve authorization for each read path.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/worker/src/feature-flags/success-metric-readout.ts (1)
378-394: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReadouts are recomputed per flag on every admin request.
Each measured flag costs two Analytics Engine REST round-trips, fanned out unbounded via
Promise.all, and this runs on every GET and on every mutation response (admin-feature-flags-data.ts). Fine at one measured flag; consider a short TTL cache (KV or in-worker) or lazily loading the readout on demand before more flags declare metrics.🤖 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 `@packages/worker/src/feature-flags/success-metric-readout.ts` around lines 378 - 394, Update attachFeatureFlagMetricReadouts to avoid recomputing readouts on every admin request: add a short-TTL cache for loadFeatureFlagSuccessMetricReadout results, keyed by flag identity and the relevant metric inputs, and reuse valid cached values across both GET and mutation-response paths. Preserve stale or metric-less flags and ensure cache expiry triggers a fresh readout while limiting concurrent Analytics Engine requests.
🤖 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 `@packages/worker/src/feature-flags/success-metric-readout.ts`:
- Around line 280-288: Update the D1 query in the exposure readout flow around
the `exposureResult` query to bound `day` inclusively by both
`window.monthStartDay` and the window end day, matching the Analytics Engine
path. Bind the additional upper-bound parameter using the existing window end
value, while preserving the current grouping and row mapping.
---
Outside diff comments:
In `@packages/worker/src/feature-flags/service.ts`:
- Around line 297-300: Update listFeatureFlagsForAdmin and its related
queries/response mappings to require the requesting userId and scope every
feature-flag, override-owner, and updater read to that user. Do not return
cross-user stableUserId values through web or MCP admin responses; omit them
unless a separately authorized server-only operation is used. Propagate the
requester scope through all affected call sites and preserve authorization for
each read path.
---
Nitpick comments:
In `@packages/worker/src/feature-flags/success-metric-readout.ts`:
- Around line 378-394: Update attachFeatureFlagMetricReadouts to avoid
recomputing readouts on every admin request: add a short-TTL cache for
loadFeatureFlagSuccessMetricReadout results, keyed by flag identity and the
relevant metric inputs, and reuse valid cached values across both GET and
mutation-response paths. Preserve stale or metric-less flags and ensure cache
expiry triggers a fresh readout while limiting concurrent Analytics Engine
requests.
🪄 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: fa6890a5-5ff7-4c63-b2d7-7cf4f266b193
📒 Files selected for processing (35)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/feature-flags.mddocs/contributing/architecture/usage-metering.mde2e/admin-feature-flags.spec.tspackages/worker/client/routes/admin-feature-flags.tsxpackages/worker/migrations/0117-feature-flag-exposure-rollups.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/app/account-retention-dispositions.tspackages/worker/src/app/admin-feature-flags-data.tspackages/worker/src/app/handlers/admin-feature-flags.node.test.tspackages/worker/src/app/handlers/session-handler.node.test.tspackages/worker/src/app/request-feature-flags-cache.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/app/retention.tspackages/worker/src/app/session-info.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/env-schema.tspackages/worker/src/feature-flags/exposure.node.test.tspackages/worker/src/feature-flags/exposure.tspackages/worker/src/feature-flags/registry.tspackages/worker/src/feature-flags/service.node.test.tspackages/worker/src/feature-flags/service.tspackages/worker/src/feature-flags/success-metric-readout.node.test.tspackages/worker/src/feature-flags/success-metric-readout.tspackages/worker/src/feature-flags/types.tspackages/worker/src/mcp/capabilities/access-control.tspackages/worker/src/mcp/capabilities/admin/admin-feature-flag-list.tspackages/worker/src/mcp/capabilities/admin/feature-flag-shared.tspackages/worker/src/usage/aggregate-rollups.tspackages/worker/src/usage/event-types.tspackages/worker/src/usage/record-usage.tspackages/worker/tsconfig-client.jsonpackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonctools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/worker/src/app/account-retention-dispositions.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/worker-configuration.d.ts
- packages/worker/src/usage/record-usage.ts
- docs/contributing/architecture/feature-flags.md
- docs/contributing/architecture/usage-metering.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/worker/src/feature-flags/success-metric-readout.ts`:
- Around line 283-290: The exposure query in the success-metric readout must
enforce an authorized user or account boundary instead of aggregating all users
for the flag. Update the query and its bindings around the exposure read to
filter by the applicable user/account scope, and apply the same scope to the
associated usage read before combining results; preserve explicitly authorized
aggregate behavior only if an established aggregate scope is available.
🪄 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: 66612818-2031-48c9-81d3-d6b7d25d951c
📒 Files selected for processing (2)
packages/worker/src/feature-flags/success-metric-readout.node.test.tspackages/worker/src/feature-flags/success-metric-readout.ts
| const exposureResult = await input.db | ||
| .prepare( | ||
| `SELECT user_id, enabled, source | ||
| FROM feature_flag_exposure_rollups | ||
| WHERE flag_key = ? AND day >= ? AND day <= ? | ||
| GROUP BY user_id, enabled, source`, | ||
| ) | ||
| .bind(input.flagKey, window.monthStartDay, windowEndDay) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope the exposure read by an authorized user or account boundary.
This query reads every user’s exposure for flagKey without a userId or other enforced aggregate scope, allowing user-owned records to be combined into the admin readout. Apply the same boundary to the associated usage read, or introduce an explicitly authorized aggregate scope before reading cross-user data.
As per coding guidelines, every read and write path must be scoped by userId; every Durable Object ID backing user-owned state must be namespaced by userId; and every search or vector path must filter by userId.
🤖 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 `@packages/worker/src/feature-flags/success-metric-readout.ts` around lines 283
- 290, The exposure query in the success-metric readout must enforce an
authorized user or account boundary instead of aggregating all users for the
flag. Update the query and its bindings around the exposure read to filter by
the applicable user/account scope, and apply the same scope to the associated
usage read before combining results; preserve explicitly authorized aggregate
behavior only if an established aggregate scope is available.
Source: Coding guidelines
3 participants
What
Every feature flag can now declare the metric it exists to move, and the admin surfaces show whether it is actually moving it.
FeatureFlagDefinitiongains an optionalsuccessMetric(eventTypefrom the closed usage-metering union,measure,goal,hypothesis), reviewed in the same PR that creates the flag.execute-pre-exec-typecheckdeclares one (executeerror rate should decrease); the permanentdemo-indicatordeliberately does not.(stable user id, flag key, on/off, assignment source, timestamp). Production/preview writes go to a newFLAG_EXPOSURESAnalytics Engine dataset; local dev/tests use a new D1feature_flag_exposure_rollupstable (migration0117, 90-day retention, covered by account deletion/export targets). The writer mirrorsrecordUsage's never-throws contract.success-metric-readout.tsjoins exposures with usage events for the declared metric over the current UTC month, splits users into on/off cohorts, and excludes hand-picked override users and mixed-exposure users (reported separately). Attached to/admin/feature-flagsandadmin_feature_flag_list.successMetricrender a notice in the admin UI and carry asuccessMetricNoticefield in the MCP list output.Why
Flags gate rollouts, but nothing connected "flag is on" to "the thing we hoped would improve actually improved". Associating each flag with a code-reviewed metric hypothesis plus exposure-based cohort data makes "push forward or kill it" an evidence-based operator decision. Assignment-source tracking is what keeps the comparison honest: current flag state cannot reconstruct who was inside a percentage rollout last week, and override users are selection-biased.
Demo (local dev)
Two seeded users were split by a 20% rollout (jane on, kody off), with seeded
executeusage for the month. The admin page shows the missing-metric notice ondemo-indicatorand the cohort readout onexecute-pre-exec-typecheck(on cohort 6.7% error rate vs off cohort 36.0%).Notes for review
UsageEventTypemoved to a dependency-freepackages/worker/src/usage/event-types.ts(re-exported fromrecord-usage.ts) so the client-safe flag registry can reference it.getFeatureFlagsForUseris now a thin wrapper overgetFeatureFlagEvaluationsForUser, which also returns the assignment source (override/global/rollout/default). Evaluation precedence is unchanged.WRANGLER_IS_LOCAL_DEVis set.accountUserDataTargetsentry.{ status: 'unavailable' }so the admin page renders even when the analytics backend is down.0113to0117after rebasing on main; the branch is rebased on top of the stable-user-id admin transport change (Use stable user IDs across identity boundaries #1072).System recap — extends feature flags and usage metering (medium risk)
Mode: recap · Base:
main@3b84c504· Head:6580b1bcClassification: extends — no new primitive; the
feature-flagsprimitive gains success-metric declaration, exposure recording, and a metric readout, joined against the existingusage-meteringevent stream.Primitives touched
feature-flagssuccessMetricregistry field, evaluation sources, exposure writer, cohort readoutusage-meteringUsageEventTypeextracted to a dependency-free module; AE SQL helper exported for reused1-app-dbfeature_flag_exposure_rollupstable (migration0117)app-ui/admin/feature-flagscapability-registryadmin_feature_flag_listoutput extendedscheduled-cronaccount-exportSystem map
Flag evaluation now emits exposures alongside its answer, and the admin surfaces join those exposures with usage events into an on/off cohort readout.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
Invariants
Per-user isolation: exposures are keyed by the stable user id, the D1 fallback table is in
accountUserDataTargets(deletion + export), and the readout only ever aggregates across users for the admin-only surfaces (admin role required on both the route and the capability).Summary by CodeRabbit