-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(grouping): Add model parameter support for v2 similarity grouping rollout #102263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…g rollout Add backward-compatible `model` and `training_mode` parameters to Seer similarity requests. Introduces `projects:similarity-grouping-v2-model` Flagpole feature flag to control model version selection (defaults to v1). Creates `GroupingVersion` enum for type safety. Updates request builders, logging, and tests.
| sample_rate=options.get("seer.similarity.metrics_sample_rate"), | ||
| ): | ||
| # Get model configuration from feature flags | ||
| project = Project.objects.get(id=project_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Project.objects.get(id=project_id) lacks Project.DoesNotExist handling in send_group_and_stacktrace_to_seer() and send_group_and_stacktrace_to_seer_multithreaded().
Severity: CRITICAL | Confidence: 0.99
🔍 Detailed Analysis
The Project.objects.get(id=project_id) call within send_group_and_stacktrace_to_seer() and send_group_and_stacktrace_to_seer_multithreaded() does not include exception handling for Project.DoesNotExist. This condition arises if a project is deleted by an administrator after the backfill task initiates but prior to these functions attempting to retrieve the project. The absence of a handler for this exception causes the task to terminate unexpectedly, disrupting its execution. This behavior is inconsistent with existing codebase patterns for similar project lookups.
💡 Suggested Fix
Pass the project object directly to send_group_and_stacktrace_to_seer() and send_group_and_stacktrace_to_seer_multithreaded() to prevent redundant lookups, or implement a try-except block around Project.objects.get() to gracefully handle Project.DoesNotExist.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/tasks/embeddings_grouping/utils.py#L496
Potential issue: The `Project.objects.get(id=project_id)` call within
`send_group_and_stacktrace_to_seer()` and
`send_group_and_stacktrace_to_seer_multithreaded()` does not include exception handling
for `Project.DoesNotExist`. This condition arises if a project is deleted by an
administrator after the backfill task initiates but prior to these functions attempting
to retrieve the project. The absence of a handler for this exception causes the task to
terminate unexpectedly, disrupting its execution. This behavior is inconsistent with
existing codebase patterns for similar project lookups.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102263 +/- ##
============================================
+ Coverage 66.15% 80.75% +14.60%
============================================
Files 8758 8770 +12
Lines 389253 392810 +3557
Branches 24779 24779
============================================
+ Hits 257522 317227 +59705
+ Misses 131353 75205 -56148
Partials 378 378 |
| sample_rate=options.get("seer.similarity.metrics_sample_rate"), | ||
| ): | ||
| # Get model configuration from feature flags | ||
| project = Project.objects.get(id=project_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you can use Project.objects.get_from_cache
JoshFerge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment about project cache but lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a couple places where you're having to get the Project from the db just to check the option - take a look at the way Richard recently configured our option where we are also gating on project:
setup https://github.com/getsentry/sentry/pull/101609/files
usage https://github.com/getsentry/sentry/pull/101637/files#diff-f3bb390a27a81a7182610d29c9f93b343ac8fca32b700a5ad775ce48c8564982
other than that, you have some failing tests but lgtm otherwise!
23c129f to
680993f
Compare
…g rollout (#102263) Add backward-compatible `model` and `training_mode` parameters to Seer similarity requests. Introduces `projects:similarity-grouping-v2-model` Flagpole feature flag to control model version selection (defaults to v1). Creates `GroupingVersion` enum for type safety. Updates request builders, logging, and tests.
…g rollout (#102263) Add backward-compatible `model` and `training_mode` parameters to Seer similarity requests. Introduces `projects:similarity-grouping-v2-model` Flagpole feature flag to control model version selection (defaults to v1). Creates `GroupingVersion` enum for type safety. Updates request builders, logging, and tests.
Add backward-compatible
modelandtraining_modeparameters to Seer similarity requests. Introducesprojects:similarity-grouping-v2-modelFlagpole feature flag to control model version selection (defaults to v1). CreatesGroupingVersionenum for type safety. Updates request builders, logging, and tests.