-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(seer grouping): Call Seer before creating a new group #71026
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
feat(seer grouping): Call Seer before creating a new group #71026
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
17bef00
to
17b2bb7
Compare
7df4e9e
to
49ec31c
Compare
17b2bb7
to
90f52bb
Compare
ebf05ed
to
2d9484b
Compare
90f52bb
to
42793eb
Compare
2d9484b
to
b2bc90b
Compare
42793eb
to
5a1d263
Compare
This adds two helpers, `should_call_seer_for_grouping` and `get_seer_similar_issues`, to be used when we (maybe) call Seer as part of event ingestion. `should_call_seer_for_grouping` does exactly what you'd think given the name, right now only basing the decision on feature flags and whether or not the event has a usable title and/or stacktrace. In the future we'll also include rate limit and killswitch checks, and any other criteria which it makes sense to add. `get_seer_similar_issues` is a wrapper around `get_similarity_data_from_seer` (which is what actually makes the API call to Seer). It extracts request data from the given event, makes the request, pulls together metadata about the results, and if a matching group is found and the flag is on, pulls the `Group` record out of the database. (I chose to put the feature flag check there rather than in the code where the the the grouping actually happens so that we can save the trip to the database if we're not going to end up using the results for grouping.) Code to actually use these helpers is added in #71026.
5a1d263
to
9d64934
Compare
# We only want to add this data to new groups, while we're testing | ||
# TODO: Remove this once we're out of the testing phase |
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.
Wouldn't we want to keep that for debugging even after testing phase?
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.
We might end up keeping it, but the current plan is to take it out eventually. For debugging purposes, we have the same information on the event hopefully added to the corresponding GroupHash
record. (See #70454.)
src/sentry/event_manager.py
Outdated
is_new = False if seer_matched_group else True | ||
is_regression = ( | ||
False | ||
if is_new | ||
else _process_existing_aggregate( | ||
# If `seer_matched_group` were `None`, `is_new` would be true and we | ||
# wouldn't be here | ||
group=NonNone(seer_matched_group), | ||
event=event, | ||
incoming_group_values=group_creation_kwargs, | ||
release=release, | ||
) | ||
) |
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.
purely opinion based, so entirely up to you, but I find just regular ifs more readable than using conditional expressions multiple times.
is_new = False if seer_matched_group else True | |
is_regression = ( | |
False | |
if is_new | |
else _process_existing_aggregate( | |
# If `seer_matched_group` were `None`, `is_new` would be true and we | |
# wouldn't be here | |
group=NonNone(seer_matched_group), | |
event=event, | |
incoming_group_values=group_creation_kwargs, | |
release=release, | |
) | |
) | |
if is_new := not seer_matched_group: | |
is_regression = False | |
outcome = "new_group" | |
else: | |
is_regression = _process_existing_aggregate( | |
# If `seer_matched_group` were `None`, `is_new` would be true and we | |
# wouldn't be here | |
group=NonNone(seer_matched_group), | |
event=event, | |
incoming_group_values=group_creation_kwargs, | |
release=release, | |
) | |
outcome = "seer_match" |
and below...
span.set_tag("outcome", outcome)
metric_tags["outcome"] = outcome
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.
It's true, I do love a good ternary. 🙂 I do sometimes get the sense that they're a little more idiomatic in JS/TS (which is - for better or worse - very much my first love, programming-language-wise) than in Python. Also for whatever reason it actually does make more sense to my brain. But you're right that there's a time and a place, and I do probably lean into it a little hard sometimes.
I'm going to leave it as is for now. [UPDATE: Partially true - I did change to is_new = not seer_matched_group
.] It works, and there's lots more to do, but I do take the overall point.
(FWIW, the structure of _save_aggregate_new
is different enough that when I port this there, I think I am going to have to structure it more with if
s than with ternaries.)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #71026 +/- ##
===========================================
+ Coverage 56.77% 77.90% +21.12%
===========================================
Files 6516 6525 +9
Lines 290322 290617 +295
Branches 50236 50286 +50
===========================================
+ Hits 164834 226399 +61565
+ Misses 120747 57971 -62776
- Partials 4741 6247 +1506 |
"seer_similarity" | ||
] = seer_response_data | ||
|
||
except Exception as e: |
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.
do we know what exception types we're catching here? or is this meant to be a generic catch so that we don't break anything and lose events?
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.
Much more the latter. Any reasonably-anticipatable errors in that logic are caught before they bubble up this high, but the last thing I want to do is break ingestion, so it seemed wise to try-catch it just in case. (But yeah, I can add a comment, because it's a reasonable question. [UPDATE: Done.])
9d64934
to
47d36a3
Compare
47d36a3
to
a60dc00
Compare
This adds two helpers, `should_call_seer_for_grouping` and `get_seer_similar_issues`, to be used when we (maybe) call Seer as part of event ingestion. `should_call_seer_for_grouping` does exactly what you'd think given the name, right now only basing the decision on feature flags and whether or not the event has a usable title and/or stacktrace. In the future we'll also include rate limit and killswitch checks, and any other criteria which it makes sense to add. `get_seer_similar_issues` is a wrapper around `get_similarity_data_from_seer` (which is what actually makes the API call to Seer). It extracts request data from the given event, makes the request, pulls together metadata about the results, and if a matching group is found and the flag is on, pulls the `Group` record out of the database. (I chose to put the feature flag check there rather than in the code where the the the grouping actually happens so that we can save the trip to the database if we're not going to end up using the results for grouping.) Code to actually use these helpers is added in #71026.
This uses the helpers added in #70999 to - depending on the state of the `projects:similarity-embeddings-metadata` and `projects:similarity-embeddings-grouping` flags - decide whether we should call Seer before creating a new group, make the API call if so, and then store the results and/or use them to actually prevent new group creation in favor of using an existing similar issue. The behavior is as follows: | metadata | grouping | call | metadata in | metadata in | use Seer-matched | | flag | flag | Seer? | event? | group? | group, if any? | |-----------|----------|-------|-------------|-------------|------------------| | off | off | no | - | - | - | | on | off | yes | yes * | yes | no | | on or off | on | yes | yes * | only if new | yes | * For now, the only event with the data will be the event which triggers the Seer call, not subsequent events with that hash. In the long run we will probably need to store the data on the `GroupHash` record itself. See #70454. This should be enough for us to run a POC on S4S and measure the effect on grouping.
This uses the helpers added in #70999 to - depending on the state of the
projects:similarity-embeddings-metadata
andprojects:similarity-embeddings-grouping
flags - decide whether we should call Seer before creating a new group, make the API call if so, and then store the results and/or use them to actually prevent new group creation in favor of using an existing similar issue. The behavior is as follows:This should be enough for us to run a POC on S4S and measure the effect on grouping.