-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore: use $exception_issue_id #26488
Conversation
const where = [ | ||
`has(${stringifyFingerprints( | ||
fingerprints | ||
)}, JSONExtract(ifNull(properties.$exception_fingerprint,'[]'),'Array(String)'))`, | ||
] | ||
// TODO: fix this where clause. It does not take into account the events | ||
// associated with issues that have been merged into this primary issue | ||
const where = [`eq(${issueId}, properties.$exception_issue_id`] |
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.
Need to come back to this but think it's ok for now. Basically we piggy back on top of the EventsQuery
to fetch individual exceptions for an issue. Because we no longer have all the merged_fingerprints
associated with an issue model in PG we cannot filter the query.
I think what we'll end up doing here is returning issue_ids
as a field on the ErrorTrackingIssueQueryResponse
object (computed as groupUniqArray(properties.$exception_issue_id)
) which can be reused to fetch individual events. I just want to confirm that the groupUniqArray
doesn't make the query slow
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.
We'll need to hit the overrides table here, right? Same kind of query as getting the events for a person profile?
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Size Change: -222 B (-0.02%) Total Size: 1.16 MB ℹ️ View Unchanged
|
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.
Kind of stumbling in the dark on a lot of the query stuff here but overall seems reasonable
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Problem
$exception_fingerprint
is no longer the unique identifier of an issueChanges
Note: this does not support issue merging via the overrides table. That will come in a follow up PR. I thought it would be simpler to do a direct renaming PR first
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Updated all the tests. We need to create an
ErrorTrackingIssue
for each event we have as we no longer lazily create those models