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
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ module = [
"sentry.api.event_search",
"sentry.api.helpers.deprecation",
"sentry.api.helpers.environments",
"sentry.api.helpers.error_upsampling",
"sentry.api.helpers.group_index.delete",
"sentry.api.helpers.group_index.update",
"sentry.api.helpers.source_map_helper",
Expand Down Expand Up @@ -460,6 +461,7 @@ module = [
"tests.sentry.api.endpoints.issues.test_organization_derive_code_mappings",
"tests.sentry.api.endpoints.test_browser_reporting_collector",
"tests.sentry.api.endpoints.test_project_repo_path_parsing",
"tests.sentry.api.helpers.test_error_upsampling",
"tests.sentry.audit_log.services.*",
"tests.sentry.deletions.test_group",
"tests.sentry.event_manager.test_event_manager",
Expand Down
1 change: 1 addition & 0 deletions sentry-repo
Submodule sentry-repo added at a5d290
38 changes: 33 additions & 5 deletions src/sentry/api/endpoints/organization_events_stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
)
Comment on lines +220 to +222

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Base the upsampling decision on the effective query, not the outer request state.

_get_event_stats() can run with a rewritten dataset/query, but this check still reads the outer dataset plus request.GET["query"]. In the dashboard split paths that means eligibility is evaluated against the original request instead of the query you actually send to Snuba, so upsampling can be skipped or applied incorrectly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/endpoints/organization_events_stats.py` around lines 220 -
222, The upsampling check is using the outer dataset and request state instead
of the effective query/dataset used to call Snuba; update the call to
is_errors_query_for_error_upsampled_projects inside _get_event_stats() to pass
the rewritten/effective values from snuba_params (e.g., the effective query and
dataset stored on snuba_params) and organization instead of using the outer
dataset and request.GET["query"], so eligibility is evaluated against the actual
query sent to Snuba.


# 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:
final_columns = transform_query_columns_for_error_upsampling(query_columns)

if use_rpc:
return scoped_dataset.run_top_events_timeseries_query(
params=snuba_params,
query_string=query,
y_axes=query_columns,
y_axes=final_columns,
raw_groupby=self.get_field_list(organization, request),
orderby=self.get_orderby(request),
limit=top_events,
Expand All @@ -231,7 +251,7 @@ def _get_event_stats(
equations=self.get_equation_list(organization, request),
)
return scoped_dataset.top_events_timeseries(
timeseries_columns=query_columns,
timeseries_columns=final_columns,
selected_columns=self.get_field_list(organization, request),
equations=self.get_equation_list(organization, request),
user_query=query,
Expand All @@ -252,10 +272,14 @@ def _get_event_stats(
)

if use_rpc:
# Apply upsampling transformation just before RPC query execution
if upsampling_enabled:
final_columns = transform_query_columns_for_error_upsampling(query_columns)

return scoped_dataset.run_timeseries_query(
params=snuba_params,
query_string=query,
y_axes=query_columns,
y_axes=final_columns,
referrer=referrer,
config=SearchResolverConfig(
auto_fields=False,
Expand All @@ -267,8 +291,12 @@ def _get_event_stats(
comparison_delta=comparison_delta,
)

# Apply upsampling transformation just before standard query execution
if upsampling_enabled:
final_columns = transform_query_columns_for_error_upsampling(query_columns)

return scoped_dataset.timeseries_query(
selected_columns=query_columns,
selected_columns=final_columns,
query=query,
snuba_params=snuba_params,
rollup=rollup,
Expand Down
140 changes: 140 additions & 0 deletions src/sentry/api/helpers/error_upsampling.py
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)))}"

# Check cache first for performance optimization
cached_result = cache.get(cache_key)
if cached_result is not None:
return cached_result and _should_apply_sample_weight_transform(dataset, request)

# Cache miss - perform fresh allowlist check
is_eligible = _are_all_projects_error_upsampled(snuba_params.project_ids, organization)

# Cache for 60 seconds to improve performance during traffic spikes
cache.set(cache_key, is_eligible, 60)
Comment on lines +27 to +38

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use a deterministic cache key instead of hash(...).

hash(tuple(sorted(...))) is salted per Python process, so different workers will generate different keys for the same project set. That means invalidate_upsampling_cache() can miss entries written by another worker, leaving stale allowlist decisions around until the TTL expires.

Suggested fix
-    cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}"
+    project_key = ",".join(str(project_id) for project_id in sorted(snuba_params.project_ids))
+    cache_key = f"error_upsampling_eligible:{organization.id}:{project_key}"
...
-    cache_key = f"error_upsampling_eligible:{organization_id}:{hash(tuple(sorted(project_ids)))}"
+    project_key = ",".join(str(project_id) for project_id in sorted(project_ids))
+    cache_key = f"error_upsampling_eligible:{organization_id}:{project_key}"

Also applies to: 73-74

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/helpers/error_upsampling.py` around lines 27 - 38, The cache
key uses Python's process-salted hash which is non-deterministic across workers;
replace hash(tuple(sorted(snuba_params.project_ids))) with a deterministic
representation (e.g., join the sorted project IDs into a string or a stable
digest like hashlib.sha256 of the joined IDs) when building cache_key in the
error upsampling flow (the block that computes cache_key and uses
cache.get/cache.set) and apply the same deterministic construction in the other
occurrence referenced (the lines around the second use). Ensure you keep the
sort to make ordering stable, reference snuba_params.project_ids, and update any
related helpers such as _are_all_projects_error_upsampled and
invalidate_upsampling_cache usage to use the same deterministic key construction
so invalidation works across workers.


return is_eligible and _should_apply_sample_weight_transform(dataset, request)


def _are_all_projects_error_upsampled(
project_ids: Sequence[int], organization: Organization
) -> bool:
"""
Check if ALL projects in the query are allowlisted for error upsampling.
Only returns True if all projects pass the allowlist condition.

NOTE: This function reads the allowlist configuration fresh each time,
which means it can return different results between calls if the
configuration changes during request processing. This is intentional
to ensure we always have the latest configuration state.
"""
if not project_ids:
return False

allowlist = options.get("issues.client_error_sampling.project_allowlist", [])
if not allowlist:
return False

# All projects must be in the allowlist
result = all(project_id in allowlist for project_id in project_ids)
return result


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)


