-
Notifications
You must be signed in to change notification settings - Fork 0
feat(upsampling) - Support upsampled error count with performance optimizations #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,10 @@ | |||||||||||||||||||||||||||
| from sentry.api.api_publish_status import ApiPublishStatus | ||||||||||||||||||||||||||||
| from sentry.api.base import region_silo_endpoint | ||||||||||||||||||||||||||||
| from sentry.api.bases import OrganizationEventsV2EndpointBase | ||||||||||||||||||||||||||||
| from sentry.api.helpers.error_upsampling import ( | ||||||||||||||||||||||||||||
| is_errors_query_for_error_upsampled_projects, | ||||||||||||||||||||||||||||
| transform_query_columns_for_error_upsampling, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| from sentry.constants import MAX_TOP_EVENTS | ||||||||||||||||||||||||||||
| from sentry.models.dashboard_widget import DashboardWidget, DashboardWidgetTypes | ||||||||||||||||||||||||||||
| from sentry.models.organization import Organization | ||||||||||||||||||||||||||||
|
|
@@ -117,7 +121,7 @@ def get(self, request: Request, organization: Organization) -> Response: | |||||||||||||||||||||||||||
| status=400, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| comparison_delta = None | ||||||||||||||||||||||||||||
| if "comparisonDelta" in request.GET: | ||||||||||||||||||||||||||||
|
|
@@ -211,12 +215,28 @@ def _get_event_stats( | |||||||||||||||||||||||||||
| zerofill_results: bool, | ||||||||||||||||||||||||||||
| comparison_delta: timedelta | None, | ||||||||||||||||||||||||||||
| ) -> SnubaTSResult | dict[str, SnubaTSResult]: | ||||||||||||||||||||||||||||
| # 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 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if top_events > 0: | ||||||||||||||||||||||||||||
| # Apply upsampling transformation just before query execution | ||||||||||||||||||||||||||||
| # This late transformation ensures we use the most current schema assumptions | ||||||||||||||||||||||||||||
| if upsampling_enabled: | ||||||||||||||||||||||||||||
|
Comment on lines
+226
to
+232
|
||||||||||||||||||||||||||||
| upsampling_enabled = should_upsample | |
| final_columns = query_columns | |
| if top_events > 0: | |
| # Apply upsampling transformation just before query execution | |
| # This late transformation ensures we use the most current schema assumptions | |
| if upsampling_enabled: | |
| final_columns = query_columns | |
| if top_events > 0: | |
| # Apply upsampling transformation just before query execution | |
| # This late transformation ensures we use the most current schema assumptions | |
| if should_upsample: |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,140 @@ | ||||||||||
| from collections.abc import Sequence | ||||||||||
| from types import ModuleType | ||||||||||
| from typing import Any | ||||||||||
|
|
||||||||||
| from rest_framework.request import Request | ||||||||||
|
|
||||||||||
| from sentry import options | ||||||||||
| from sentry.models.organization import Organization | ||||||||||
| from sentry.search.events.types import SnubaParams | ||||||||||
| from sentry.utils.cache import cache | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def is_errors_query_for_error_upsampled_projects( | ||||||||||
| snuba_params: SnubaParams, | ||||||||||
| organization: Organization, | ||||||||||
| dataset: ModuleType, | ||||||||||
| request: Request, | ||||||||||
| ) -> bool: | ||||||||||
| """ | ||||||||||
| Determine if this query should use error upsampling transformations. | ||||||||||
| Only applies when ALL projects are allowlisted and we're querying error events. | ||||||||||
| Performance optimization: Cache allowlist eligibility for 60 seconds to avoid | ||||||||||
| 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)))}" | ||||||||||
|
||||||||||
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" | |
| sorted_project_ids = ",".join(map(str, sorted(snuba_params.project_ids))) | |
| cache_key = f"error_upsampling_eligible:{organization.id}:{hashlib.md5(sorted_project_ids.encode()).hexdigest()}" |
Copilot
AI
Jul 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same hash() consistency issue as above - this cache key should use a deterministic hashing method to ensure consistent cache behavior across processes.
| cache_key = f"error_upsampling_eligible:{organization_id}:{hash(tuple(sorted(project_ids)))}" | |
| import hashlib | |
| project_ids_hash = hashlib.md5(str(tuple(sorted(project_ids))).encode('utf-8')).hexdigest() | |
| cache_key = f"error_upsampling_eligible:{organization_id}:{project_ids_hash}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed typo in error message - removed duplicate 'If' from 'If topEvents needs to be at least 1'.