-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: Add logic to increment GroupTombstone hits asynchronously #93685
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
base: priscila/chore/add-hit-tracker-columns-to-grouptombstone
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## priscila/chore/add-hit-tracker-columns-to-grouptombstone #93685 +/- ##
============================================================================================
+ Coverage 88.03% 88.05% +0.01%
============================================================================================
Files 10331 10329 -2
Lines 596251 596082 -169
Branches 23150 23116 -34
============================================================================================
- Hits 524901 524867 -34
+ Misses 70899 70722 -177
- Partials 451 493 +42 |
1de5ba6
to
ca9ff99
Compare
ca9ff99
to
07b527b
Compare
07b527b
to
3a650ec
Compare
src/sentry/event_manager.py
Outdated
group_tombstone = GroupTombstone.objects.get(id=tombstone_id) | ||
buffer_incr( | ||
GroupTombstone, | ||
{"times_seen": group_tombstone.times_seen + 1 if group_tombstone.times_seen else 1}, |
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.
You don't want to include the current count here. Instead only pass 1
here. The buffers system will generate F(column) + value
when the buffered values are flushed to postgres.
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.
yes just noticed that. that is why the test broke :)
with mock.patch("sentry.event_manager.track_outcome", mock_track_outcome): | ||
with self.feature("organizations:event-attachments"): | ||
with self.tasks(): | ||
with self.feature("organizations:grouptombstones-hit-counter"): | ||
with pytest.raises(HashDiscarded): |
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.
You could combine all of these
with (
mock.patch(...),
self.feature(...)
):
which can help with the indentation levels.
…e' into priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone
…e' into priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone
…e' into priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone
…e' into priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone
@@ -294,6 +294,31 @@ def get_stored_crashreports(cache_key: str | None, event: Event, max_crashreport | |||
return query[:max_crashreports].count() | |||
|
|||
|
|||
def increment_group_tombstone_hit_counter(tombstone_id: int | None, event: Event) -> None: | |||
if tombstone_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.
nit: prefer return early to minimize code arrowing, e.g. if not tombstone_id return
Contributes to https://linear.app/getsentry/issue/TET-595/display-discarded-issue-count-and-timestamp-for-each-entry-in