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

Open
wants to merge 9 commits into
base: priscila/chore/add-hit-tracker-columns-to-grouptombstone
Choose a base branch
from

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 89.18919% with 4 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 69.23% 4 Missing ⚠️
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     

@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.

@priscilawebdev priscilawebdev requested a review from markstory June 17, 2025 15:52
…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:
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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