fix(grouping): Only run grouping calculation once (#78630)

During grouping, the slowest part of the process is calculating the variants. Before Seer and before grouphash metadata, we only needed them once, but now we use them in two places in the Seer flow and are about to use them in another place for grouphash metadata. To avoid calculating them now potentially up to four times, this PR refactors things so that they're passed from the place where they're initially calculated (in `event.get_hashes`) through the various intermediate functions to the spots in the Seer flow where they're currently used. Along this path is the place where we'll need them from grouphash metadata also.

Notes:

- In order to not have to change unrelated uses of `get_hashes`, instead of changing its return value I instead extracted most of its inner logic into a separate `get_hashes_and_variants` method. Now `get_hashes` calls `get_hashes_and_variants` (and just ignores the variants) and in the spot in ingest where we used to call `get_hashes`, we now call `get_hashes_and_variants`.

- We have a pair of helpers, `run_primary_grouping` and `maybe_run_secondary_grouping`, for calculating primary and secondary hashes, respectively, which need to have matching signatures, because they're both passed to `get_hashes_and_grouphashes` as the `hash_calculation_function` parameter. We don't ever need (or want) variants from the secondary hash calculation, so in place of real variants data `maybe_run_secondary_grouping` just passes an empty dictionary.

Author: lobsterkatie

src/sentry/event_manager.py

@@ -68,6 +68,7 @@
     check_for_group_creation_load_shed,
     is_non_error_type_group,
 )
+from sentry.grouping.variants import BaseVariant
 from sentry.ingest.inbound_filters import FilterStatKeys
 from sentry.integrations.tasks.kick_off_status_syncs import kick_off_status_syncs
 from sentry.issues.grouptype import ErrorGroupType
@@ -1318,7 +1319,9 @@ def assign_event_to_group(
             result = "found_secondary"
         # If we still haven't found a group, ask Seer for a match (if enabled for the project)
         else:
-            seer_matched_grouphash = maybe_check_seer_for_matching_grouphash(event, all_grouphashes)
+            seer_matched_grouphash = maybe_check_seer_for_matching_grouphash(
+                event, primary.variants, all_grouphashes
+            )
 
             if seer_matched_grouphash:
                 group_info = handle_existing_grouphash(job, seer_matched_grouphash, all_grouphashes)
@@ -1358,7 +1361,7 @@ def get_hashes_and_grouphashes(
     job: Job,
     hash_calculation_function: Callable[
         [Project, Job, MutableTags],
-        tuple[GroupingConfig, list[str]],
+        tuple[GroupingConfig, list[str], dict[str, BaseVariant]],
     ],
     metric_tags: MutableTags,
 ) -> GroupHashInfo:
@@ -1373,14 +1376,14 @@ def get_hashes_and_grouphashes(
     project = job["event"].project
 
     # These will come back as Nones if the calculation decides it doesn't need to run
-    grouping_config, hashes = hash_calculation_function(project, job, metric_tags)
+    grouping_config, hashes, variants = hash_calculation_function(project, job, metric_tags)
 
     if hashes:
         grouphashes = get_or_create_grouphashes(project, hashes, grouping_config["id"])
 
         existing_grouphash = find_grouphash_with_group(grouphashes)
 
-        return GroupHashInfo(grouping_config, hashes, grouphashes, existing_grouphash)
+        return GroupHashInfo(grouping_config, variants, hashes, grouphashes, existing_grouphash)
     else:
         return NULL_GROUPHASH_INFO
 

src/sentry/eventstore/models.py

@@ -332,6 +332,29 @@ def get_grouping_config(self) -> GroupingConfig:
 
         return get_grouping_config_dict_for_event_data(self.data, self.project)
 
+    def get_hashes_and_variants(
+        self, config: StrategyConfiguration | None = None
+    ) -> tuple[list[str], dict[str, BaseVariant]]:
+        """
+        Return the event's hash values, calculated using the given config, along with the
+        `variants` data used in grouping.
+        """
+
+        variants = self.get_grouping_variants(config)
+        # Sort the variants so that the system variant (if any) is always last, in order to resolve
+        # ambiguities when choosing primary_hash for Snuba
+        sorted_variants = sorted(
+            variants.items(),
+            key=lambda name_and_variant: 1 if name_and_variant[0] == "system" else 0,
+        )
+        # Get each variant's hash value, filtering out Nones
+        hashes = list({variant.get_hash() for _, variant in sorted_variants} - {None})
+
+        # Write to event before returning
+        self.data["hashes"] = hashes
+
+        return (hashes, variants)
+
     def get_hashes(self, force_config: StrategyConfiguration | None = None) -> list[str]:
         """
         Returns the calculated hashes for the event. This uses the stored
@@ -353,20 +376,7 @@ def get_hashes(self, force_config: StrategyConfiguration | None = None) -> list[
                 return hashes
 
         # Create fresh hashes
-
-        variants = self.get_grouping_variants(force_config)
-        # Sort the variants so that the system variant (if any) is always last, in order to resolve
-        # ambiguities when choosing primary_hash for Snuba
-        sorted_variants = sorted(
-            variants.items(),
-            key=lambda name_and_variant: 1 if name_and_variant[0] == "system" else 0,
-        )
-        # Get each variant's hash value, filtering out Nones
-        hashes = list({variant.get_hash() for _, variant in sorted_variants} - {None})
-
-        # Write to event before returning
-        self.data["hashes"] = hashes
-        return hashes
+        return self.get_hashes_and_variants(force_config)[0]
 
     def normalize_stacktraces_for_grouping(self, grouping_config: StrategyConfiguration) -> None:
         """Normalize stacktraces and clear memoized interfaces

src/sentry/grouping/api.py

@@ -42,13 +42,14 @@
 @dataclass
 class GroupHashInfo:
     config: GroupingConfig
+    variants: dict[str, BaseVariant]
     hashes: list[str]
     grouphashes: list[GroupHash]
     existing_grouphash: GroupHash | None
 
 
 NULL_GROUPING_CONFIG: GroupingConfig = {"id": "", "enhancements": ""}
-NULL_GROUPHASH_INFO = GroupHashInfo(NULL_GROUPING_CONFIG, [], [], None)
+NULL_GROUPHASH_INFO = GroupHashInfo(NULL_GROUPING_CONFIG, {}, [], [], None)
 
 
 class GroupingConfigNotFound(LookupError):

src/sentry/grouping/ingest/hashing.py

@@ -21,6 +21,7 @@
     load_grouping_config,
 )
 from sentry.grouping.ingest.config import is_in_transition
+from sentry.grouping.variants import BaseVariant
 from sentry.models.grouphash import GroupHash
 from sentry.models.grouphashmetadata import GroupHashMetadata
 from sentry.models.project import Project
@@ -37,10 +38,10 @@
 
 def _calculate_event_grouping(
     project: Project, event: Event, grouping_config: GroupingConfig
-) -> list[str]:
+) -> tuple[list[str], dict[str, BaseVariant]]:
     """
-    Calculate hashes for the event using the given grouping config, and add them into the event
-    data.
+    Calculate hashes for the event using the given grouping config, add them to the event data, and
+    return them, along with the variants data upon which they're based.
     """
     metric_tags: MutableTags = {
         "grouping_config": grouping_config["id"],
@@ -59,7 +60,7 @@ def _calculate_event_grouping(
             # The active grouping config was put into the event in the
             # normalize step before.  We now also make sure that the
             # fingerprint was set to `'{{ default }}' just in case someone
-            # removed it from the payload.  The call to get_hashes will then
+            # removed it from the payload.  The call to `get_hashes_and_variants` will then
             # look at `grouping_config` to pick the right parameters.
             event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"]
             apply_server_fingerprinting(
@@ -69,9 +70,9 @@ def _calculate_event_grouping(
             )
 
         with metrics.timer("event_manager.event.get_hashes", tags=metric_tags):
-            hashes = event.get_hashes(loaded_grouping_config)
+            hashes, variants = event.get_hashes_and_variants(loaded_grouping_config)
 
-        return hashes
+        return (hashes, variants)
 
 
 def maybe_run_background_grouping(project: Project, job: Job) -> None:
@@ -100,12 +101,12 @@ def _calculate_background_grouping(
         "sdk": normalized_sdk_tag_from_event(event.data),
     }
     with metrics.timer("event_manager.background_grouping", tags=metric_tags):
-        return _calculate_event_grouping(project, event, config)
+        return _calculate_event_grouping(project, event, config)[0]
 
 
 def maybe_run_secondary_grouping(
     project: Project, job: Job, metric_tags: MutableTags
-) -> tuple[GroupingConfig, list[str]]:
+) -> tuple[GroupingConfig, list[str], dict[str, BaseVariant]]:
     """
     If the projct is in a grouping config transition phase, calculate a set of secondary hashes for
     the job's event.
@@ -119,18 +120,20 @@ def maybe_run_secondary_grouping(
             secondary_grouping_config = SecondaryGroupingConfigLoader().get_config_dict(project)
             secondary_hashes = _calculate_secondary_hashes(project, job, secondary_grouping_config)
 
-    return (secondary_grouping_config, secondary_hashes)
+    # Return an empty variants dictionary because we need the signature of this function to match
+    # that of `run_primary_grouping` (so we have to return something), but we don't ever actually
+    # need the variant information
+    return (secondary_grouping_config, secondary_hashes, {})
 
 
 def _calculate_secondary_hashes(
     project: Project, job: Job, secondary_grouping_config: GroupingConfig
 ) -> list[str]:
-    """Calculate secondary hash for event using a fallback grouping config for a period of time.
-    This happens when we upgrade all projects that have not opted-out to automatic upgrades plus
-    when the customer changes the grouping config.
-    This causes extra load in save_event processing.
     """
-    secondary_hashes = []
+    Calculate hashes based on an older grouping config, so that unknown hashes calculated by the
+    current config can be matched to an existing group if there is one.
+    """
+    secondary_hashes: list[str] = []
     try:
         with sentry_sdk.start_span(
             op="event_manager",
@@ -139,7 +142,7 @@ def _calculate_secondary_hashes(
             # create a copy since `_calculate_event_grouping` modifies the event to add all sorts
             # of grouping info and we don't want the secondary grouping data in there
             event_copy = copy.deepcopy(job["event"])
-            secondary_hashes = _calculate_event_grouping(
+            secondary_hashes, _ = _calculate_event_grouping(
                 project, event_copy, secondary_grouping_config
             )
     except Exception as err:
@@ -150,9 +153,9 @@ def _calculate_secondary_hashes(
 
 def run_primary_grouping(
     project: Project, job: Job, metric_tags: MutableTags
-) -> tuple[GroupingConfig, list[str]]:
+) -> tuple[GroupingConfig, list[str], dict[str, BaseVariant]]:
     """
-    Get the primary grouping config and primary hashes for the event.
+    Get the primary grouping config, primary hashes, and variants for the event.
     """
     with metrics.timer("event_manager.load_grouping_config"):
         grouping_config = get_grouping_config_dict_for_project(project)
@@ -165,16 +168,16 @@ def run_primary_grouping(
         ),
         metrics.timer("event_manager.calculate_event_grouping", tags=metric_tags),
     ):
-        hashes = _calculate_primary_hashes(project, job, grouping_config)
+        hashes, variants = _calculate_primary_hashes_and_variants(project, job, grouping_config)
 
-    return (grouping_config, hashes)
+    return (grouping_config, hashes, variants)
 
 
-def _calculate_primary_hashes(
+def _calculate_primary_hashes_and_variants(
     project: Project, job: Job, grouping_config: GroupingConfig
-) -> list[str]:
+) -> tuple[list[str], dict[str, BaseVariant]]:
     """
-    Get the primary hash for the event.
+    Get the primary hash and variants for the event.
 
     This is pulled out into a separate function mostly in order to make testing easier.
     """

src/sentry/grouping/ingest/seer.py

@@ -10,6 +10,7 @@
 from sentry.conf.server import SEER_SIMILARITY_MODEL_VERSION
 from sentry.eventstore.models import Event
 from sentry.grouping.grouping_info import get_grouping_info_from_variants
+from sentry.grouping.variants import BaseVariant
 from sentry.models.grouphash import GroupHash
 from sentry.models.project import Project
 from sentry.seer.similarity.similar_issues import get_similarity_data_from_seer
@@ -27,7 +28,7 @@
 logger = logging.getLogger("sentry.events.grouping")
 
 
-def should_call_seer_for_grouping(event: Event) -> bool:
+def should_call_seer_for_grouping(event: Event, variants: dict[str, BaseVariant]) -> bool:
     """
     Use event content, feature flags, rate limits, killswitches, seer health, etc. to determine
     whether a call to Seer should be made.
@@ -42,7 +43,7 @@ def should_call_seer_for_grouping(event: Event) -> bool:
         return False
 
     if (
-        _has_customized_fingerprint(event)
+        _has_customized_fingerprint(event, variants)
         or killswitch_enabled(project.id, event)
         or _circuit_breaker_broken(event, project)
         # **Do not add any new checks after this.** The rate limit check MUST remain the last of all
@@ -79,7 +80,7 @@ def _project_has_similarity_grouping_enabled(project: Project) -> bool:
 # combined with some other value). To the extent to which we're then using this function to decide
 # whether or not to call Seer, this means that the calculations giving rise to the default part of
 # the value never involve Seer input. In the long run, we probably want to change that.
-def _has_customized_fingerprint(event: Event) -> bool:
+def _has_customized_fingerprint(event: Event, variants: dict[str, BaseVariant]) -> bool:
     fingerprint = event.data.get("fingerprint", [])
 
     if "{{ default }}" in fingerprint:
@@ -97,7 +98,6 @@ def _has_customized_fingerprint(event: Event) -> bool:
             return True
 
     # Fully customized fingerprint (from either us or the user)
-    variants = event.get_grouping_variants()
     fingerprint_variant = variants.get("custom-fingerprint") or variants.get("built-in-fingerprint")
 
     if fingerprint_variant:
@@ -178,6 +178,7 @@ def _circuit_breaker_broken(event: Event, project: Project) -> bool:
 
 def get_seer_similar_issues(
     event: Event,
+    variants: dict[str, BaseVariant],
     num_neighbors: int = 1,
 ) -> tuple[dict[str, Any], GroupHash | None]:
     """
@@ -186,9 +187,7 @@ def get_seer_similar_issues(
     should go in (if any), or None if no neighbor was near enough.
     """
     event_hash = event.get_primary_hash()
-    stacktrace_string = get_stacktrace_string(
-        get_grouping_info_from_variants(event.get_grouping_variants())
-    )
+    stacktrace_string = get_stacktrace_string(get_grouping_info_from_variants(variants))
     exception_type = get_path(event.data, "exception", "values", -1, "type")
 
     request_data: SimilarIssuesEmbeddingsRequest = {
@@ -232,11 +231,11 @@ def get_seer_similar_issues(
 
 
 def maybe_check_seer_for_matching_grouphash(
-    event: Event, all_grouphashes: list[GroupHash]
+    event: Event, variants: dict[str, BaseVariant], all_grouphashes: list[GroupHash]
 ) -> GroupHash | None:
     seer_matched_grouphash = None
 
-    if should_call_seer_for_grouping(event):
+    if should_call_seer_for_grouping(event, variants):
         metrics.incr(
             "grouping.similarity.did_call_seer",
             sample_rate=options.get("seer.similarity.metrics_sample_rate"),
@@ -246,7 +245,7 @@ def maybe_check_seer_for_matching_grouphash(
         try:
             # If no matching group is found in Seer, we'll still get back result
             # metadata, but `seer_matched_grouphash` will be None
-            seer_response_data, seer_matched_grouphash = get_seer_similar_issues(event)
+            seer_response_data, seer_matched_grouphash = get_seer_similar_issues(event, variants)
         except Exception as e:  # Insurance - in theory we shouldn't ever land here
             sentry_sdk.capture_exception(
                 e, tags={"event": event.event_id, "project": event.project.id}

tests/sentry/event_manager/grouping/test_assign_to_group.py

@@ -10,7 +10,7 @@
 from sentry.event_manager import _create_group
 from sentry.eventstore.models import Event
 from sentry.grouping.ingest.hashing import (
-    _calculate_primary_hashes,
+    _calculate_primary_hashes_and_variants,
     _calculate_secondary_hashes,
     find_grouphash_with_group,
 )
@@ -29,7 +29,9 @@
 @contextmanager
 def patch_grouping_helpers(return_values: dict[str, Any]):
     wrapped_find_grouphash_with_group = capture_results(find_grouphash_with_group, return_values)
-    wrapped_calculate_primary_hashes = capture_results(_calculate_primary_hashes, return_values)
+    wrapped_calculate_primary_hashes = capture_results(
+        _calculate_primary_hashes_and_variants, return_values
+    )
     wrapped_calculate_secondary_hashes = capture_results(_calculate_secondary_hashes, return_values)
 
     with (
@@ -38,7 +40,7 @@ def patch_grouping_helpers(return_values: dict[str, Any]):
             wraps=wrapped_find_grouphash_with_group,
         ) as find_grouphash_with_group_spy,
         mock.patch(
-            "sentry.grouping.ingest.hashing._calculate_primary_hashes",
+            "sentry.grouping.ingest.hashing._calculate_primary_hashes_and_variants",
             wraps=wrapped_calculate_primary_hashes,
         ) as calculate_primary_hashes_spy,
         mock.patch(
@@ -59,7 +61,7 @@ def patch_grouping_helpers(return_values: dict[str, Any]):
     ):
         yield {
             "find_grouphash_with_group": find_grouphash_with_group_spy,
-            "_calculate_primary_hashes": calculate_primary_hashes_spy,
+            "_calculate_primary_hashes_and_variants": calculate_primary_hashes_spy,
             "_calculate_secondary_hashes": calculate_secondary_hashes_spy,
             "_create_group": create_group_spy,
             "record_calculation_metrics": record_calculation_metrics_spy,
@@ -145,7 +147,7 @@ def get_results_from_saving_event(
     with patch_grouping_helpers(return_values) as spies:
         calculate_secondary_hash_spy = spies["_calculate_secondary_hashes"]
         create_group_spy = spies["_create_group"]
-        calculate_primary_hash_spy = spies["_calculate_primary_hashes"]
+        calculate_primary_hash_spy = spies["_calculate_primary_hashes_and_variants"]
         record_calculation_metrics_spy = spies["record_calculation_metrics"]
 
         set_grouping_configs(
@@ -173,7 +175,7 @@ def get_results_from_saving_event(
         primary_hash_calculated = calculate_primary_hash_spy.call_count == 1
         secondary_hash_calculated = calculate_secondary_hash_spy.call_count == 1
 
-        primary_hash = return_values["_calculate_primary_hashes"][0][0]
+        primary_hash = return_values["_calculate_primary_hashes_and_variants"][0][0][0]
         primary_hash_found = bool(hash_search_result) and hash_search_result.hash == primary_hash
 
         new_group_created = create_group_spy.call_count == 1

tests/sentry/event_manager/grouping/test_group_creation_lock.py

@@ -67,7 +67,7 @@ def test_group_creation_race(monkeypatch, default_project, lock_disabled):
     with (
         patch(
             "sentry.grouping.ingest.hashing._calculate_event_grouping",
-            return_value=["pound sign", "octothorpe"],
+            return_value=(["pound sign", "octothorpe"], {}),
         ),
         patch(
             "sentry.event_manager._get_group_processing_kwargs",