Skip to content

Conversation

@nora-shap
Copy link
Member

@nora-shap nora-shap commented Aug 29, 2025

Summary

Removes the legacy release-based fallback logic from the event file committers endpoint, simplifying it to use only GroupOwner-based suspect commit detection.

Current behavior: If there are no suspect commits for the event, do a live analysis using the legacy strategy, do not save any of the commits it finds as suspect, return as many as it finds.

💡 : it was doing this because it was the only way to show a suspect commit if the Author was NOT a User of their org. The legacy strategy does not allow GroupOwner creation if there is not an associated User, it ignores those. This does not match the behavior of the non-legacy strategy, which allows GroupOwners with user=None and all serializers have fallback logic to display details from the CommitAuthor object if there is no User. The legacy method needs to be updated to allow GroupOwners with user=None.

Proposed changes: no live analysis - only GroupOwners created during original analysis will be retuned.

While my intention has been to create fewer but better suspect commits, with the logic in this flow meant I was actually displaying a lot more legacy strategy suspect commits, which is not what I want.

Changes Made

Core Logic Changes

  • Removed legacy fallback: get_serialized_event_file_committers now only calls _get_serialized_committers_from_group_owners
  • Dynamic suspect commit type: Uses suspectCommitStrategy from GroupOwner context instead of hardcoding INTEGRATION_COMMIT
    • RELEASE_BASED"via commit in release"
    • SCM_BASED"via SCM integration"
  • Preserved API contract: Still returns 404 when no committers found (not empty array)

Behavior Changes

Before

# 1. Check GroupOwners first
# 2. If no GroupOwners, fall back to release-based analysis (legacy logic)
# 3. Analyze stacktrace frames against recent release commits  
# 4. Always returned "via SCM integration" regardless of actual strategy
# 5. Any number of suspect commits can be returned

After

# 1. Check GroupOwners only - all suspect commit strategies create GroupOwners when the Issue is first processed
# 2. If no GroupOwners, return 404 "No committers found"
# 3. Use actual commit strategy from GroupOwner context
# 4. Only 1 suspect commit can be returned

@nora-shap nora-shap requested a review from a team August 29, 2025 00:29
@nora-shap nora-shap requested a review from a team as a code owner August 29, 2025 00:29
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 29, 2025
@codecov
Copy link

codecov bot commented Aug 29, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/integrations/utils/commit_context.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #98504      +/-   ##
==========================================
- Coverage   81.18%   80.06%   -1.13%     
==========================================
  Files        8544     8549       +5     
  Lines      376576   387896   +11320     
  Branches    23954    23954              
==========================================
+ Hits       305719   310562    +4843     
- Misses      70492    76969    +6477     
  Partials      365      365              

Copy link
Contributor

@thetruecpaul thetruecpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall — please fix the failing tests & typing jobs

@nora-shap nora-shap force-pushed the nora/kill-legacy-strategy branch from af0c37c to 390cb1d Compare September 3, 2025 21:49
@nora-shap nora-shap enabled auto-merge (squash) September 3, 2025 22:01
@nora-shap nora-shap merged commit b011ca7 into master Sep 3, 2025
64 checks passed
@nora-shap nora-shap deleted the nora/kill-legacy-strategy branch September 3, 2025 22:11
@sentry
Copy link

sentry bot commented Sep 4, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎

armenzg pushed a commit that referenced this pull request Sep 5, 2025
…ly behavior (#98504)

## Summary
Removes the legacy release-based fallback logic from the event file
committers endpoint, simplifying it to use only GroupOwner-based suspect
commit detection.

Current behavior: If there are no suspect commits for the event, do a
live analysis using the legacy strategy, do not save any of the commits
it finds as suspect, return as many as it finds.

💡 : it was doing this because it was the only way to show a suspect
commit if the Author was NOT a User of their org. The legacy strategy
does not allow GroupOwner creation if there is not an associated User,
it ignores those. This does not match the behavior of the non-legacy
strategy, which allows GroupOwners with `user=None` and all serializers
have fallback logic to display details from the `CommitAuthor` object if
there is no `User`. The legacy method needs to be updated to allow
GroupOwners with `user=None`.

Proposed changes: no live analysis - only GroupOwners created during
original analysis will be retuned.

While my intention has been to create _fewer_ but _better_ suspect
commits, with the logic in this flow meant I was actually displaying a
lot more legacy strategy suspect commits, which is not what I want.

## Changes Made

### Core Logic Changes
- **Removed legacy fallback**: `get_serialized_event_file_committers`
now only calls `_get_serialized_committers_from_group_owners`
- **Dynamic suspect commit type**: Uses `suspectCommitStrategy` from
GroupOwner context instead of hardcoding `INTEGRATION_COMMIT`
  - `RELEASE_BASED` → `"via commit in release"`  
  - `SCM_BASED` → `"via SCM integration"`
- **Preserved API contract**: Still returns 404 when no committers found
(not empty array)

## Behavior Changes

### Before
```python
# 1. Check GroupOwners first
# 2. If no GroupOwners, fall back to release-based analysis (legacy logic)
# 3. Analyze stacktrace frames against recent release commits  
# 4. Always returned "via SCM integration" regardless of actual strategy
# 5. Any number of suspect commits can be returned
```

### After  
```python
# 1. Check GroupOwners only - all suspect commit strategies create GroupOwners when the Issue is first processed
# 2. If no GroupOwners, return 404 "No committers found"
# 3. Use actual commit strategy from GroupOwner context
# 4. Only 1 suspect commit can be returned
```
andrewshie-sentry pushed a commit that referenced this pull request Sep 9, 2025
…ly behavior (#98504)

## Summary
Removes the legacy release-based fallback logic from the event file
committers endpoint, simplifying it to use only GroupOwner-based suspect
commit detection.

Current behavior: If there are no suspect commits for the event, do a
live analysis using the legacy strategy, do not save any of the commits
it finds as suspect, return as many as it finds.

💡 : it was doing this because it was the only way to show a suspect
commit if the Author was NOT a User of their org. The legacy strategy
does not allow GroupOwner creation if there is not an associated User,
it ignores those. This does not match the behavior of the non-legacy
strategy, which allows GroupOwners with `user=None` and all serializers
have fallback logic to display details from the `CommitAuthor` object if
there is no `User`. The legacy method needs to be updated to allow
GroupOwners with `user=None`.

Proposed changes: no live analysis - only GroupOwners created during
original analysis will be retuned.

While my intention has been to create _fewer_ but _better_ suspect
commits, with the logic in this flow meant I was actually displaying a
lot more legacy strategy suspect commits, which is not what I want.

## Changes Made

### Core Logic Changes
- **Removed legacy fallback**: `get_serialized_event_file_committers`
now only calls `_get_serialized_committers_from_group_owners`
- **Dynamic suspect commit type**: Uses `suspectCommitStrategy` from
GroupOwner context instead of hardcoding `INTEGRATION_COMMIT`
  - `RELEASE_BASED` → `"via commit in release"`  
  - `SCM_BASED` → `"via SCM integration"`
- **Preserved API contract**: Still returns 404 when no committers found
(not empty array)

## Behavior Changes

### Before
```python
# 1. Check GroupOwners first
# 2. If no GroupOwners, fall back to release-based analysis (legacy logic)
# 3. Analyze stacktrace frames against recent release commits  
# 4. Always returned "via SCM integration" regardless of actual strategy
# 5. Any number of suspect commits can be returned
```

### After  
```python
# 1. Check GroupOwners only - all suspect commit strategies create GroupOwners when the Issue is first processed
# 2. If no GroupOwners, return 404 "No committers found"
# 3. Use actual commit strategy from GroupOwner context
# 4. Only 1 suspect commit can be returned
```
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 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.

3 participants