Skip to content

Conversation

roggenkemper
Copy link
Member

adds flag so we can roll out experimental n+1 api call detector.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 15, 2025
Copy link

codecov bot commented Sep 15, 2025

✅ All tests passed

@roggenkemper roggenkemper marked this pull request as ready for review September 16, 2025 16:47
@roggenkemper roggenkemper requested a review from a team as a code owner September 16, 2025 16:47
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, is the n_plus_one_api_calls_dectector in experiments/ going to be deleted in favor of the other one?

Copy link
Member Author

Choose a reason for hiding this comment

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

eventually - right now we need to keep it because we want to run the experimental detector for any org with the flag, but keep the legacy behavior for everyone else. once we fully move people over, we can replace the old file with this one

Comment on lines 81 to 82
def is_creation_allowed_for_project(self, project: Project) -> bool:
return self.settings["detection_enabled"]
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 used but not registered, causing it to always evaluate to False. This prevents the experimental detector from ever running.
  • Description: The feature flag organizations:experimental-n-plus-one-api-detector-rollout is used to control the rollout of the NPlusOneAPICallsExperimentalDetector. However, this flag is never registered in any configuration file, such as src/sentry/features/temporary.py. The feature management system defaults unregistered flags to False. As a result, the condition features.has("organizations:experimental-n-plus-one-api-detector-rollout", ...) will always be false, preventing the experimental detector from ever being executed. Conversely, the stable detector will always run, defeating the purpose of the gradual rollout mechanism introduced in this change.

  • Suggested fix: Register the feature flag organizations:experimental-n-plus-one-api-detector-rollout in src/sentry/features/temporary.py, similar to how other experimental detector flags like organizations:experimental-mn-plus-one-detector-rollout are registered. This will allow the flag to be enabled for organizations, enabling the experimental detector as intended.
    severity: 0.7, confidence: 0.95

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

@roggenkemper roggenkemper merged commit 6ee5c6e into master Sep 16, 2025
64 checks passed
@roggenkemper roggenkemper deleted the roggenkemper/expapidetector branch September 16, 2025 17:40
liuirene256 pushed a commit that referenced this pull request Sep 16, 2025
… flag (#99556)

adds flag so we can roll out experimental n+1 api call detector.
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.

2 participants