feat(upsampling) - Support upsampled error count with performance optimizations - #1
feat(upsampling) - Support upsampled error count with performance optimizations#1linxia0415 wants to merge 2 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.
📝 WalkthroughWalkthroughThis PR implements error upsampling for Sentry's query system by determining which projects should apply upsampled count aggregations, rewriting query columns to use an ChangesError Upsampling Feature
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant Endpoint as OrganizationEventsStatsEndpoint
participant Helper as error_upsampling helpers
participant Cache as Eligibility Cache
participant Snuba as Snuba Query Engine
Client->>Endpoint: GET /organization-events-stats?yAxis=count()
Endpoint->>Helper: is_errors_query_for_error_upsampled_projects()
Helper->>Cache: Check cached eligibility (org, project_ids)
alt Cache Miss
Helper->>Helper: _are_all_projects_error_upsampled()
Helper->>Helper: _should_apply_sample_weight_transform()
Helper->>Cache: Store result (60s TTL)
end
Cache-->>Helper: eligibility boolean
alt Eligible for Upsampling
Endpoint->>Helper: transform_query_columns_for_error_upsampling()
Helper-->>Endpoint: [upsampled_count() as count, ...]
Endpoint->>Snuba: execute with upsampled columns
else Not Eligible
Endpoint->>Snuba: execute with original columns
end
Snuba-->>Endpoint: timeseries results
Endpoint-->>Client: JSON response with counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/api/endpoints/organization_events_stats.py (1)
229-271:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTop-events still ranks by raw
count().Only
y_axes/timeseries_columnsare rewritten here.orderbyand the selected top-events fields still come from the unmodified request, sotopEventscan choose and bucket series using raw counts while rendering upsampled counts. That gives incorrect top-N ordering whenever sample weights differ across groups.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/api/endpoints/organization_events_stats.py` around lines 229 - 271, The top-N ordering still uses raw count fields so when upsampling is enabled you must rewrite the orderby and selected top-event fields to the upsampled equivalents alongside y_axes/timeseries_columns; update the use_rpc branch (scoped_dataset.run_top_events_timeseries_query) to pass an adjusted orderby and raw_groupby/selected columns derived from transform_query_columns_for_error_upsampling (or a mapping from final_columns) instead of get_orderby(request)/get_field_list(...), and likewise update the non-RPC call to pass transformed selected_columns and orderby to scoped_dataset.top_events_timeseries (replace selected_columns=self.get_field_list(...) and orderby=self.get_orderby(request) with their upsampled equivalents), ensuring any alias->input-format transformation is applied consistently so top-N selection uses the same upsampled metric as the timeseries.
🧹 Nitpick comments (2)
tests/snuba/api/endpoints/test_organization_events_stats.py (1)
3604-3722: ⚡ Quick winAdd one
topEventsregression case for upsampled counts.These tests only cover the plain timeseries path. The endpoint has separate top-events branches, so a
topEvents=...case with unequal sample weights would catch the ranking/"Other" regressions there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/snuba/api/endpoints/test_organization_events_stats.py` around lines 3604 - 3722, Add a new test method (e.g., test_error_upsampling_top_events_regression) alongside the existing ones that patches sentry.api.helpers.error_upsampling.options, sets the allowlist to include both projects (mock_options.get.return_value = [self.project.id, self.project2.id]), stores multiple error events with differing sample weights (via self.store_event using distinct event_ids and sample_weight/context to create unequal effective counts) across the two time buckets, then calls the endpoint at self.url with the same start/end/interval/yAxis="count()" and include topEvents=<N> in the query and project list; assert response.status_code == 200 and check response.data["data"] for two buckets that the topEvents list is ranked by upsampled counts and that the "Other" entry (if present) reflects the sum of upsampled counts for non-top events to catch ranking/"Other" regressions.tests/sentry/api/helpers/test_error_upsampling.py (1)
54-76: ⚡ Quick winAdd test cases for column expressions that should NOT be transformed.
The current tests verify that
count()is transformed, but don't verify that similar expressions likecount(id)orcount(distinct id)are left unchanged. Based on the upstream implementation, only exactcount()(case-insensitive, after strip) should be transformed.📝 Suggested test additions
def test_transform_query_columns_for_error_upsampling(self) -> None: # ... existing tests ... # Test that count with arguments is NOT transformed columns = ["count(id)", "count(distinct id)", "count(*)"] result = transform_query_columns_for_error_upsampling(columns) assert result == columns # Should remain unchanged # Test mixed columns with multiple count() columns = ["count()", "other_column", "count()"] expected = [ "upsampled_count() as count", "other_column", "upsampled_count() as count", ] assert transform_query_columns_for_error_upsampling(columns) == expected🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sentry/api/helpers/test_error_upsampling.py` around lines 54 - 76, Add tests to ensure transform_query_columns_for_error_upsampling only transforms exact "count()" (case-insensitive after strip) and leaves other count forms unchanged: add a case with columns = ["count(id)", "count(distinct id)", "count(*)"] asserting the result equals the original list, and add a mixed case with ["count()", "other_column", "count()"] asserting the two count() entries are transformed to "upsampled_count() as count" while other_column remains unchanged; reference the transform_query_columns_for_error_upsampling function in the test additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sentry-repo`:
- Line 1: This PR mixes an unrelated submodule bump (commit
a5d290951def84afdcc4c88d2f1f20023fc36e2a) that introduces OTLP span-link
ingestion with the error-upsampling changes; separate them by removing the
submodule update from this branch/commit and creating a new PR containing only
the submodule bump (or, if truly required, add an explicit justification and
dependency note to this PR). Concretely: revert or drop the submodule reference
change tied to commit a5d290951def84afdcc4c88d2f1f20023fc36e2a from this branch,
create a new branch/PR that applies just that submodule update, run CI/tests on
both branches, and if you keep the bump here, add a short rationale in the PR
description explaining the hard dependency on OTLP span-link ingestion.
In `@src/sentry/api/endpoints/organization_events_stats.py`:
- Around line 220-222: The upsampling check is using the outer dataset and
request state instead of the effective query/dataset used to call Snuba; update
the call to is_errors_query_for_error_upsampled_projects inside
_get_event_stats() to pass the rewritten/effective values from snuba_params
(e.g., the effective query and dataset stored on snuba_params) and organization
instead of using the outer dataset and request.GET["query"], so eligibility is
evaluated against the actual query sent to Snuba.
In `@src/sentry/api/helpers/error_upsampling.py`:
- Around line 27-38: The cache key uses Python's process-salted hash which is
non-deterministic across workers; replace
hash(tuple(sorted(snuba_params.project_ids))) with a deterministic
representation (e.g., join the sorted project IDs into a string or a stable
digest like hashlib.sha256 of the joined IDs) when building cache_key in the
error upsampling flow (the block that computes cache_key and uses
cache.get/cache.set) and apply the same deterministic construction in the other
occurrence referenced (the lines around the second use). Ensure you keep the
sort to make ordering stable, reference snuba_params.project_ids, and update any
related helpers such as _are_all_projects_error_upsampled and
invalidate_upsampling_cache usage to use the same deterministic key construction
so invalidation works across workers.
In `@src/sentry/search/events/datasets/discover.py`:
- Around line 1041-1051: The SnQLFunction "upsampled_count" currently sets
default_result_type="number" but returns toInt64(sum(sample_weight)); update the
SnQLFunction definition for "upsampled_count" to use an integer result type
(e.g., default_result_type="integer") so its advertised type matches normal
count() responses and downstream metadata consumers remain consistent.
In `@src/sentry/testutils/factories.py`:
- Around line 347-357: The current bare excepts around extracting
client_sample_rate and converting it to float should be replaced with specific
exceptions: when retrieving nested keys from normalized_data (the expression
using normalized_data.get("contexts", {}).get("error_sampling",
{}).get("client_sample_rate")), catch only AttributeError and TypeError (to
handle cases where contexts or error_sampling are not mappings) instead of
Exception; when assigning normalized_data["sample_rate"] =
float(client_sample_rate) catch only ValueError and TypeError (to handle invalid
string/None conversions). Update the try/except blocks around client_sample_rate
and the float conversion accordingly, referencing normalized_data and
client_sample_rate/sample_rate to locate the code.
- Around line 353-357: The truthiness check using "if client_sample_rate:"
prevents valid zero values from being applied; change the guard to an explicit
None check (e.g., "if client_sample_rate is not None:") in the block that sets
normalized_data["sample_rate"], then attempt to coerce client_sample_rate to
float (catching ValueError/TypeError) and assign it to
normalized_data["sample_rate"] so 0 and 0.0 are preserved; reference the
normalized_data dict and the client_sample_rate variable in the sample-rate
assignment logic.
---
Outside diff comments:
In `@src/sentry/api/endpoints/organization_events_stats.py`:
- Around line 229-271: The top-N ordering still uses raw count fields so when
upsampling is enabled you must rewrite the orderby and selected top-event fields
to the upsampled equivalents alongside y_axes/timeseries_columns; update the
use_rpc branch (scoped_dataset.run_top_events_timeseries_query) to pass an
adjusted orderby and raw_groupby/selected columns derived from
transform_query_columns_for_error_upsampling (or a mapping from final_columns)
instead of get_orderby(request)/get_field_list(...), and likewise update the
non-RPC call to pass transformed selected_columns and orderby to
scoped_dataset.top_events_timeseries (replace
selected_columns=self.get_field_list(...) and orderby=self.get_orderby(request)
with their upsampled equivalents), ensuring any alias->input-format
transformation is applied consistently so top-N selection uses the same
upsampled metric as the timeseries.
---
Nitpick comments:
In `@tests/sentry/api/helpers/test_error_upsampling.py`:
- Around line 54-76: Add tests to ensure
transform_query_columns_for_error_upsampling only transforms exact "count()"
(case-insensitive after strip) and leaves other count forms unchanged: add a
case with columns = ["count(id)", "count(distinct id)", "count(*)"] asserting
the result equals the original list, and add a mixed case with ["count()",
"other_column", "count()"] asserting the two count() entries are transformed to
"upsampled_count() as count" while other_column remains unchanged; reference the
transform_query_columns_for_error_upsampling function in the test additions.
In `@tests/snuba/api/endpoints/test_organization_events_stats.py`:
- Around line 3604-3722: Add a new test method (e.g.,
test_error_upsampling_top_events_regression) alongside the existing ones that
patches sentry.api.helpers.error_upsampling.options, sets the allowlist to
include both projects (mock_options.get.return_value = [self.project.id,
self.project2.id]), stores multiple error events with differing sample weights
(via self.store_event using distinct event_ids and sample_weight/context to
create unequal effective counts) across the two time buckets, then calls the
endpoint at self.url with the same start/end/interval/yAxis="count()" and
include topEvents=<N> in the query and project list; assert response.status_code
== 200 and check response.data["data"] for two buckets that the topEvents list
is ranked by upsampled counts and that the "Other" entry (if present) reflects
the sum of upsampled counts for non-top events to catch ranking/"Other"
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34418560-94fc-463b-9a87-8341228682c3
📒 Files selected for processing (8)
pyproject.tomlsentry-reposrc/sentry/api/endpoints/organization_events_stats.pysrc/sentry/api/helpers/error_upsampling.pysrc/sentry/search/events/datasets/discover.pysrc/sentry/testutils/factories.pytests/sentry/api/helpers/test_error_upsampling.pytests/snuba/api/endpoints/test_organization_events_stats.py
| @@ -0,0 +1 @@ | |||
| Subproject commit a5d290951def84afdcc4c88d2f1f20023fc36e2a | |||
There was a problem hiding this comment.
Split unrelated submodule bump from error-upsampling changes.
This submodule update introduces OTLP span-link ingestion behavior, which is outside the error-upsampling objective. Please move it to a separate PR (or justify hard dependency) to reduce risk and simplify rollback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sentry-repo` at line 1, This PR mixes an unrelated submodule bump (commit
a5d290951def84afdcc4c88d2f1f20023fc36e2a) that introduces OTLP span-link
ingestion with the error-upsampling changes; separate them by removing the
submodule update from this branch/commit and creating a new PR containing only
the submodule bump (or, if truly required, add an explicit justification and
dependency note to this PR). Concretely: revert or drop the submodule reference
change tied to commit a5d290951def84afdcc4c88d2f1f20023fc36e2a from this branch,
create a new branch/PR that applies just that submodule update, run CI/tests on
both branches, and if you keep the bump here, add a short rationale in the PR
description explaining the hard dependency on OTLP span-link ingestion.
| should_upsample = is_errors_query_for_error_upsampled_projects( | ||
| snuba_params, organization, dataset, request | ||
| ) |
There was a problem hiding this comment.
Base the upsampling decision on the effective query, not the outer request state.
_get_event_stats() can run with a rewritten dataset/query, but this check still reads the outer dataset plus request.GET["query"]. In the dashboard split paths that means eligibility is evaluated against the original request instead of the query you actually send to Snuba, so upsampling can be skipped or applied incorrectly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/api/endpoints/organization_events_stats.py` around lines 220 -
222, The upsampling check is using the outer dataset and request state instead
of the effective query/dataset used to call Snuba; update the call to
is_errors_query_for_error_upsampled_projects inside _get_event_stats() to pass
the rewritten/effective values from snuba_params (e.g., the effective query and
dataset stored on snuba_params) and organization instead of using the outer
dataset and request.GET["query"], so eligibility is evaluated against the actual
query sent to Snuba.
| 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) | ||
| if cached_result is not None: | ||
| return cached_result and _should_apply_sample_weight_transform(dataset, request) | ||
|
|
||
| # Cache miss - perform fresh allowlist check | ||
| is_eligible = _are_all_projects_error_upsampled(snuba_params.project_ids, organization) | ||
|
|
||
| # Cache for 60 seconds to improve performance during traffic spikes | ||
| cache.set(cache_key, is_eligible, 60) |
There was a problem hiding this comment.
Use a deterministic cache key instead of hash(...).
hash(tuple(sorted(...))) is salted per Python process, so different workers will generate different keys for the same project set. That means invalidate_upsampling_cache() can miss entries written by another worker, leaving stale allowlist decisions around until the TTL expires.
Suggested fix
- cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}"
+ project_key = ",".join(str(project_id) for project_id in sorted(snuba_params.project_ids))
+ cache_key = f"error_upsampling_eligible:{organization.id}:{project_key}"
...
- cache_key = f"error_upsampling_eligible:{organization_id}:{hash(tuple(sorted(project_ids)))}"
+ project_key = ",".join(str(project_id) for project_id in sorted(project_ids))
+ cache_key = f"error_upsampling_eligible:{organization_id}:{project_key}"Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/api/helpers/error_upsampling.py` around lines 27 - 38, The cache
key uses Python's process-salted hash which is non-deterministic across workers;
replace hash(tuple(sorted(snuba_params.project_ids))) with a deterministic
representation (e.g., join the sorted project IDs into a string or a stable
digest like hashlib.sha256 of the joined IDs) when building cache_key in the
error upsampling flow (the block that computes cache_key and uses
cache.get/cache.set) and apply the same deterministic construction in the other
occurrence referenced (the lines around the second use). Ensure you keep the
sort to make ordering stable, reference snuba_params.project_ids, and update any
related helpers such as _are_all_projects_error_upsampled and
invalidate_upsampling_cache usage to use the same deterministic key construction
so invalidation works across workers.
| SnQLFunction( | ||
| "upsampled_count", | ||
| required_args=[], | ||
| # Optimized aggregation for error upsampling - assumes sample_weight | ||
| # exists for all events in allowlisted projects as per schema design | ||
| snql_aggregate=lambda args, alias: Function( | ||
| "toInt64", | ||
| [Function("sum", [Column("sample_weight")])], | ||
| alias, | ||
| ), | ||
| default_result_type="number", |
There was a problem hiding this comment.
Keep upsampled_count typed as an integer.
This aggregate returns toInt64(sum(sample_weight)), but it is registered as "number". Since the endpoint aliases it back to count, upsampled responses will advertise a different type than normal count(), which can break metadata consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/search/events/datasets/discover.py` around lines 1041 - 1051, The
SnQLFunction "upsampled_count" currently sets default_result_type="number" but
returns toInt64(sum(sample_weight)); update the SnQLFunction definition for
"upsampled_count" to use an integer result type (e.g.,
default_result_type="integer") so its advertised type matches normal count()
responses and downstream metadata consumers remain consistent.
| try: | ||
| client_sample_rate = ( | ||
| normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate") | ||
| ) | ||
| except Exception: | ||
| pass | ||
| if client_sample_rate: | ||
| try: | ||
| normalized_data["sample_rate"] = float(client_sample_rate) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Replace bare exception handlers with specific exception types.
The bare except Exception: pass blocks silently swallow all errors, making debugging difficult and potentially hiding real issues. Catch only the specific exceptions expected during dict traversal and type conversion.
🛡️ Proposed fix
client_sample_rate = None
try:
client_sample_rate = (
normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate")
)
- except Exception:
+ except (AttributeError, KeyError, TypeError):
pass
- if client_sample_rate:
+ if client_sample_rate is not None:
try:
normalized_data["sample_rate"] = float(client_sample_rate)
- except Exception:
+ except (TypeError, ValueError):
pass🧰 Tools
🪛 Ruff (0.15.15)
[error] 351-352: try-except-pass detected, consider logging the exception
(S110)
[warning] 351-351: Do not catch blind exception: Exception
(BLE001)
[error] 356-357: try-except-pass detected, consider logging the exception
(S110)
[warning] 356-356: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/testutils/factories.py` around lines 347 - 357, The current bare
excepts around extracting client_sample_rate and converting it to float should
be replaced with specific exceptions: when retrieving nested keys from
normalized_data (the expression using normalized_data.get("contexts",
{}).get("error_sampling", {}).get("client_sample_rate")), catch only
AttributeError and TypeError (to handle cases where contexts or error_sampling
are not mappings) instead of Exception; when assigning
normalized_data["sample_rate"] = float(client_sample_rate) catch only ValueError
and TypeError (to handle invalid string/None conversions). Update the try/except
blocks around client_sample_rate and the float conversion accordingly,
referencing normalized_data and client_sample_rate/sample_rate to locate the
code.
| if client_sample_rate: | ||
| try: | ||
| normalized_data["sample_rate"] = float(client_sample_rate) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Truthiness check prevents setting sample_rate=0.0.
Line 353's if client_sample_rate: evaluates to False when client_sample_rate is 0 or 0.0, preventing valid zero sample rates from being set. A sample rate of 0.0 is semantically valid (indicates all events should be dropped during sampling).
🔧 Proposed fix
except Exception:
pass
- if client_sample_rate:
+ if client_sample_rate is not None:
try:
normalized_data["sample_rate"] = float(client_sample_rate)
except Exception:🧰 Tools
🪛 Ruff (0.15.15)
[error] 356-357: try-except-pass detected, consider logging the exception
(S110)
[warning] 356-356: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/testutils/factories.py` around lines 353 - 357, The truthiness
check using "if client_sample_rate:" prevents valid zero values from being
applied; change the guard to an explicit None check (e.g., "if
client_sample_rate is not None:") in the block that sets
normalized_data["sample_rate"], then attempt to coerce client_sample_rate to
float (catching ValueError/TypeError) and assign it to
normalized_data["sample_rate"] so 0 and 0.0 are preserved; reference the
normalized_data dict and the client_sample_rate variable in the sample-rate
assignment logic.
Test 3
Summary by CodeRabbit
New Features
Tests
Chores