Repository navigation
feat(upsampling) - Support upsampled error count with performance optimizations - #1833
kaihao-zhao wants to merge 3 commits into
Conversation
…(#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.
![unblocked-local-kaihao[bot]](https://avatars.githubusercontent.com/u/304571795?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 issues found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked-local-kaihao to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| if "event.type:error" in query: | ||
| return True |
There was a problem hiding this comment.
This substring also matches !event.type:error, event.type:error OR event.type:transaction, and message:"event.type:error". Those queries are not restricted to errors, but their count() axes will now be replaced with error-weight aggregation. Inspect the parsed filter and its boolean structure rather than treating the presence of this text as an error-only constraint.
| if column_lower == "count()": | ||
| # Transform to upsampled count - assumes sample_weight column exists | ||
| # for all events in allowlisted projects per our data model requirements | ||
| transformed_columns.append("upsampled_count() as count") |
There was a problem hiding this comment.
For an eligible non-top-events request with yAxis=count() and transformAliasToInputFormat=1, process_results() translates the count alias to upsampled_count() as count. The endpoint still passes the original count() field to the serializer, whose missing-field lookup returns zero even when the aggregate is nonzero. Preserve the original input-to-result mapping when rewriting the aggregation.
| if column_lower == "count()": | ||
| # Transform to upsampled count - assumes sample_weight column exists | ||
| # for all events in allowlisted projects per our data model requirements | ||
| transformed_columns.append("upsampled_count() as count") |
There was a problem hiding this comment.
A top-events request with field=["count()", "message"] and yAxis=count() combines the original count() with upsampled_count() as count. TopEventsQueryBuilder retains both expressions, and aggregate resolution appends both under the count alias while overwriting the single function_alias_map entry for that alias. Count-based equations also auto-add ordinary count() alongside the rewritten aggregate. Resolve these references consistently so distinct aggregates do not share one result alias and input mapping.
| ) | ||
| return scoped_dataset.top_events_timeseries( | ||
| timeseries_columns=query_columns, | ||
| timeseries_columns=final_columns, |
There was a problem hiding this comment.
The timeseries uses weighted counts, but selected_columns and orderby still use the original count() when fetching the top groups. For example, a group with one event of weight 100 loses to a group with two events of weight 1 when topEvents=1, despite having the larger displayed count. Apply the eligible aggregation consistently to the top-group selection and ordering.
| # Early upsampling eligibility check for performance optimization | ||
| # 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.
dataset is captured from the original request, whereas _get_event_stats() can execute a different scoped_dataset. The dashboard-splitting paths call this function with discover from a metrics-enhanced request. Even with an explicit event.type:error filter and fully allowlisted projects, eligibility is checked against the metrics dataset and returns false, leaving the actual error query unweighted. Pass scoped_dataset to the eligibility check.
| expensive repeated option lookups during high-traffic periods. This is safe | ||
| because allowlist changes are infrequent and eventual consistency is acceptable. | ||
| """ | ||
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" |
There was a problem hiding this comment.
Different project sets can share this key and reuse each other’s eligibility. For example, hash((1,)) == hash((2**61,)), and both IDs fit the model’s signed 64-bit field. Within one organization, caching eligibility for the allowlisted first project then incorrectly permits the non-allowlisted second project. Include the normalized IDs themselves or use a collision-resistant digest of their serialization. Apply the same key construction to invalidation.
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" | ||
|
|
||
| # Check cache first for performance optimization | ||
| cached_result = cache.get(cache_key) |
There was a problem hiding this comment.
Every invocation of the stats query callback performs an eligibility cache lookup, and misses perform a write, before checking whether the dataset supports upsampling. This also happens for axes the transformer cannot change. These are unnecessary shared-cache operations when using the conditional self-hosted Memcached configuration; options.get() already checks an in-process cache first. Reject unsupported datasets and non-transformable axes before accessing the eligibility cache.
| SnQLFunction( | ||
| "upsampled_count", | ||
| required_args=[], |
There was a problem hiding this comment.
SnQLFunction defaults to public access, and this converter is shared with the Events and Transactions datasets. A caller can therefore request yAxis=upsampled_count() directly and bypass both the all-project allowlist check and the dataset restriction. The automatic rewrite is not sufficient to enforce those preconditions. Restrict direct access or validate eligibility when resolving the aggregate itself.
| # exists for all events in allowlisted projects as per schema design | ||
| snql_aggregate=lambda args, alias: Function( | ||
| "toInt64", | ||
| [Function("sum", [Column("sample_weight")])], |
There was a problem hiding this comment.
A status-filtered errors query joins events to group_attributes, and ErrorsQueryBuilder assigns an entity to event columns through column(). The new aggregate constructs Column("sample_weight") directly, leaving that column unqualified in the generated joined query instead of using the existing event-entity resolution. Use self.builder.column("sample_weight") to preserve that qualification; backend rejection is not established by the available code.
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" | ||
|
|
||
| # Check cache first for performance optimization | ||
| cached_result = cache.get(cache_key) |
There was a problem hiding this comment.
Both cache.get() here and cache.set() below run without a fallback. With an operator-configured backend that propagates connection or timeout errors, this optional optimization aborts the request before query execution—even for unsupported datasets. The supplied self-hosted configuration ignores cache exceptions, but other deployed backend settings could not be verified. Catch cache failures and perform the eligibility check without caching, as the options store already does for its cache operations.
2 participants
See title.