feat(upsampling) - Support upsampled error count with performance optimizations #13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…(#94376) Part of the Error Upsampling project: https://www.notion.so/sentry/Tech-Spec-Error-Up-Sampling-1e58b10e4b5d80af855cf3b992f75894?source=copy_link Events-stats API will now check if all projects in the query are allowlisted for upsampling, and convert the count query to a sum over `sample_weight` in Snuba, this is done by defining a new SnQL function `upsampled_count()`. I noticed there are also eps() and epm() functions in use in this endpoint. I considered (and even worked on) also supporting swapping eps() and epm() which for correctness should probably also not count naively and use `sample_weight`, but this caused some complications and since they are only in use by specific dashboard widgets and not available in discover I decided to defer changing them until we realize it is needed.
- Add 60-second cache for upsampling eligibility checks to improve performance - Separate upsampling eligibility check from query transformation for better optimization - Remove unnecessary null checks in upsampled_count() function per schema requirements - Add cache invalidation utilities for configuration management This improves performance during high-traffic periods by avoiding repeated expensive allowlist lookups while maintaining data consistency.
Author
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # This cached result ensures consistent behavior across query execution | ||
| should_upsample = is_errors_query_for_error_upsampled_projects( | ||
| snuba_params, organization, dataset, request | ||
| ) |
There was a problem hiding this comment.
Bug: Wrong Dataset Causes Upsampling Errors
The _get_event_stats function uses dataset from the outer closure instead of the scoped_dataset parameter when checking upsampling eligibility. This causes incorrect behavior when the dashboard widget split logic passes a different dataset (e.g., discover) than the original request dataset (e.g., metrics_enhanced_performance). The upsampling check will evaluate against the wrong dataset, potentially applying or skipping transformations incorrectly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test 3
Replicated from ai-code-review-evaluation/sentry-cursor#3
Note
Adds error upsampling that transforms count() to upsampled_count for eligible error queries, integrates it into organization events stats with caching, and covers with tests.
organization_events_statsby conditionally transforming query columns (count()->upsampled_count() as count) for eligible error queries across top events and timeseries (RPC and non-RPC).sentry.api.helpers.error_upsampling):transform_query_columns_for_error_upsamplingfor column rewriting.upsampled_count()SnQL aggregate mapping totoInt64(sum(sample_weight)).Factories: setsample_ratefromcontexts.error_sampling.client_sample_ratefor stored events to support tests.Written by Cursor Bugbot for commit 6ad6fe3. Configure here.