-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(issue detection): Add time-based commit filtering for suspect commit detection #97971
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #97971 +/- ##
==========================================
- Coverage 81.01% 80.95% -0.07%
==========================================
Files 8555 8567 +12
Lines 376380 377848 +1468
Branches 24119 24119
==========================================
+ Hits 304932 305871 +939
- Misses 71067 71596 +529
Partials 381 381 |
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.
some minor nits, looks good otherwise though!
@@ -96,7 +108,7 @@ def find_commit_context_for_event_all_frames( | |||
|
|||
_record_commit_context_all_frames_analytics( | |||
selected_blame=selected_blame, | |||
most_recent_blame=most_recent_blame, | |||
most_recent_blame=None, |
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.
New selection logic no longer keeps track of "most recent" so this metric is lost. Is it worth it to add the logic to track this piece?
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.
why do we need the most recent blame for these analytics?
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.
most_recent_blame
is used for 1 thing: determining the reason
that is added to the metrics, logs, and analytics.
I updated the looping function and analytics logic to account for the shift in functionality, now it can distinguish between commits being too recent, commits being too old, or both
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.
makes sense to me, just the question about the analytics
# Select best valid commit | ||
if selected_date < commit_date < group_first_seen: | ||
selected_blame = blame | ||
selected_date = commit_date |
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.
Bug: Commit Boundary Logic Excludes Relevant Commits
The commit selection logic in find_commit_context_for_event_all_frames
uses strict inequalities, which means commits exactly 60 days old or at the group_first_seen
timestamp are incorrectly excluded. This might cause us to miss relevant suspect commits at these boundaries.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues: Did you find this useful? React with a 👍 or 👎 |
…mmit detection (#97971) Replaces simple "most recent commit" selection with time-based filtering that: - Filters out commits that occurred after the issue was first seen (too young) - Filters out commits older than 60 days before the issue occurred (too old) - Processes commits in chronological order and stops early when hitting age threshold - Selects the most recent valid commit within the time window
…mmit detection (#97971) Replaces simple "most recent commit" selection with time-based filtering that: - Filters out commits that occurred after the issue was first seen (too young) - Filters out commits older than 60 days before the issue occurred (too old) - Processes commits in chronological order and stops early when hitting age threshold - Selects the most recent valid commit within the time window
…mmit detection (#97971) Replaces simple "most recent commit" selection with time-based filtering that: - Filters out commits that occurred after the issue was first seen (too young) - Filters out commits older than 60 days before the issue occurred (too old) - Processes commits in chronological order and stops early when hitting age threshold - Selects the most recent valid commit within the time window
Replaces simple "most recent commit" selection with time-based filtering that:
first_seen
on the Issue rather than time of assessment (.now()
)