feat(upsampling) - Support upsampled error count with performance optimizations#8
feat(upsampling) - Support upsampled error count with performance optimizations#8ShashankFC 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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change introduces error upsampling functionality to Sentry's API infrastructure, including eligibility checks with caching, query column transformations, database support for upsampled aggregates, and comprehensive test coverage across multiple modules. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Endpoint as organization_events_stats
participant UpsamplingHelper as Error Upsampling
participant Cache as Cache Layer
participant SnubaEngine as Snuba Query Engine
participant Database as ClickHouse
Client->>Endpoint: GET /organization_events_stats<br/>(with projects & query)
Endpoint->>UpsamplingHelper: is_errors_query_for_error_upsampled_projects<br/>(organization, projects)
rect rgba(200, 220, 255, 0.3)
Note over UpsamplingHelper,Cache: Eligibility Check with Cache
UpsamplingHelper->>Cache: Get cached eligibility<br/>(org_id, project_ids)
alt Cache Hit
Cache-->>UpsamplingHelper: Return cached result
else Cache Miss
UpsamplingHelper->>UpsamplingHelper: Check project allowlist<br/>_are_all_projects_error_upsampled()
UpsamplingHelper->>Cache: Store eligibility result
end
end
UpsamplingHelper-->>Endpoint: should_upsample: bool
rect rgba(200, 255, 200, 0.3)
Note over Endpoint: Column Transformation
alt should_upsample == true
Endpoint->>Endpoint: transform_query_columns<br/>(count() → upsampled_count())
else should_upsample == false
Endpoint->>Endpoint: Keep original columns
end
end
Endpoint->>SnubaEngine: Execute timeseries query<br/>(with final_columns)
SnubaEngine->>Database: Execute aggregation<br/>(upsampled_count or count)
Database-->>SnubaEngine: Return aggregated results
SnubaEngine-->>Endpoint: Query results
Endpoint-->>Client: HTTP 200 response<br/>(upsampled stats)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @src/sentry/api/endpoints/organization_events_stats.py:
- Around line 218-227: The upsampling eligibility call inside _get_event_stats
incorrectly uses the outer closure dataset; change the call to
is_errors_query_for_error_upsampled_projects(..., scoped_dataset, request) so it
uses the function parameter scoped_dataset, and remove the redundant
upsampling_enabled assignment by using should_upsample directly everywhere (keep
final_columns = query_columns as-is); update references to use should_upsample
instead of upsampling_enabled and ensure the variable name is used consistently
when building the query.
In @src/sentry/api/helpers/error_upsampling.py:
- Around line 67-74: invalidate_upsampling_cache currently uses Python's
built-in hash() which is process-randomized; replace that with a stable hash
(e.g., compute a SHA256 hex of the sorted project_ids sequence) to build
cache_key so invalidation works across processes. Update the cache_key
construction in invalidate_upsampling_cache to sort project_ids, serialize them
deterministically (like joining as comma-separated ints or JSON), compute a
stable digest (hashlib.sha256(...).hexdigest()), and use that digest in the
f"error_upsampling_eligible:{organization_id}:{digest}" key; keep the rest of
the function logic unchanged.
- Line 27: The cache key construction using Python's builtin hash() (cache_key =
f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}")
is non-deterministic across processes; replace hash(...) with a stable digest
(e.g., compute a deterministic string of the sorted project_ids and run a stable
hash like hashlib.sha256(...).hexdigest()) so the key is consistent across
workers, and make the identical change in invalidate_upsampling_cache so both
creation and invalidation use the same deterministic key derivation.
🧹 Nitpick comments (4)
src/sentry/testutils/factories.py (1)
344-358: Consider more specific exception handling for better debuggability.The function uses broad
except Exception: passblocks which could silently hide unexpected errors during test development. While defensive programming is reasonable for test utilities, consider at least logging exceptions or being more specific about what you're catching.🔎 Suggested improvement
def _set_sample_rate_from_error_sampling(normalized_data: MutableMapping[str, Any]) -> None: """Set 'sample_rate' on normalized_data if contexts.error_sampling.client_sample_rate is present and valid.""" client_sample_rate = None try: client_sample_rate = ( normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate") ) - except Exception: + except (KeyError, AttributeError, TypeError): pass if client_sample_rate: try: normalized_data["sample_rate"] = float(client_sample_rate) - except Exception: + except (ValueError, TypeError): passThis makes it clearer what errors are expected and helps catch unexpected issues during development.
tests/sentry/api/helpers/test_error_upsampling.py (1)
19-101: Good test coverage for helper functions, but consider testing the main entry point.The tests provide comprehensive coverage of the private helper functions with good edge case handling (empty lists, case insensitivity, whitespace, different datasets). However, I notice that
is_errors_query_for_error_upsampled_projects(the main public entry point) andinvalidate_upsampling_cacheare not tested.Consider adding tests for:
is_errors_query_for_error_upsampled_projects- The main function that integrates all the helpers and includes caching logic- Cache behavior - Verify the 60-second cache works correctly and returns cached results
invalidate_upsampling_cache- Ensure cache invalidation works as expectedExample test structure
@patch("sentry.api.helpers.error_upsampling.options") def test_is_errors_query_with_caching(self, mock_options: Mock) -> None: """Test that caching reduces option lookups.""" mock_options.get.return_value = self.project_ids # First call should check options result1 = is_errors_query_for_error_upsampled_projects( self.snuba_params, self.organization, errors, self.request ) assert result1 is True assert mock_options.get.call_count == 1 # Second call should use cache result2 = is_errors_query_for_error_upsampled_projects( self.snuba_params, self.organization, errors, self.request ) assert result2 is True assert mock_options.get.call_count == 1 # No additional call def test_invalidate_upsampling_cache(self) -> None: """Test cache invalidation.""" # ... test implementationsrc/sentry/api/helpers/error_upsampling.py (2)
43-64: Unusedorganizationparameter and O(n) allowlist lookupTwo issues in this function:
The
organizationparameter is unused (confirmed by static analysis). Either remove it or use it for future organization-scoped allowlist functionality.The allowlist lookup uses
inoperator on a list, which is O(n) per project. Convert to a set for O(1) lookups.🔎 Proposed fix
def _are_all_projects_error_upsampled( - project_ids: Sequence[int], organization: Organization + project_ids: Sequence[int], ) -> bool: """ Check if ALL projects in the query are allowlisted for error upsampling. ... """ if not project_ids: return False allowlist = options.get("issues.client_error_sampling.project_allowlist", []) if not allowlist: return False + allowlist_set = set(allowlist) # All projects must be in the allowlist - result = all(project_id in allowlist for project_id in project_ids) + result = all(project_id in allowlist_set for project_id in project_ids) return resultNote: Update the caller on line 35 to remove the
organizationargument if you remove the parameter.
130-140: Query parsing may miss valid error-focused queriesThe current check only matches the exact substring
event.type:error. This could miss:
- Queries with spaces:
event.type: error- Quoted values:
event.type:"error"- Negated transaction queries:
!event.type:transactionThe docstring mentions this is intentional to "err on the side of caution," which is reasonable for an initial implementation. Consider adding a comment or TODO to expand this in the future if needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 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
🧰 Additional context used
🧬 Code graph analysis (5)
src/sentry/search/events/datasets/discover.py (1)
src/sentry/search/events/fields.py (2)
SnQLFunction(2179-2203)args(1372-1373)
tests/snuba/api/endpoints/test_organization_events_stats.py (1)
src/sentry/api/endpoints/organization_events_stats.py (1)
get(105-524)
src/sentry/api/endpoints/organization_events_stats.py (2)
src/sentry/api/helpers/error_upsampling.py (2)
is_errors_query_for_error_upsampled_projects(13-40)transform_query_columns_for_error_upsampling(77-100)src/sentry/snuba/rpc_dataset_common.py (1)
run_top_events_timeseries_query(484-641)
src/sentry/api/helpers/error_upsampling.py (1)
src/sentry/api/endpoints/organization_events_stats.py (1)
get(105-524)
tests/sentry/api/helpers/test_error_upsampling.py (1)
src/sentry/api/helpers/error_upsampling.py (4)
_are_all_projects_error_upsampled(43-64)_is_error_focused_query(130-140)_should_apply_sample_weight_transform(103-127)transform_query_columns_for_error_upsampling(77-100)
🪛 Ruff (0.14.10)
src/sentry/testutils/factories.py
351-352: try-except-pass detected, consider logging the exception
(S110)
351-351: Do not catch blind exception: Exception
(BLE001)
356-357: try-except-pass detected, consider logging the exception
(S110)
356-356: Do not catch blind exception: Exception
(BLE001)
src/sentry/search/events/datasets/discover.py
1046-1046: Unused lambda argument: args
(ARG005)
src/sentry/api/helpers/error_upsampling.py
44-44: Unused function argument: organization
(ARG001)
🔇 Additional comments (8)
sentry-repo (1)
1-1: Verify the relevance of this submodule update to the PR objectives.This submodule reference shows changes related to OTLP span links processing, which appears unrelated to the PR's stated objective of adding error upsampling support. The commit message mentions "feat(otlp): Write span links to EAP as a JSON-encoded string attribute" which has no connection to error sampling or upsampling functionality.
Please confirm whether this submodule update should be part of this PR, or if it was included unintentionally.
pyproject.toml (1)
176-176: LGTM! Module registration is correct.The new error upsampling module and its tests are properly registered in the project configuration. Including them in the "stronger typing" section ensures stricter type checking, which is a good practice for new code.
Also applies to: 464-464
src/sentry/testutils/factories.py (1)
1049-1049: LGTM! Proper integration of sample rate extraction.The function is called at the right point in the event normalization flow, ensuring that sample rates from error sampling contexts are properly propagated to the normalized event data.
src/sentry/search/events/datasets/discover.py (1)
1041-1052: Schema assumption for sample_weight is verified and properly enforced.The implementation correctly relies on the database schema guarantee that
sample_weightexists for all events in allowlisted projects. This assumption is documented insrc/sentry/api/helpers/error_upsampling.py(lines 85-86) and enforced through the_are_all_projects_error_upsampled()validation function, which ensures upsampling only applies when all projects in the query are allowlisted. Null handling is intentionally omitted because the schema contract guarantees non-null values for allowlisted projects—unlike fields likeissue.idwhich require defensivecoalesce()handling.The unused
argsparameter is required by thesnql_aggregatelambda signature pattern and is consistent with all other aggregate functions in this file.src/sentry/api/endpoints/organization_events_stats.py (1)
14-17: LGTM!The new imports for error upsampling helpers are correctly placed and follow the existing import organization pattern.
tests/snuba/api/endpoints/test_organization_events_stats.py (1)
3554-3603: LGTM - Good test coverage for error upsampling scenarios.The test class is well-structured with clear setup and covers the key scenarios:
- Fully allowlisted projects (upsampling applied)
- Partially allowlisted projects (upsampling not applied)
- Transaction events (upsampling not applied)
- No allowlisted projects (upsampling not applied)
The test data correctly uses
client_sample_rate: 0.1which should result in a 10x multiplier when upsampling is applied.src/sentry/api/helpers/error_upsampling.py (2)
77-100: LGTM!The column transformation logic is clean and handles the
count()→upsampled_count() as countmapping correctly. The alias preservation ensures downstream code continues to work with the expected column name.
103-127: LGTM!The dataset-based decision logic is well-structured. Using lazy imports inside the function avoids circular import issues while maintaining clean separation of concerns.
| # 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 | ||
| ) | ||
|
|
||
| # Store the upsampling decision to apply later during query building | ||
| # This separation allows for better query optimization and caching | ||
| upsampling_enabled = should_upsample | ||
| final_columns = query_columns |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how _get_event_stats is called with different scoped_dataset values
rg -n -B2 -A5 '_get_event_stats\(' src/sentry/api/endpoints/organization_events_stats.py | head -80Repository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 2975
🏁 Script executed:
# Get the full _get_event_stats function definition and context
sed -n '200,300p' src/sentry/api/endpoints/organization_events_stats.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 5040
🏁 Script executed:
# Check the widget splitting logic that calls _get_event_stats with different datasets
sed -n '360,400p' src/sentry/api/endpoints/organization_events_stats.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 2132
🏁 Script executed:
# Check the parent function and outer dataset variable
sed -n '150,210p' src/sentry/api/endpoints/organization_events_stats.pyRepository: AI-Code-Review-Evals/coderabbit-sentry
Length of output: 2610
Use scoped_dataset parameter instead of outer dataset for upsampling eligibility check
The eligibility check on line 221 uses dataset from the outer closure scope rather than scoped_dataset which is passed as a parameter. Since _get_event_stats is called with different datasets (e.g., discover for errors-only queries at line 386-387, or split datasets at line 372-373), the upsampling decision will be based on the original outer dataset instead of the actual dataset being queried.
Additionally, the variable assignment on line 226 (upsampling_enabled = should_upsample) is redundant—use should_upsample directly throughout the function.
🤖 Prompt for AI Agents
In @src/sentry/api/endpoints/organization_events_stats.py around lines 218 -
227, The upsampling eligibility call inside _get_event_stats incorrectly uses
the outer closure dataset; change the call to
is_errors_query_for_error_upsampled_projects(..., scoped_dataset, request) so it
uses the function parameter scoped_dataset, and remove the redundant
upsampling_enabled assignment by using should_upsample directly everywhere (keep
final_columns = query_columns as-is); update references to use should_upsample
instead of upsampling_enabled and ensure the variable name is used consistently
when building the query.
| 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.
Critical: hash() produces non-deterministic results across processes
Python's built-in hash() function is randomized by default via PYTHONHASHSEED. This means the cache key will be different across different worker processes, causing:
- Cache misses on every request to a different worker
invalidate_upsampling_cache()will fail to invalidate entries created by other workers- Potential inconsistent behavior during rolling deployments
Use a deterministic hashing approach instead:
🔎 Proposed fix using a stable hash
+import hashlib
+
+
+def _stable_project_ids_hash(project_ids: Sequence[int]) -> str:
+ """Generate a stable hash for project IDs that is consistent across processes."""
+ sorted_ids = sorted(project_ids)
+ ids_str = ",".join(str(pid) for pid in sorted_ids)
+ return hashlib.sha256(ids_str.encode()).hexdigest()[:16]
+
def is_errors_query_for_error_upsampled_projects(
snuba_params: SnubaParams,
organization: Organization,
dataset: ModuleType,
request: Request,
) -> bool:
...
- cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}"
+ cache_key = f"error_upsampling_eligible:{organization.id}:{_stable_project_ids_hash(snuba_params.project_ids)}"Apply the same fix to invalidate_upsampling_cache.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" | |
| import hashlib | |
| def _stable_project_ids_hash(project_ids: Sequence[int]) -> str: | |
| """Generate a stable hash for project IDs that is consistent across processes.""" | |
| sorted_ids = sorted(project_ids) | |
| ids_str = ",".join(str(pid) for pid in sorted_ids) | |
| return hashlib.sha256(ids_str.encode()).hexdigest()[:16] | |
| def is_errors_query_for_error_upsampled_projects( | |
| snuba_params: SnubaParams, | |
| organization: Organization, | |
| dataset: ModuleType, | |
| request: Request, | |
| ) -> bool: | |
| ... | |
| cache_key = f"error_upsampling_eligible:{organization.id}:{_stable_project_ids_hash(snuba_params.project_ids)}" |
🤖 Prompt for AI Agents
In @src/sentry/api/helpers/error_upsampling.py at line 27, The cache key
construction using Python's builtin hash() (cache_key =
f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}")
is non-deterministic across processes; replace hash(...) with a stable digest
(e.g., compute a deterministic string of the sorted project_ids and run a stable
hash like hashlib.sha256(...).hexdigest()) so the key is consistent across
workers, and make the identical change in invalidate_upsampling_cache so both
creation and invalidation use the same deterministic key derivation.
| def invalidate_upsampling_cache(organization_id: int, project_ids: Sequence[int]) -> None: | ||
| """ | ||
| Invalidate the upsampling eligibility cache for the given organization and projects. | ||
| This should be called when the allowlist configuration changes to ensure | ||
| cache consistency across the system. | ||
| """ | ||
| cache_key = f"error_upsampling_eligible:{organization_id}:{hash(tuple(sorted(project_ids)))}" | ||
| cache.delete(cache_key) |
There was a problem hiding this comment.
Cache invalidation uses the same non-deterministic hash
This function has the same hash() issue as the main eligibility check. The invalidation will only work within the same process that created the cache entry.
Apply the same stable hashing fix proposed above.
🤖 Prompt for AI Agents
In @src/sentry/api/helpers/error_upsampling.py around lines 67 - 74,
invalidate_upsampling_cache currently uses Python's built-in hash() which is
process-randomized; replace that with a stable hash (e.g., compute a SHA256 hex
of the sorted project_ids sequence) to build cache_key so invalidation works
across processes. Update the cache_key construction in
invalidate_upsampling_cache to sort project_ids, serialize them
deterministically (like joining as comma-separated ints or JSON), compute a
stable digest (hashlib.sha256(...).hexdigest()), and use that digest in the
f"error_upsampling_eligible:{organization_id}:{digest}" key; keep the rest of
the function logic unchanged.
2 participants
Test 3
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/sentry-coderabbit#3