Skip to content

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

Conversation

priscilawebdev
Copy link
Member

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

codecov bot commented Jun 17, 2025

Codecov Report

Attention: Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/event_manager.py 64.28% 5 Missing ⚠️
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     

@priscilawebdev priscilawebdev requested review from markstory and a team June 17, 2025 09:16
…lag' into priscila/chore/add-hit-tracker-columns-to-grouptombstone
@priscilawebdev priscilawebdev force-pushed the priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone branch from 1de5ba6 to ca9ff99 Compare June 17, 2025 14:00
@priscilawebdev priscilawebdev force-pushed the priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone branch from ca9ff99 to 07b527b Compare June 17, 2025 14:02
@priscilawebdev priscilawebdev force-pushed the priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone branch from 07b527b to 3a650ec Compare June 17, 2025 14:03
@priscilawebdev priscilawebdev removed the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 17, 2025
@getsentry getsentry deleted a comment from github-actions bot Jun 17, 2025
@priscilawebdev priscilawebdev marked this pull request as ready for review June 17, 2025 14:21
@priscilawebdev priscilawebdev requested a review from a team as a code owner June 17, 2025 14:21
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},
Copy link
Member

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.

Copy link
Member Author

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 :)

Comment on lines 2139 to 2143
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):
Copy link
Member

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.

@@ -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:
Copy link
Member

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

Base automatically changed from priscila/chore/add-hit-tracker-columns-to-grouptombstone to master June 23, 2025 06:35
@priscilawebdev priscilawebdev requested a review from a team as a code owner June 23, 2025 06:35
@priscilawebdev priscilawebdev force-pushed the priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone branch from 0149eb0 to 70c5882 Compare June 23, 2025 09:35
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0930_add_hit_counter_columns_to_grouptombstone.py

for 0930_add_hit_counter_columns_to_grouptombstone in sentry

--
-- 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;

@priscilawebdev priscilawebdev merged commit c2c41b0 into master Jun 23, 2025
64 checks passed
@priscilawebdev priscilawebdev deleted the priscila/feat/add-logic-to-update-hit-counter-columns-in-grouptomstone branch June 23, 2025 11:04
@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants