Skip to content
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(issue-platform): Support passing fingerprints with multiple hashes when sending occurrences #87311

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

wedamija
Copy link
Member

This introduces handling for fingerprints with multiple components to the issue platform. This works in the same way that events do - we try to fetch grouphashes for each component, where earlier hashes have priority. If none exist, create a new group and associated both hashes with that group.

If there are multiple groups associated with the hashes, then associate this occurrence to the first group found, and don't change the hash association. This shouldn't happen in the issue platform in general, but worth being cautious.

I will follow up with another pr to handle this for status changes as well.

…es when sending occurrences

This introduces handling for fingerprints with multiple components to the issue platform. This works in the same way that events do - we try to fetch grouphashes for each component, where earlier hashes have priority. If none exist, create a new group and associated both hashes with that group.

If there are multiple groups associated with the hashes, then associate this occurrence to the first group found, and don't change the hash association. This shouldn't happen in the issue platform in general, but worth being cautious.

I will follow up with another pr to handle this for status changes as well.
@wedamija wedamija requested review from a team as code owners March 18, 2025 19:07
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 18, 2025
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

Maybe (probably) this ship has sailed long since, but as somebody quite familiar with our regular grouping process, I'm finding this code a little confusing because of the fact that the field called fingerprint includes hashes rather than the fingerprint. Any chance of aligning the naming?

(Even if we can't change the schema field name, could we change the variable names in the code here, so we're referring to things as just "hashes" rather than "fingerprint" or "fingerprint_hashes?" I'll grant it's not a huge deal, but they do mean distinctly different things on the error-grouping side.)

Otherwise this change looks good.

@wedamija
Copy link
Member Author

Maybe (probably) this ship has sailed long since, but as somebody quite familiar with our regular grouping process, I'm finding this code a little confusing because of the fact that the field called fingerprint includes hashes rather than the fingerprint. Any chance of aligning the naming?

(Even if we can't change the schema field name, could we change the variable names in the code here, so we're referring to things as just "hashes" rather than "fingerprint" or "fingerprint_hashes?" I'll grant it's not a huge deal, but they do mean distinctly different things on the error-grouping side.)

Otherwise this change looks good.

Isn't that how the error fingerprints work too? We have a fingerprint, which is an array of strings, where each string in the array might map to a hash value in GroupHash?

wedamija added a commit that referenced this pull request Mar 19, 2025
…es when sending status updates

This is a follow up to #87311

We now allow status updates with multiple hashes to be sent to the issue platform. We have a shared function here with statistical detectors, which I've also updated to pass work with full fingerprints. There should be no change to how the detector works.
Copy link
Member

@JoshFerge JoshFerge left a 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.

@wedamija wedamija merged commit 476f26f into master Mar 19, 2025
48 checks passed
@wedamija wedamija deleted the danf/issue-platform-multiple-fingerprint branch March 19, 2025 19:35
wedamija added a commit that referenced this pull request Mar 19, 2025
…es when sending status updates

This is a follow up to #87311

We now allow status updates with multiple hashes to be sent to the issue platform. We have a shared function here with statistical detectors, which I've also updated to pass work with full fingerprints. There should be no change to how the detector works.
wedamija added a commit that referenced this pull request Mar 19, 2025
…es when sending status updates

This is a follow up to #87311

We now allow status updates with multiple hashes to be sent to the issue platform. We have a shared function here with statistical detectors, which I've also updated to pass work with full fingerprints. There should be no change to how the detector works.
Copy link

sentry-io bot commented Mar 20, 2025

Suspect Issues

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

  • ‼️ DataError: StringDataRightTruncation('value too long for type character varying(200)\n') issues.occurrence_consumer View Issue

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

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.

3 participants