Skip to content

Commit 9c80bbc

Browse files
authored
feat(upsampling) - Support upsampled error count in events-stats API (#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.
1 parent 83f4055 commit 9c80bbc

File tree

7 files changed

+425
-6
lines changed

7 files changed

+425
-6
lines changed

pyproject.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ module = [
173173
"sentry.api.event_search",
174174
"sentry.api.helpers.deprecation",
175175
"sentry.api.helpers.environments",
176+
"sentry.api.helpers.error_upsampling",
176177
"sentry.api.helpers.group_index.delete",
177178
"sentry.api.helpers.group_index.update",
178179
"sentry.api.helpers.source_map_helper",
@@ -460,6 +461,7 @@ module = [
460461
"tests.sentry.api.endpoints.issues.test_organization_derive_code_mappings",
461462
"tests.sentry.api.endpoints.test_browser_reporting_collector",
462463
"tests.sentry.api.endpoints.test_project_repo_path_parsing",
464+
"tests.sentry.api.helpers.test_error_upsampling",
463465
"tests.sentry.audit_log.services.*",
464466
"tests.sentry.deletions.test_group",
465467
"tests.sentry.event_manager.test_event_manager",

src/sentry/api/endpoints/organization_events_stats.py

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@
1111
from sentry.api.api_publish_status import ApiPublishStatus
1212
from sentry.api.base import region_silo_endpoint
1313
from sentry.api.bases import OrganizationEventsV2EndpointBase
14+
from sentry.api.helpers.error_upsampling import (
15+
is_errors_query_for_error_upsampled_projects,
16+
transform_query_columns_for_error_upsampling,
17+
)
1418
from sentry.constants import MAX_TOP_EVENTS
1519
from sentry.models.dashboard_widget import DashboardWidget, DashboardWidgetTypes
1620
from sentry.models.organization import Organization
@@ -117,7 +121,7 @@ def get(self, request: Request, organization: Organization) -> Response:
117121
status=400,
118122
)
119123
elif top_events <= 0:
120-
return Response({"detail": "If topEvents needs to be at least 1"}, status=400)
124+
return Response({"detail": "topEvents needs to be at least 1"}, status=400)
121125

122126
comparison_delta = None
123127
if "comparisonDelta" in request.GET:
@@ -211,12 +215,19 @@ def _get_event_stats(
211215
zerofill_results: bool,
212216
comparison_delta: timedelta | None,
213217
) -> SnubaTSResult | dict[str, SnubaTSResult]:
218+
should_upsample = is_errors_query_for_error_upsampled_projects(
219+
snuba_params, organization, dataset, request
220+
)
221+
final_columns = query_columns
222+
if should_upsample:
223+
final_columns = transform_query_columns_for_error_upsampling(query_columns)
224+
214225
if top_events > 0:
215226
if use_rpc:
216227
return scoped_dataset.run_top_events_timeseries_query(
217228
params=snuba_params,
218229
query_string=query,
219-
y_axes=query_columns,
230+
y_axes=final_columns,
220231
raw_groupby=self.get_field_list(organization, request),
221232
orderby=self.get_orderby(request),
222233
limit=top_events,
@@ -231,7 +242,7 @@ def _get_event_stats(
231242
equations=self.get_equation_list(organization, request),
232243
)
233244
return scoped_dataset.top_events_timeseries(
234-
timeseries_columns=query_columns,
245+
timeseries_columns=final_columns,
235246
selected_columns=self.get_field_list(organization, request),
236247
equations=self.get_equation_list(organization, request),
237248
user_query=query,
@@ -255,7 +266,7 @@ def _get_event_stats(
255266
return scoped_dataset.run_timeseries_query(
256267
params=snuba_params,
257268
query_string=query,
258-
y_axes=query_columns,
269+
y_axes=final_columns,
259270
referrer=referrer,
260271
config=SearchResolverConfig(
261272
auto_fields=False,
@@ -268,7 +279,7 @@ def _get_event_stats(
268279
)
269280

270281
return scoped_dataset.timeseries_query(
271-
selected_columns=query_columns,
282+
selected_columns=final_columns,
272283
query=query,
273284
snuba_params=snuba_params,
274285
rollup=rollup,
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
from collections.abc import Sequence
2+
from types import ModuleType
3+
from typing import Any
4+
5+
from rest_framework.request import Request
6+
7+
from sentry import options
8+
from sentry.models.organization import Organization
9+
from sentry.search.events.types import SnubaParams
10+
11+
12+
def is_errors_query_for_error_upsampled_projects(
13+
snuba_params: SnubaParams,
14+
organization: Organization,
15+
dataset: ModuleType,
16+
request: Request,
17+
) -> bool:
18+
"""
19+
Determine if this query should use error upsampling transformations.
20+
Only applies when ALL projects are allowlisted and we're querying error events.
21+
"""
22+
if not _are_all_projects_error_upsampled(snuba_params.project_ids, organization):
23+
return False
24+
25+
return _should_apply_sample_weight_transform(dataset, request)
26+
27+
28+
def _are_all_projects_error_upsampled(
29+
project_ids: Sequence[int], organization: Organization
30+
) -> bool:
31+
"""
32+
Check if ALL projects in the query are allowlisted for error upsampling.
33+
Only returns True if all projects pass the allowlist condition.
34+
"""
35+
if not project_ids:
36+
return False
37+
38+
allowlist = options.get("issues.client_error_sampling.project_allowlist", [])
39+
if not allowlist:
40+
return False
41+
42+
# All projects must be in the allowlist
43+
result = all(project_id in allowlist for project_id in project_ids)
44+
return result
45+
46+
47+
def transform_query_columns_for_error_upsampling(
48+
query_columns: Sequence[str],
49+
) -> list[str]:
50+
"""
51+
Transform aggregation functions to use sum(sample_weight) instead of count()
52+
for error upsampling. Only called when all projects are allowlisted.
53+
"""
54+
transformed_columns = []
55+
for column in query_columns:
56+
column_lower = column.lower().strip()
57+
58+
if column_lower == "count()":
59+
# Simple count becomes sum of sample weights
60+
transformed_columns.append("upsampled_count() as count")
61+
62+
else:
63+
transformed_columns.append(column)
64+
65+
return transformed_columns
66+
67+
68+
def _should_apply_sample_weight_transform(dataset: Any, request: Request) -> bool:
69+
"""
70+
Determine if we should apply sample_weight transformations based on the dataset
71+
and query context. Only apply for error events since sample_weight doesn't exist
72+
for transactions.
73+
"""
74+
from sentry.snuba import discover, errors
75+
76+
# Always apply for the errors dataset
77+
if dataset == errors:
78+
return True
79+
80+
from sentry.snuba import transactions
81+
82+
# Never apply for the transactions dataset
83+
if dataset == transactions:
84+
return False
85+
86+
# For the discover dataset, check if we're querying errors specifically
87+
if dataset == discover:
88+
result = _is_error_focused_query(request)
89+
return result
90+
91+
# For other datasets (spans, metrics, etc.), don't apply
92+
return False
93+
94+
95+
def _is_error_focused_query(request: Request) -> bool:
96+
"""
97+
Check if a query is focused on error events.
98+
Reduced to only check for event.type:error to err on the side of caution.
99+
"""
100+
query = request.GET.get("query", "").lower()
101+
102+
if "event.type:error" in query:
103+
return True
104+
105+
return False

src/sentry/search/events/datasets/discover.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1038,6 +1038,16 @@ def function_converter(self) -> Mapping[str, SnQLFunction]:
10381038
default_result_type="integer",
10391039
private=True,
10401040
),
1041+
SnQLFunction(
1042+
"upsampled_count",
1043+
required_args=[],
1044+
snql_aggregate=lambda args, alias: Function(
1045+
"toInt64",
1046+
[Function("sum", [Function("ifNull", [Column("sample_weight"), 1])])],
1047+
alias,
1048+
),
1049+
default_result_type="number",
1050+
),
10411051
]
10421052
}
10431053

src/sentry/testutils/factories.py

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import zipfile
99
from base64 import b64encode
1010
from binascii import hexlify
11-
from collections.abc import Mapping, Sequence
11+
from collections.abc import Mapping, MutableMapping, Sequence
1212
from datetime import UTC, datetime
1313
from enum import Enum
1414
from hashlib import sha1
@@ -341,6 +341,22 @@ def _patch_artifact_manifest(path, org=None, release=None, project=None, extra_f
341341
return orjson.dumps(manifest).decode()
342342

343343

344+
def _set_sample_rate_from_error_sampling(normalized_data: MutableMapping[str, Any]) -> None:
345+
"""Set 'sample_rate' on normalized_data if contexts.error_sampling.client_sample_rate is present and valid."""
346+
client_sample_rate = None
347+
try:
348+
client_sample_rate = (
349+
normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate")
350+
)
351+
except Exception:
352+
pass
353+
if client_sample_rate:
354+
try:
355+
normalized_data["sample_rate"] = float(client_sample_rate)
356+
except Exception:
357+
pass
358+
359+
344360
# TODO(dcramer): consider moving to something more scalable like factoryboy
345361
class Factories:
346362
@staticmethod
@@ -1029,6 +1045,9 @@ def store_event(
10291045
assert not errors, errors
10301046

10311047
normalized_data = manager.get_data()
1048+
1049+
_set_sample_rate_from_error_sampling(normalized_data)
1050+
10321051
event = None
10331052

10341053
# When fingerprint is present on transaction, inject performance problems
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
from unittest.mock import Mock, patch
2+
3+
from django.http import QueryDict
4+
from django.test import RequestFactory
5+
from rest_framework.request import Request
6+
7+
from sentry.api.helpers.error_upsampling import (
8+
_are_all_projects_error_upsampled,
9+
_is_error_focused_query,
10+
_should_apply_sample_weight_transform,
11+
transform_query_columns_for_error_upsampling,
12+
)
13+
from sentry.models.organization import Organization
14+
from sentry.search.events.types import SnubaParams
15+
from sentry.snuba import discover, errors, transactions
16+
from sentry.testutils.cases import TestCase
17+
18+
19+
class ErrorUpsamplingTest(TestCase):
20+
def setUp(self) -> None:
21+
self.organization = Organization.objects.create(name="test-org")
22+
self.projects = [
23+
self.create_project(organization=self.organization, name="Project 1"),
24+
self.create_project(organization=self.organization, name="Project 2"),
25+
self.create_project(organization=self.organization, name="Project 3"),
26+
]
27+
self.project_ids = [p.id for p in self.projects]
28+
self.snuba_params = SnubaParams(
29+
start=None,
30+
end=None,
31+
projects=self.projects,
32+
)
33+
factory = RequestFactory()
34+
self.request = Request(factory.get("/"))
35+
self.request.GET = QueryDict("")
36+
37+
@patch("sentry.api.helpers.error_upsampling.options")
38+
def test_are_all_projects_error_upsampled(self, mock_options: Mock) -> None:
39+
# Test when all projects are allowlisted
40+
mock_options.get.return_value = self.project_ids
41+
assert _are_all_projects_error_upsampled(self.project_ids, self.organization) is True
42+
43+
# Test when some projects are not allowlisted
44+
mock_options.get.return_value = self.project_ids[:-1]
45+
assert _are_all_projects_error_upsampled(self.project_ids, self.organization) is False
46+
47+
# Test when no projects are allowlisted
48+
mock_options.get.return_value = []
49+
assert _are_all_projects_error_upsampled(self.project_ids, self.organization) is False
50+
51+
# Test when no project IDs provided
52+
assert _are_all_projects_error_upsampled([], self.organization) is False
53+
54+
def test_transform_query_columns_for_error_upsampling(self) -> None:
55+
# Test count() transformation
56+
columns = ["count()", "other_column"]
57+
expected = [
58+
"upsampled_count() as count",
59+
"other_column",
60+
]
61+
assert transform_query_columns_for_error_upsampling(columns) == expected
62+
63+
# Test case insensitivity
64+
columns = ["COUNT()"]
65+
expected = [
66+
"upsampled_count() as count",
67+
]
68+
assert transform_query_columns_for_error_upsampling(columns) == expected
69+
70+
# Test whitespace handling
71+
columns = [" count() "]
72+
expected = [
73+
"upsampled_count() as count",
74+
]
75+
assert transform_query_columns_for_error_upsampling(columns) == expected
76+
77+
def test_is_error_focused_query(self) -> None:
78+
# Test explicit error type
79+
self.request.GET = QueryDict("query=event.type:error")
80+
assert _is_error_focused_query(self.request) is True
81+
82+
# Test explicit transaction type
83+
self.request.GET = QueryDict("query=event.type:transaction")
84+
assert _is_error_focused_query(self.request) is False
85+
86+
# Test empty query
87+
self.request.GET = QueryDict("")
88+
assert _is_error_focused_query(self.request) is False
89+
90+
def test_should_apply_sample_weight_transform(self) -> None:
91+
# Test errors dataset
92+
assert _should_apply_sample_weight_transform(errors, self.request) is True
93+
94+
# Test transactions dataset
95+
assert _should_apply_sample_weight_transform(transactions, self.request) is False
96+
97+
self.request.GET = QueryDict("query=event.type:error")
98+
assert _should_apply_sample_weight_transform(discover, self.request) is True
99+
100+
self.request.GET = QueryDict("query=event.type:transaction")
101+
assert _should_apply_sample_weight_transform(discover, self.request) is False

0 commit comments

Comments
 (0)