-
-
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
feat: Add logic to increment GroupTombstone hits asynchronously #93685
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #93685 +/- ##
==========================================
+ Coverage 88.04% 88.08% +0.04%
==========================================
Files 10358 10343 -15
Lines 598302 597697 -605
Branches 23239 23123 -116
==========================================
- Hits 526761 526502 -259
+ Misses 71073 70702 -371
- Partials 468 493 +25 |
…lag' into priscila/chore/add-hit-tracker-columns-to-grouptombstone
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.
src/sentry/event_manager.py
Outdated
@@ -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
…ter-columns-in-grouptomstone
0149eb0
to
70c5882
Compare
This PR has a migration; here is the generated SQL for for --
-- Add field last_seen to grouptombstone
--
ALTER TABLE "sentry_grouptombstone" ADD COLUMN "last_seen" timestamp with time zone DEFAULT '2025-06-23T09:37:22.724698+00:00'::timestamptz NULL;
ALTER TABLE "sentry_grouptombstone" ALTER COLUMN "last_seen" DROP DEFAULT;
--
-- Add field times_seen to grouptombstone
--
ALTER TABLE "sentry_grouptombstone" ADD COLUMN "times_seen" integer DEFAULT 0 NULL CHECK ("times_seen" >= 0);
ALTER TABLE "sentry_grouptombstone" ALTER COLUMN "times_seen" DROP DEFAULT; |
Contributes to https://linear.app/getsentry/issue/TET-595/display-discarded-issue-count-and-timestamp-for-each-entry-in