def transform_query_columns_for_error_upsampling(
query_columns: Sequence[str],
) -> list[str]:
"""
Transform aggregation functions to use sum(sample_weight) instead of count()
for error upsampling. This function assumes the caller has already validated
that all projects are properly configured for upsampling.

Note: We rely on the database schema to ensure sample_weight exists for all
events in allowlisted projects, so no additional null checks are needed here.
"""
transformed_columns = []
for column in query_columns:
column_lower = column.lower().strip()

if column_lower == "count()":
# Transform to upsampled count - assumes sample_weight column exists
# for all events in allowlisted projects per our data model requirements
transformed_columns.append("upsampled_count() as count")

else:
transformed_columns.append(column)

return transformed_columns


def _should_apply_sample_weight_transform(dataset: Any, request: Request) -> bool:
"""
Determine if we should apply sample_weight transformations based on the dataset
and query context. Only apply for error events since sample_weight doesn't exist
for transactions.
"""
from sentry.snuba import discover, errors

# Always apply for the errors dataset
if dataset == errors:
return True

from sentry.snuba import transactions

# Never apply for the transactions dataset
if dataset == transactions:
return False

# For the discover dataset, check if we're querying errors specifically
if dataset == discover:
result = _is_error_focused_query(request)
return result

# For other datasets (spans, metrics, etc.), don't apply
return False


def _is_error_focused_query(request: Request) -> bool:
"""
Check if a query is focused on error events.
Reduced to only check for event.type:error to err on the side of caution.
"""
query = request.GET.get("query", "").lower()

if "event.type:error" in query:
return True

