-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: add limit to error tracking group events query #24511
Conversation
Size Change: 0 B Total Size: 1.08 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
@@ -131,6 +135,8 @@ export const errorTrackingGroupQuery = ({ | |||
dateRange: dateRange, | |||
filterGroup: filterGroup as PropertyGroupFilter, | |||
filterTestAccounts: filterTestAccounts, | |||
limit: limit, |
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.
can you add these params to a backend test as well so we can see the snapshot update? iirc there's no default order by clause, which means the offset can be arbitrary and non-deterministic, which can screw up our stats (example: same event is counted multiple times in different offsets, and some other event is completely discarded)
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.
there's no default order by clause, which means the offset can be arbitrary and non-deterministic
yeah pagination has to be deterministic
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Closing in favour of #24711 |
Problem
The error tracking is super slow because we don't limit the number of events we're fetching :/
Changes
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Add test to ensure new errors are merged into group