fix: correct feature flag and experiment evaluation#527
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds async experiment exposure tracking for custom events, an exposure-attribution subquery for experiment results/charts, unique-variant-key validation, stricter targeting-rule handling for missing attributes, earlier rate-limiting/bot checks and Redis-backed project loading for flag evaluation, forwardRef module wiring, and adds ChangesExperiment Exposure Tracking
Experiment Results, Attribution & Variant Validation
Feature-Flag Evaluation Flow & Wiring
Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalyticsController
participant ExperimentService
participant ClickHouse
Client->>AnalyticsController: POST /analytics/custom (pid, ev, profileId)
AnalyticsController->>ClickHouse: insert transformed event into events table
AnalyticsController->>ExperimentService: find running experiments (pid, CUSTOM_EVENT, eventName)
ExperimentService-->>AnalyticsController: experiments + variants
AnalyticsController->>AnalyticsController: sort variants, compute variantKey via getExperimentVariant(profileId)
AnalyticsController->>ClickHouse: async_insert JSONEachRow -> experiment_exposures (pid, experimentId, variantKey, profileId, created)
ClickHouse-->>AnalyticsController: async_insert result (logged)
AnalyticsController-->>Client: 200 OK (event response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/community/src/feature-flag/feature-flag.controller.ts (1)
263-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep disabled flags out of the public evaluation payload.
Switching to
findByProject()makes this unauthenticated endpoint return disabled flag keys asfalse. That exposes staged/internal flag names to anyone with a project id, and it also records syntheticfalseevaluations for disabled flags infeature_flag_evaluations.🤖 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 `@backend/apps/community/src/feature-flag/feature-flag.controller.ts` around lines 263 - 316, The endpoint is returning disabled flags as false because you call featureFlagService.findByProject() and pass all flags to evaluateFlags and trackEvaluations; filter out flags where flag.enabled (or staged/enabled property) is false before calling this.featureFlagService.evaluateFlags and before invoking this.trackEvaluations so disabled flags are neither evaluated nor recorded; alternatively use a service method like featureFlagService.findEnabledByProject or apply flags = flags.filter(f => f.enabled) prior to evaluateFlags/trackEvaluations so the returned payload and feature_flag_evaluations contain only enabled flags.backend/apps/cloud/src/feature-flag/evaluation.ts (1)
187-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't force unallocated traffic into the last variant.
When the variant percentages add up to less than 100, this fallback silently assigns the remainder to the last variant instead of returning no assignment. That skews experiment exposure and results.
Proposed fix
for (const variant of variants) { cumulativePercentage += variant.rolloutPercentage if (normalizedValue < cumulativePercentage) { return variant.key } } - return variants[variants.length - 1].key + return null🤖 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 `@backend/apps/cloud/src/feature-flag/evaluation.ts` around lines 187 - 195, The current loop forcibly assigns any leftover traffic to the last variant by returning variants[variants.length - 1].key when cumulativePercentage < 100; change this so that if normalizedValue is not less than the running cumulativePercentage (i.e., the variant percentages sum to less than 100 and the user falls in the unallocated range) the function returns no assignment (null or undefined) instead of defaulting to the last variant. Modify the logic around cumulativePercentage, variants, and normalizedValue so that after the for-loop you return null/undefined when the normalizedValue wasn't matched, rather than returning the last variant key.
🤖 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 `@backend/apps/cloud/src/analytics/analytics.controller.ts`:
- Around line 1674-1679: The code is awaiting
trackCustomEventExperimentExposures(...) which forces the custom-event request
to wait for an extra DB read/ClickHouse write; remove the await and invoke
trackCustomEventExperimentExposures(eventsDTO.pid, eventsDTO.ev, profileId,
transformed.created) as a fire-and-forget call so the request path isn't blocked
(the function already catches/logs its own errors). Ensure the call remains in
the same place (same parameters) but is not awaited; do not add additional
synchronous error handling that would re-block the request.
In `@backend/apps/cloud/src/experiment/experiment.controller.ts`:
- Around line 1192-1216: In getExposureAttributionSubquery, replace the
variantSelector expression for the MultipleVariantHandling.EXCLUDE branch
(currently yielding any(variantKey)) with the same argMin(variantKey,
tuple(created, variantKey)) used for FIRST_EXPOSURE so the selected variant is
explicitly paired with min(created); update the variantSelector conditional that
checks experiment.multipleVariantHandling (and keep existing
multiVariantFilter/HAVING logic) so both FIRST_EXPOSURE and EXCLUDE use argMin
for clear variant-timestamp pairing.
In `@backend/apps/cloud/src/feature-flag/feature-flag.controller.ts`:
- Around line 299-304: The code currently swallows Redis misses by returning {
flags: {} } when this.projectService.getRedisProject(...) throws; instead, treat
Redis as a fast path and fall back to the authoritative lookup: catch errors
from getRedisProject and call the authoritative method (e.g.
this.projectService.getProject(evaluateDto.pid) or equivalent) to retrieve the
project, only returning { flags: {} } if the authoritative lookup returns no
project; apply the same change for the second occurrence at lines ~308-309 so
transient Redis failures don't produce a silent "all flags off" response.
---
Outside diff comments:
In `@backend/apps/cloud/src/feature-flag/evaluation.ts`:
- Around line 187-195: The current loop forcibly assigns any leftover traffic to
the last variant by returning variants[variants.length - 1].key when
cumulativePercentage < 100; change this so that if normalizedValue is not less
than the running cumulativePercentage (i.e., the variant percentages sum to less
than 100 and the user falls in the unallocated range) the function returns no
assignment (null or undefined) instead of defaulting to the last variant. Modify
the logic around cumulativePercentage, variants, and normalizedValue so that
after the for-loop you return null/undefined when the normalizedValue wasn't
matched, rather than returning the last variant key.
In `@backend/apps/community/src/feature-flag/feature-flag.controller.ts`:
- Around line 263-316: The endpoint is returning disabled flags as false because
you call featureFlagService.findByProject() and pass all flags to evaluateFlags
and trackEvaluations; filter out flags where flag.enabled (or staged/enabled
property) is false before calling this.featureFlagService.evaluateFlags and
before invoking this.trackEvaluations so disabled flags are neither evaluated
nor recorded; alternatively use a service method like
featureFlagService.findEnabledByProject or apply flags = flags.filter(f =>
f.enabled) prior to evaluateFlags/trackEvaluations so the returned payload and
feature_flag_evaluations contain only enabled flags.
🪄 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
Run ID: b77aa91e-3b91-41d3-aeea-127279b1a23f
📒 Files selected for processing (11)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.module.tsbackend/apps/cloud/src/analytics/bot-detection.service.tsbackend/apps/cloud/src/experiment/experiment.controller.tsbackend/apps/cloud/src/feature-flag/evaluation.tsbackend/apps/cloud/src/feature-flag/feature-flag.controller.tsbackend/apps/cloud/src/feature-flag/feature-flag.module.tsbackend/apps/cloud/src/feature-flag/feature-flag.service.tsbackend/apps/community/src/analytics/bot-detection.service.tsbackend/apps/community/src/feature-flag/evaluation.tsbackend/apps/community/src/feature-flag/feature-flag.controller.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/cloud/src/feature-flag/evaluation.ts (1)
183-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the experiment hash bucket strictly below
100.
hashValue / 0xffffffffcan evaluate to exactly100when the first 32 bits are all1. With the newreturn nullfallback, that bucket never matches any variant even when rollout totals add up to100.Suggested fix
- const normalizedValue = (hashValue / 0xffffffff) * 100 + const normalizedValue = (hashValue / 0x100000000) * 100🤖 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 `@backend/apps/cloud/src/feature-flag/evaluation.ts` around lines 183 - 195, The normalized bucket can equal 100 when hashValue is 0xffffffff, causing no variant to match; update the bucket calculation so it never reaches 100 by dividing by 0x100000000 (or 4294967296) instead of 0xffffffff when computing normalizedValue, i.e. change the divisor used in normalizedValue computation (the variables involved are hashValue and normalizedValue) so the loop over variants (using cumulativePercentage and variant.rolloutPercentage) will correctly match when rollouts total 100.
🤖 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 `@backend/apps/cloud/src/analytics/analytics.controller.ts`:
- Around line 1746-1750: Change the direct awaited ClickHouse insert into a
fire-and-forget async_insert call: call clickhouse.insert({ table:
'experiment_exposures', values: exposures, format: 'JSONEachRow',
clickhouse_settings: { async_insert: 1 } }) without awaiting it and attach a
.catch to log any error (so the request path isn't blocked but errors are
recorded); update the call site that currently awaits clickhouse.insert(...) in
analytics.controller.ts and reference the exposures variable and the
clickhouse.insert invocation when making the change.
In `@backend/apps/community/src/feature-flag/feature-flag.controller.ts`:
- Around line 243-248: The catch block around the call to
this.projectService.getRedisProject(evaluateDto.pid) silently swallows errors;
update that catch to log a warning via this.logger.warn (include the caught
error and the evaluateDto.pid/context) before returning { flags: {} } so Redis
failures are visible in logs; reference the getRedisProject call and mirror the
style used in analyticsService.checkBot for message formatting and error
inclusion.
---
Outside diff comments:
In `@backend/apps/cloud/src/feature-flag/evaluation.ts`:
- Around line 183-195: The normalized bucket can equal 100 when hashValue is
0xffffffff, causing no variant to match; update the bucket calculation so it
never reaches 100 by dividing by 0x100000000 (or 4294967296) instead of
0xffffffff when computing normalizedValue, i.e. change the divisor used in
normalizedValue computation (the variables involved are hashValue and
normalizedValue) so the loop over variants (using cumulativePercentage and
variant.rolloutPercentage) will correctly match when rollouts total 100.
🪄 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
Run ID: 7ddb91da-a99f-4dc4-a168-700a81781218
📒 Files selected for processing (5)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/experiment/experiment.controller.tsbackend/apps/cloud/src/feature-flag/evaluation.tsbackend/apps/cloud/src/feature-flag/feature-flag.controller.tsbackend/apps/community/src/feature-flag/feature-flag.controller.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/cloud/src/feature-flag/feature-flag.controller.ts
- backend/apps/cloud/src/experiment/experiment.controller.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@backend/apps/community/src/feature-flag/feature-flag.controller.ts`:
- Around line 203-206: The Swagger ApiOperation description claims it “Evaluates
all feature flags” but the handler invokes findEnabledByProject(...), so update
the contract to match implementation: either change the description text in the
ApiOperation decorators to say “Evaluates enabled feature flags” or change the
lookup call from findEnabledByProject(...) to the method that returns all flags
(e.g., findByProject(...) or equivalent). Apply the same fix to the second
occurrence referenced (around the other ApiOperation at lines ~267-269) so
descriptions and query logic remain consistent.
- Around line 222-240: The per-project rate limiter
checkRateLimit(evaluateDto.pid, 'feature-flag-evaluate-project', 5000, 60) is
being executed before analyticsService.checkBot(...) so bot requests get charged
against the shared project bucket; modify the flow so you call
this.analyticsService.checkBot(...) first and, if botResult.isBot is true,
return { flags: {} } without invoking checkRateLimit, or alter checkRateLimit to
accept a skip/metadata parameter and short-circuit when botResult.isBot; ensure
you reference checkRateLimit, this.analyticsService.checkBot, and
evaluateDto.pid when changing the order or adding the skip logic.
🪄 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
Run ID: 3cef120e-c158-4d75-8947-e4a5deaea296
📒 Files selected for processing (3)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/feature-flag/evaluation.tsbackend/apps/community/src/feature-flag/feature-flag.controller.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/cloud/src/feature-flag/evaluation.ts
6 tasks
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
1 participant
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.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Performance