return False
12 changes: 12 additions & 0 deletions src/sentry/search/events/datasets/discover.py
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,18 @@ def function_converter(self) -> Mapping[str, SnQLFunction]:
default_result_type="integer",
private=True,
),
SnQLFunction(
"upsampled_count",
required_args=[],
# Optimized aggregation for error upsampling - assumes sample_weight
# exists for all events in allowlisted projects as per schema design
snql_aggregate=lambda args, alias: Function(
"toInt64",
[Function("sum", [Column("sample_weight")])],
alias,
),
default_result_type="number",
Comment on lines +1041 to +1051

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep upsampled_count typed as an integer.

This aggregate returns toInt64(sum(sample_weight)), but it is registered as "number". Since the endpoint aliases it back to count, upsampled responses will advertise a different type than normal count(), which can break metadata consumers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/search/events/datasets/discover.py` around lines 1041 - 1051, The
SnQLFunction "upsampled_count" currently sets default_result_type="number" but
returns toInt64(sum(sample_weight)); update the SnQLFunction definition for
"upsampled_count" to use an integer result type (e.g.,
default_result_type="integer") so its advertised type matches normal count()
responses and downstream metadata consumers remain consistent.

),
]
}

Expand Down
21 changes: 20 additions & 1 deletion src/sentry/testutils/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import zipfile
from base64 import b64encode
from binascii import hexlify
from collections.abc import Mapping, Sequence
from collections.abc import Mapping, MutableMapping, Sequence
from datetime import UTC, datetime
from enum import Enum
from hashlib import sha1
Expand Down Expand Up @@ -341,6 +341,22 @@ def _patch_artifact_manifest(path, org=None, release=None, project=None, extra_f
return orjson.dumps(manifest).decode()


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:
pass
if client_sample_rate:
try:
normalized_data["sample_rate"] = float(client_sample_rate)
except Exception:
pass
Comment on lines +347 to +357

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace bare exception handlers with specific exception types.

The bare except Exception: pass blocks silently swallow all errors, making debugging difficult and potentially hiding real issues. Catch only the specific exceptions expected during dict traversal and type conversion.

🛡️ Proposed fix
     client_sample_rate = None
     try:
         client_sample_rate = (
             normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate")
         )
-    except Exception:
+    except (AttributeError, KeyError, TypeError):
         pass
-    if client_sample_rate:
+    if client_sample_rate is not None:
         try:
             normalized_data["sample_rate"] = float(client_sample_rate)
-        except Exception:
+        except (TypeError, ValueError):
             pass
🧰 Tools
🪛 Ruff (0.15.15)

[error] 351-352: try-except-pass detected, consider logging the exception

(S110)


[warning] 351-351: Do not catch blind exception: Exception

(BLE001)


[error] 356-357: try-except-pass detected, consider logging the exception

(S110)


[warning] 356-356: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/testutils/factories.py` around lines 347 - 357, The current bare
excepts around extracting client_sample_rate and converting it to float should
be replaced with specific exceptions: when retrieving nested keys from
normalized_data (the expression using normalized_data.get("contexts",
{}).get("error_sampling", {}).get("client_sample_rate")), catch only
AttributeError and TypeError (to handle cases where contexts or error_sampling
are not mappings) instead of Exception; when assigning
normalized_data["sample_rate"] = float(client_sample_rate) catch only ValueError
and TypeError (to handle invalid string/None conversions). Update the try/except
blocks around client_sample_rate and the float conversion accordingly,
referencing normalized_data and client_sample_rate/sample_rate to locate the
code.

Comment on lines +353 to +357

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Truthiness check prevents setting sample_rate=0.0.

Line 353's if client_sample_rate: evaluates to False when client_sample_rate is 0 or 0.0, preventing valid zero sample rates from being set. A sample rate of 0.0 is semantically valid (indicates all events should be dropped during sampling).

🔧 Proposed fix
     except Exception:
         pass
-    if client_sample_rate:
+    if client_sample_rate is not None:
         try:
             normalized_data["sample_rate"] = float(client_sample_rate)
         except Exception:
🧰 Tools
🪛 Ruff (0.15.15)

[error] 356-357: try-except-pass detected, consider logging the exception

(S110)


[warning] 356-356: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/testutils/factories.py` around lines 353 - 357, The truthiness
check using "if client_sample_rate:" prevents valid zero values from being
applied; change the guard to an explicit None check (e.g., "if
client_sample_rate is not None:") in the block that sets
normalized_data["sample_rate"], then attempt to coerce client_sample_rate to
float (catching ValueError/TypeError) and assign it to
normalized_data["sample_rate"] so 0 and 0.0 are preserved; reference the
normalized_data dict and the client_sample_rate variable in the sample-rate
assignment logic.



# TODO(dcramer): consider moving to something more scalable like factoryboy
class Factories:
@staticmethod
Expand Down Expand Up @@ -1029,6 +1045,9 @@ def store_event(
assert not errors, errors

normalized_data = manager.get_data()

_set_sample_rate_from_error_sampling(normalized_data)

event = None

# When fingerprint is present on transaction, inject performance problems
Expand Down
Loading