-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(occurrences on eap): Implement double reads from EAP for error counts timeseries #107308
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 |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from datetime import datetime | ||
| import logging | ||
| from datetime import datetime, timezone | ||
| from typing import Any | ||
|
|
||
| from snuba_sdk import Request as SnubaRequest | ||
|
|
@@ -12,10 +13,18 @@ | |
| from snuba_sdk.orderby import Direction, OrderBy | ||
| from snuba_sdk.query import Query | ||
|
|
||
| from sentry.models.organization import Organization | ||
| from sentry.models.project import Project | ||
| from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator | ||
| from sentry.search.eap.types import SearchResolverConfig | ||
| from sentry.search.events.types import SnubaParams | ||
| from sentry.snuba.dataset import Dataset | ||
| from sentry.snuba.occurrences_rpc import Occurrences | ||
| from sentry.snuba.referrer import Referrer | ||
| from sentry.utils import snuba | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_errors_counts_timeseries_by_project_and_release( | ||
| end: datetime, | ||
|
|
@@ -24,6 +33,45 @@ def get_errors_counts_timeseries_by_project_and_release( | |
| release_value_list: list[str], | ||
| start: datetime, | ||
| environments_list: list[str] | None = None, | ||
| ) -> list[dict[str, Any]]: | ||
| snuba_results = _get_errors_counts_timeseries_snuba( | ||
| end=end, | ||
| organization_id=organization_id, | ||
| project_id_list=project_id_list, | ||
| release_value_list=release_value_list, | ||
| start=start, | ||
| environments_list=environments_list, | ||
| ) | ||
| results = snuba_results | ||
|
|
||
| if EAPOccurrencesComparator.should_check_experiment( | ||
| "release_thresholds.get_errors_counts_timeseries" | ||
| ): | ||
| eap_results = _get_errors_counts_timeseries_eap( | ||
| end=end, | ||
| organization_id=organization_id, | ||
| project_id_list=project_id_list, | ||
| release_value_list=release_value_list, | ||
| start=start, | ||
| environments_list=environments_list, | ||
| ) | ||
| results = EAPOccurrencesComparator.check_and_choose( | ||
| snuba_results, | ||
| eap_results, | ||
| "release_thresholds.get_errors_counts_timeseries", | ||
| is_experimental_data_a_null_result=len(eap_results) == 0, | ||
| ) | ||
|
|
||
| return results | ||
|
|
||
|
|
||
| def _get_errors_counts_timeseries_snuba( | ||
| end: datetime, | ||
| organization_id: int, | ||
| project_id_list: list[int], | ||
| release_value_list: list[str], | ||
| start: datetime, | ||
| environments_list: list[str] | None = None, | ||
| ) -> list[dict[str, Any]]: | ||
| additional_conditions = [] | ||
| if environments_list: | ||
|
|
@@ -61,3 +109,92 @@ def get_errors_counts_timeseries_by_project_and_release( | |
| )["data"] | ||
|
|
||
| return data | ||
|
|
||
|
|
||
| def _get_errors_counts_timeseries_eap( | ||
| end: datetime, | ||
| organization_id: int, | ||
| project_id_list: list[int], | ||
| release_value_list: list[str], | ||
| start: datetime, | ||
| environments_list: list[str] | None = None, | ||
| ) -> list[dict[str, Any]]: | ||
| try: | ||
| organization = Organization.objects.get(id=organization_id) | ||
| except Organization.DoesNotExist: | ||
| logger.warning( | ||
| "Organization not found for EAP error counts timeseries query", | ||
| extra={"organization_id": organization_id}, | ||
| ) | ||
| return [] | ||
|
|
||
| projects = list(Project.objects.filter(id__in=project_id_list, organization_id=organization_id)) | ||
| if not projects: | ||
| return [] | ||
|
|
||
shashjar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if not release_value_list: | ||
| return [] | ||
|
|
||
| query_string = "type:error" | ||
|
|
||
| if len(release_value_list) == 1: | ||
| query_string += f' release:"{release_value_list[0]}"' | ||
| else: | ||
| release_filter = " OR ".join([f'release:"{r}"' for r in release_value_list]) | ||
| query_string += f" ({release_filter})" | ||
|
|
||
| if environments_list: | ||
| if len(environments_list) == 1: | ||
| query_string += f' environment:"{environments_list[0]}"' | ||
| else: | ||
| env_filter = " OR ".join([f'environment:"{e}"' for e in environments_list]) | ||
| query_string += f" ({env_filter})" | ||
shashjar marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+141
to
+151
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Release and environment names containing quotes are not escaped when building the EAP query string, which can lead to query syntax errors and silent data loss. Suggested FixInstead of building the query via f-string interpolation, use a structured query format or properly escape the Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
||
| snuba_params = SnubaParams( | ||
| start=start, | ||
| end=end, | ||
| organization=organization, | ||
| projects=projects, | ||
| granularity_secs=60, | ||
| ) | ||
|
|
||
| try: | ||
| timeseries_results = Occurrences.run_grouped_timeseries_query( | ||
| params=snuba_params, | ||
| query_string=query_string, | ||
| y_axes=["count()"], | ||
| groupby=["release", "project_id", "environment"], | ||
| referrer=Referrer.SNUBA_SESSIONS_CHECK_RELEASES_HAVE_HEALTH_DATA.value, | ||
| config=SearchResolverConfig(), | ||
| ) | ||
shashjar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # Transform to match Snuba response format | ||
| transformed: list[dict[str, Any]] = [] | ||
| for row in timeseries_results: | ||
| count = row.get("count()", 0) | ||
| if count > 0: | ||
| bucket_dt = datetime.fromtimestamp(row["time"], tz=timezone.utc) | ||
| transformed.append( | ||
| { | ||
| "release": row.get("release"), | ||
| "project_id": int(row["project_id"]), | ||
| "environment": row.get("environment"), | ||
| "time": bucket_dt.isoformat(), | ||
| "count()": int(count), | ||
| } | ||
| ) | ||
|
|
||
| transformed.sort(key=lambda x: x["time"], reverse=True) | ||
|
|
||
| return transformed | ||
|
|
||
| except Exception: | ||
| logger.exception( | ||
| "Fetching error counts timeseries from EAP failed", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_ids": project_id_list, | ||
| "releases": release_value_list, | ||
| }, | ||
| ) | ||
| return [] | ||
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.
EAP database errors crash request despite available Snuba results
Medium Severity
The
_get_errors_counts_timeseries_eapfunction has incomplete exception handling. TheOrganization.objects.get()call only catchesDoesNotExist, not other database errors (e.g., connection timeout). TheProject.objects.filter()call at line 131 has no exception handling at all. If either database operation fails, the exception propagates to the caller, crashing the API request even though Snuba results were already successfully obtained. This defeats the purpose of the double-read pattern where EAP failures should be gracefully handled without affecting production.Additional Locations (1)
src/sentry/api/endpoints/release_thresholds/utils/get_errors_counts_timeseries.py#L49-L57