Skip to content

Conversation

roggenkemper
Copy link
Member

No description provided.

@roggenkemper roggenkemper marked this pull request as ready for review September 15, 2025 21:44
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 15, 2025
@roggenkemper roggenkemper requested a review from a team September 15, 2025 21:44
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #99544      +/-   ##
==========================================
- Coverage   81.23%   81.23%   -0.01%     
==========================================
  Files        8588     8588              
  Lines      380324   380325       +1     
  Branches    24128    24128              
==========================================
  Hits       308967   308967              
- Misses      70994    70995       +1     
  Partials      363      363              

Comment on lines +146 to 147
manager.add("organizations:experimental-n-plus-one-api-detector-rollout", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
# Enable GenAI features such as Autofix and Issue Summary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug: The feature flag organizations:experimental-n-plus-one-api-detector-rollout is defined but never used, causing both stable and experimental N+1 API detectors to run simultaneously.
  • Description: The new feature flag organizations:experimental-n-plus-one-api-detector-rollout is defined but is never checked in the code. Unlike the DB detectors which use a similar flag to enable an experimental version, both the stable NPlusOneAPICallsDetector and the NPlusOneAPICallsExperimentalDetector have is_creation_allowed_for_organization methods that always return True. Since both detectors are listed in DETECTOR_CLASSES, they will run simultaneously regardless of the flag's state. This prevents the intended A/B testing and gradual rollout of the experimental detector and may cause duplicate or conflicting performance issues to be generated.

  • Suggested fix: Implement the feature flag check in the is_creation_allowed_for_organization method of both the stable and experimental N+1 API call detectors. The stable detector should return not features.has(...) and the experimental one should return features.has(...), mirroring the pattern used by the N+1 DB detectors. This will ensure only one of the two detectors is active at any time based on the flag.
    severity: 0.65, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

@roggenkemper roggenkemper merged commit 31c851b into master Sep 16, 2025
65 checks passed
@roggenkemper roggenkemper deleted the roggenkemper/expapiflag branch September 16, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants