Repository: getsentry/sentry
Pull request: feat(sort): properly calculate sort score for issue-platform occurrences
This PR parameterizes the `better_priority` aggregation to consider errors vs issue-platform events and whether to use certain columns as part of the better priority scoring. 

The only difference between the formulas for `better_priority` errors and issue-platform are the timestamp columns used and whether to consider if an event has a stacktrace in the aggregation. 

Resolves https://github.com/getsentry/sentry/issues/50172

Problem statement:
Implement more robust priority scoring for issue platform issues
Currently, we're assigning a score of zero to performance and profiling issues when using betterPriority sort which de-ranks these issues to the very bottom of the sorted issues. Instead, we should consider the data-set we're querying from and exclude columns that aren't present from the score.

Hints from issue discussion:

Commit message: params

Patch to review:
diff --git a/src/sentry/search/snuba/executors.py b/src/sentry/search/snuba/executors.py
index 87166c07de2fa5..0a5eb40a93d02a 100644
--- a/src/sentry/search/snuba/executors.py
+++ b/src/sentry/search/snuba/executors.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import dataclasses
 import functools
 import logging
 import time
@@ -237,7 +238,9 @@ def _prepare_aggregations(
             aggregation = self.aggregation_defs[alias]
             # TODO: remove this hack once we can properly support better_priority sort on issue platform dataset
             if replace_better_priority_aggregation and alias == "better_priority":
-                aggregation = self.aggregation_defs["force_last"]  # type:ignore[call-overload]
+                aggregation = self.aggregation_defs[
+                    "better_priority_platform"  # type:ignore[call-overload]
+                ]
             if callable(aggregation):
                 if aggregate_kwargs:
                     aggregation = aggregation(start, end, aggregate_kwargs.get(alias, {}))
@@ -491,24 +494,92 @@ def has_sort_strategy(self, sort_by: str) -> bool:
         return sort_by in self.sort_strategies.keys()
 
 
+@dataclasses.dataclass
+class BetterPriorityParams:
+    # (event or issue age_hours) / (event or issue halflife hours)
+    # any event or issue age that is greater than max_pow times the half-life hours will get clipped
+    max_pow: int
+    min_score: float  # apply a min on the individual scores to avoid multiplying by zeroes
+
+    # event-aggregate scoring
+    event_age_weight: int  # [1, 5]
+    log_level_weight: int  # [0, 10]
+    stacktrace_weight: int  # [0, 3]
+    event_halflife_hours: int  # halves score every x hours
+
+    # issue-aggregate scoring
+    issue_age_weight: int  # [1, 5]
+    issue_halflife_hours: int  # halves score every x hours
+    relative_volume_weight: int  # [0, 10]
+
+    #
+    v2: bool
+    normalize: bool
+
+
 def better_priority_aggregation(
     start: datetime,
     end: datetime,
     aggregate_kwargs: PrioritySortWeights,
 ) -> Sequence[str]:
-    issue_age_weight = 1  # [1, 5]
-    event_age_weight = 1  # [1, 5]
-    log_level_weight = aggregate_kwargs["log_level"]  # [0, 10]
-    stacktrace_weight = aggregate_kwargs["has_stacktrace"]  # [0, 3]
-    # (event or issue age_hours) / (event or issue halflife hours)
-    # any event or issue age that is greater than max_pow times the half-life hours will get clipped
-    max_pow = 16
-    min_score = 0.01
-    event_age_hours = "divide(now() - timestamp, 3600)"
-    event_halflife_hours = aggregate_kwargs["event_halflife_hours"]  # halves score every x hours
-    issue_halflife_hours = aggregate_kwargs["issue_halflife_hours"]  # halves score every x hours
-    issue_age_hours = "divide(now() - min(timestamp), 3600)"
+    return better_priority_aggregation_impl(
+        BetterPriorityParams(
+            max_pow=16,
+            min_score=0.01,
+            event_age_weight=1,
+            log_level_weight=aggregate_kwargs["log_level"],
+            stacktrace_weight=aggregate_kwargs["has_stacktrace"],
+            event_halflife_hours=aggregate_kwargs["event_halflife_hours"],
+            issue_age_weight=1,
+            issue_halflife_hours=aggregate_kwargs["issue_halflife_hours"],
+            relative_volume_weight=aggregate_kwargs["relative_volume"],
+            v2=aggregate_kwargs["v2"],
+            normalize=aggregate_kwargs["norm"],
+        ),
+        "timestamp",
+        True,
+    )
+
+
+def better_priority_platform_aggregation(
+    start: datetime,
+    end: datetime,
+    aggregate_kwargs: PrioritySortWeights,
+) -> Sequence[str]:
+    return better_priority_aggregation_impl(
+        BetterPriorityParams(
+            max_pow=16,
+            min_score=0.01,
+            event_age_weight=1,
+            log_level_weight=aggregate_kwargs["log_level"],
+            stacktrace_weight=0,  # issue-platform occurrences won't have stacktrace
+            event_halflife_hours=aggregate_kwargs["event_halflife_hours"],
+            issue_age_weight=1,
+            issue_halflife_hours=aggregate_kwargs["issue_halflife_hours"],
+            relative_volume_weight=aggregate_kwargs["relative_volume"],
+            v2=aggregate_kwargs["v2"],
+            normalize=aggregate_kwargs["norm"],
+        ),
+        "client_timestamp",
+        False,
+    )
 
+
+def better_priority_aggregation_impl(
+    params: BetterPriorityParams, timestamp_column: str, use_stacktrace: bool
+) -> Sequence[str]:
+    min_score = params.min_score
+    max_pow = params.max_pow
+    event_age_weight = params.event_age_weight
+    event_halflife_hours = params.event_halflife_hours
+    log_level_weight = params.log_level_weight
+    stacktrace_weight = params.stacktrace_weight
+    relative_volume_weight = params.relative_volume_weight
+    issue_age_weight = params.issue_age_weight
+    issue_halflife_hours = params.issue_halflife_hours
+
+    event_age_hours = f"divide(now() - {timestamp_column}, 3600)"
+    issue_age_hours = f"divide(now() - min({timestamp_column}), 3600)"
     log_level_score = "multiIf(equals(level, 'fatal'), 1.0, equals(level, 'error'), 0.66, equals(level, 'warning'), 0.33, 0.0)"
     stacktrace_score = "if(notEmpty(exception_stacks.type), 1.0, 0.0)"
     # event_agg_rank:
@@ -523,7 +594,13 @@ def better_priority_aggregation(
     # f(x) = ---------------------------------  ,  max(f(x)) = 1, when individual scores are all 1
     #                  lw + sw + aw
     #
-    event_agg_numerator = f"plus(plus(multiply({log_level_score}, {log_level_weight}), multiply({stacktrace_score}, {stacktrace_weight})), {event_age_weight})"
+    if use_stacktrace:
+        event_agg_numerator = f"plus(plus(multiply({log_level_score}, {log_level_weight}), multiply({stacktrace_score}, {stacktrace_weight})), {event_age_weight})"
+    else:
+        event_agg_numerator = (
+            f"plus(multiply({log_level_score}, {log_level_weight}), {event_age_weight})"
+        )
+
     event_agg_denominator = (
         f"plus(plus({log_level_weight}, {stacktrace_weight}), {event_age_weight})"
     )
@@ -531,9 +608,7 @@ def better_priority_aggregation(
 
     aggregate_issue_score = f"greatest({min_score}, divide({issue_age_weight}, pow(2, least({max_pow}, divide({issue_age_hours}, {issue_halflife_hours})))))"
 
-    v2 = aggregate_kwargs["v2"]
-
-    if not v2:
+    if not params.v2:
         aggregate_event_score = f"greatest({min_score}, sum(divide({event_agg_rank}, pow(2, least({max_pow}, divide({event_age_hours}, {event_halflife_hours}))))))"
         return [f"multiply({aggregate_event_score}, {aggregate_issue_score})", ""]
     else:
@@ -564,11 +639,9 @@ def better_priority_aggregation(
         # ln(h(x)) = [ln(1), ln(+inf)] = 0, 1, ~2.30, ~6.09,  ~13.81,     ~20.72, +inf
         aggregate_event_score = f"log(plus(1, sum(divide({event_agg_rank}, pow(2, divide({event_age_hours}, {event_halflife_hours}))))))"
 
-        event_count_60_mins = "countIf(lessOrEquals(minus(now(), timestamp), 3600))"
-        avg_hourly_event_count_last_7_days = (
-            "countIf(lessOrEquals(minus(now(), timestamp), 604800))"  # 604800 = 3600 * 24 * 7
-        )
-        relative_volume_weight = aggregate_kwargs["relative_volume"]  # [0, 10]
+        event_count_60_mins = f"countIf(lessOrEquals(minus(now(), {timestamp_column}), 3600))"
+        avg_hourly_event_count_last_7_days = f"countIf(lessOrEquals(minus(now(), {timestamp_column}), 604800))"  # 604800 = 3600 * 24 * 7
+
         max_relative_volume_weight = 10
         if relative_volume_weight > max_relative_volume_weight:
             relative_volume_weight = max_relative_volume_weight
@@ -577,9 +650,7 @@ def better_priority_aggregation(
         )
         scaled_relative_volume_score = f"divide(multiply({relative_volume_weight}, {relative_volume_score}), {max_relative_volume_weight})"
 
-        normalize = aggregate_kwargs["norm"]
-
-        if not normalize:
+        if not params.normalize:
             return [
                 f"multiply(multiply({aggregate_issue_score}, greatest({min_score}, {aggregate_event_score})), greatest({min_score}, {scaled_relative_volume_score}))",
                 "",
@@ -639,10 +710,7 @@ class PostgresSnubaQueryExecutor(AbstractQueryExecutor):
         "total": ["uniq", ISSUE_FIELD_NAME],
         "user_count": ["uniq", "tags[sentry:user]"],
         "better_priority": better_priority_aggregation,
-        "force_last": [
-            "least(min(organization_id), 0)",
-            "",
-        ],  # hack aggregation to force issue-platform sort scores to be zero
+        "better_priority_platform": better_priority_platform_aggregation,
     }
 
     @property
diff --git a/tests/snuba/search/test_backend.py b/tests/snuba/search/test_backend.py
index 3147e022445742..6092a075853118 100644
--- a/tests/snuba/search/test_backend.py
+++ b/tests/snuba/search/test_backend.py
@@ -2470,7 +2470,7 @@ def test_better_priority_mixed_group_types(self):
         error_group_score = results[0][1]
         profile_group_score = results[1][1]
         assert error_group_score > 0
-        assert profile_group_score == 0
+        assert profile_group_score > 0
 
 
 class EventsTransactionsSnubaSearchTest(SharedSnubaTest):


Code review comments:
diff_hunk: @@ -237,7 +238,9 @@ def _prepare_aggregations(
             aggregation = self.aggregation_defs[alias]
             # TODO: remove this hack once we can properly support better_priority sort on issue platform dataset
path: src/sentry/search/snuba/executors.py
text: @user1:
Should this TODO be removed now?

diff_hunk: @@ -491,24 +494,92 @@ def has_sort_strategy(self, sort_by: str) -> bool:
         return sort_by in self.sort_strategies.keys()
 
 
+@dataclasses.dataclass
+class BetterPriorityParams:
path: src/sentry/search/snuba/executors.py
text: @user1:
Can you move this to the top of the file near `PrioritySortWeights` and whatnot?

diff_hunk: @@ -491,24 +494,92 @@ def has_sort_strategy(self, sort_by: str) -> bool:
         return sort_by in self.sort_strategies.keys()
 
 
+@dataclasses.dataclass
+class BetterPriorityParams:
+    # (event or issue age_hours) / (event or issue halflife hours)
+    # any event or issue age that is greater than max_pow times the half-life hours will get clipped
+    max_pow: int
+    min_score: float  # apply a min on the individual scores to avoid multiplying by zeroes
+
+    # event-aggregate scoring
+    event_age_weight: int  # [1, 5]
+    log_level_weight: int  # [0, 10]
+    stacktrace_weight: int  # [0, 3]
+    event_halflife_hours: int  # halves score every x hours
+
+    # issue-aggregate scoring
+    issue_age_weight: int  # [1, 5]
+    issue_halflife_hours: int  # halves score every x hours
+    relative_volume_weight: int  # [0, 10]
+
+    #
+    v2: bool
+    normalize: bool
+
+
 def better_priority_aggregation(
     start: datetime,
     end: datetime,
     aggregate_kwargs: PrioritySortWeights,
 ) -> Sequence[str]:
-    issue_age_weight = 1  # [1, 5]
-    event_age_weight = 1  # [1, 5]
-    log_level_weight = aggregate_kwargs["log_level"]  # [0, 10]
-    stacktrace_weight = aggregate_kwargs["has_stacktrace"]  # [0, 3]
-    # (event or issue age_hours) / (event or issue halflife hours)
-    # any event or issue age that is greater than max_pow times the half-life hours will get clipped
-    max_pow = 16
-    min_score = 0.01
-    event_age_hours = "divide(now() - timestamp, 3600)"
-    event_halflife_hours = aggregate_kwargs["event_halflife_hours"]  # halves score every x hours
-    issue_halflife_hours = aggregate_kwargs["issue_halflife_hours"]  # halves score every x hours
-    issue_age_hours = "divide(now() - min(timestamp), 3600)"
+    return better_priority_aggregation_impl(
+        BetterPriorityParams(
+            max_pow=16,
+            min_score=0.01,
+            event_age_weight=1,
+            log_level_weight=aggregate_kwargs["log_level"],
+            stacktrace_weight=aggregate_kwargs["has_stacktrace"],
+            event_halflife_hours=aggregate_kwargs["event_halflife_hours"],
+            issue_age_weight=1,
+            issue_halflife_hours=aggregate_kwargs["issue_halflife_hours"],
+            relative_volume_weight=aggregate_kwargs["relative_volume"],
+            v2=aggregate_kwargs["v2"],
+            normalize=aggregate_kwargs["norm"],
+        ),
+        "timestamp",
+        True,
+    )
+
+
+def better_priority_platform_aggregation(
path: src/sentry/search/snuba/executors.py
text: @user1:
Maybe `better_priority_issue_platform_aggregation` would be clearer, took me a second to realize what this was for.

diff_hunk: @@ -491,24 +494,92 @@ def has_sort_strategy(self, sort_by: str) -> bool:
         return sort_by in self.sort_strategies.keys()
 
 
+@dataclasses.dataclass
path: src/sentry/search/snuba/executors.py
text: @user1:
Can you do `from dataclasses import dataclass` instead to match how we import this in other files?