feat(upsampling) - Support upsampled error count with performance optimizations - #2
feat(upsampling) - Support upsampled error count with performance optimizations#2ron-x5labs wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (41)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: feat(upsampling) - Support upsampled error count with performance optimizations
Problem
This PR adds error upsampling support: when ALL queried projects are on the issues.client_error_sampling.project_allowlist option and the query targets error events, count() aggregations are replaced with upsampled_count() (which computes toInt64(sum(ifNull(sample_weight, 1)))) to reflect upsampled error counts. The transform is wired into the organization-events-stats timeseries endpoint.
Solution Reviewed
A new error_upsampling.py helper module gates the transform behind a project allowlist check and a dataset/query-type check. The upsampled_count SnQL function is registered in discover.py. The test factory (factories.py) injects sample_rate from contexts.error_sampling.client_sample_rate on stored events. Unit and endpoint tests cover the helpers and the timeseries response.
Summary
The core design (allowlist gate → column transform → SnQL aggregate) is sound and the test coverage for the helpers is reasonable. However, there are three blocking issues: the error-query detection uses a naive substring match that matches negated queries, equations are not transformed while columns are, and the test factory sets sample_rate but the SnQL function reads sample_weight with no code in the diff bridging the two. These should be resolved before merge.
Note: The PR also deletes all 33 .github/workflows/* files. The PR body says this is a "benchmark PR" to strip CI overhead — these deletions are not part of the feature and are not reviewed here.
Verification
- No local clone available (git clone failed due to certificate verification); verification limited to diff analysis only.
- Tests/typecheck not run — no worktree.
Verdict
Recommend changes before merge — the substring matching bug can apply upsampling to non-error queries, the equations gap produces inconsistent results, and the sample_rate→sample_weight disconnect means the feature may be a no-op in production or the tests may be false positives.
| """ | ||
| query = request.GET.get("query", "").lower() | ||
|
|
||
| if "event.type:error" in query: |
There was a problem hiding this comment.
🔴 Blocking — substring match matches negated/false-positive queries
_is_error_focused_query uses "event.type:error" in query which matches:
- Negations:
!event.type:errororNOT event.type:error— these explicitly exclude errors but would trigger upsampling, producing incorrectupsampled_countresults on non-error data. - False-positive tokens:
event.type:error_report,some_field:"event.type:error", or any tag value containing the substring.
Use a token-aware parser or at minimum a word-boundary regex (e.g. r'(?:^|\s)(?<!\!)event\.type:error(?:\s|$)') that handles negation.
| ) | ||
| final_columns = query_columns | ||
| if should_upsample: | ||
| final_columns = transform_query_columns_for_error_upsampling(query_columns) |
There was a problem hiding this comment.
🔴 Blocking — equations are not transformed for upsampling
transform_query_columns_for_error_upsampling swaps count() → upsampled_count() as count in query_columns (used for y_axes, timeseries_columns, selected_columns), but self.get_equation_list(organization, request) is passed through unchanged to all three code paths (RPC top_events, non-RPC top_events, and timeseries).
If an equation references count() (e.g. equation|count() / count_unique(user)), it will compute the raw un-upsampled count, producing inconsistent results against the upsampled y_axes value that shares the same count alias. Apply the same count() → upsampled_count() transform inside equation expressions.
| pass | ||
| if client_sample_rate: | ||
| try: | ||
| normalized_data["sample_rate"] = float(client_sample_rate) |
There was a problem hiding this comment.
🔴 Blocking — sample_rate vs sample_weight disconnect
_set_sample_rate_from_error_sampling sets normalized_data["sample_rate"] from contexts.error_sampling.client_sample_rate. But the upsampled_count() SnQL function reads Column("sample_weight") with ifNull(sample_weight, 1). No code in this diff translates sample_rate → sample_weight during event storage.
If sample_weight is never populated from sample_rate, ifNull falls back to 1 for every event, and the upsampled count equals the raw event count (1, not 10). The endpoint test asserts count == 10 for sample_rate=0.1 with 1 event — either:
- The test is a false positive (passes for the wrong reason, or doesn't actually exercise the
sample_weightpath), or - There's a missing production ingestion path that should be in this PR to populate
sample_weightfromsample_rate.
Clarify which, and ensure the full pipeline populates sample_weight before relying on this feature.
| [Function("sum", [Function("ifNull", [Column("sample_weight"), 1])])], | ||
| alias, | ||
| ), | ||
| default_result_type="number", |
There was a problem hiding this comment.
🟡 Non-blocking — default_result_type should be "integer", not "number"
The snql_aggregate wraps the result in toInt64(...), guaranteeing an integer output. But default_result_type="number" tells downstream consumers to expect a float. Sibling aggregates (count, count_unique, etc.) use "integer". This mismatch can cause 10.0 instead of 10 in response formatting, or floating-point comparison where integer comparison is expected. Set default_result_type="integer".
| query_columns: Sequence[str], | ||
| ) -> list[str]: | ||
| """ | ||
| Transform aggregation functions to use sum(sample_weight) instead of count() |
There was a problem hiding this comment.
🟡 Non-blocking — misleading docstring
The docstring says the transform uses sum(sample_weight), but the actual SnQL function computes toInt64(sum(ifNull(sample_weight, 1))). The ifNull(…, 1) fallback is a significant semantic difference — rows missing sample_weight get weight 1, not 0, silently inflating counts for events that were never sampled. Update the docstring to reflect the ifNull default and its implications.
| from django.test import RequestFactory | ||
| from rest_framework.request import Request | ||
|
|
||
| from sentry.api.helpers.error_upsampling import ( |
There was a problem hiding this comment.
🟡 Non-blocking — public entry point is_errors_query_for_error_upsampled_projects is not tested
The import block only pulls in the private helpers (_are_all_projects_error_upsampled, _should_apply_sample_weight_transform, _is_error_focused_query, transform_query_columns_for_error_upsampling). The public orchestration function — the one the endpoint actually calls — has no direct test. A regression in the short-circuit order, wrong dataset reference, or composition logic would go undetected. Add a test that exercises the full decision chain (allowlist + dataset + query type → bool).
| assert len(data) == 2 | ||
|
|
||
|
|
||
| class OrganizationEventsStatsErrorUpsamplingTest(APITestCase, SnubaTestCase): |
There was a problem hiding this comment.
🟡 Non-blocking — no test covers the topEvents > 0 code path
The feature modifies both the top_events branch (run_top_events_timeseries_query / top_events_timeseries with final_columns) and the standard timeseries branch, but all 4 endpoint tests use plain timeseries queries without topEvents. The upsampling transform in top-events mode has zero coverage. Add a test with topEvents=1 or more.
|
|
||
| self.project = self.create_project() | ||
| self.project2 = self.create_project() | ||
| self.user = self.create_user() |
There was a problem hiding this comment.
🟡 Non-blocking — self.user overwritten after login_as
setUp calls self.login_as(user=self.user) at line 3559, then overwrites self.user = self.create_user() at line 3566. The authenticated session is still tied to the original self.user (from the base class), but self.user now points to a different, unauthenticated user. Any test logic that references self.user for assertions (e.g. sentry:user tag matching) will compare against the wrong user. Use a distinct attribute name (e.g. self.other_user).
| ) | ||
| except Exception: | ||
| pass | ||
| if client_sample_rate: |
There was a problem hiding this comment.
💡 Suggestion — if client_sample_rate: treats 0 as falsy
if client_sample_rate: discards a legitimate zero sample rate (meaning "no events sampled"). Use if client_sample_rate is not None: for precision, and consider validating the range (0 < rate ≤ 1) before storing.
| ) | ||
| elif top_events <= 0: | ||
| return Response({"detail": "If topEvents needs to be at least 1"}, status=400) | ||
| return Response({"detail": "topEvents needs to be at least 1"}, status=400) |
There was a problem hiding this comment.
💡 Suggestion — unrelated error message change
Changing "If topEvents needs to be at least 1" → "topEvents needs to be at least 1" is unrelated to the upsampling feature. If clients match on this error string, it's a silent breaking change. Split into a separate trivial PR.
1 participant
Benchmark PR recreated from getsentry#94376