Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Expand All @@ -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,
Expand All @@ -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:
Expand Down Expand Up @@ -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))
Copy link
Contributor

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_eap function has incomplete exception handling. The Organization.objects.get() call only catches DoesNotExist, not other database errors (e.g., connection timeout). The Project.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)

Fix in Cursor Fix in Web

if not projects:
return []

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})"
Comment on lines +141 to +151
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Severity: MEDIUM

Suggested Fix

Instead of building the query via f-string interpolation, use a structured query format or properly escape the release and environment values before inserting them into the query string. This will prevent special characters like quotes from breaking the query syntax.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
src/sentry/api/endpoints/release_thresholds/utils/get_errors_counts_timeseries.py#L141-L151

Potential issue: The function `get_errors_counts_timeseries` constructs a query string
for an external service using f-string interpolation. Release and environment names are
directly embedded into this string. Sentry does not prevent release or environment names
from containing quote characters. If a name includes a quote, it will break the query
syntax, causing an exception. A broad exception handler then catches this error and
silently returns an empty list, leading to data loss and creating a discrepancy between
the primary Snuba results and the results from this function.

Did 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(),
)

# 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 []
10 changes: 10 additions & 0 deletions src/sentry/search/eap/occurrences/attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@
internal_name="transaction",
search_type="string",
),
ResolvedAttribute(
public_alias="release",
internal_name="release",
search_type="string",
),
ResolvedAttribute(
public_alias="type",
internal_name="type",
search_type="string",
),
]
)
}
Loading
Loading