chore(grouping): Various small pre-seer-call fixes (#68560)

This is a bunch of small, mostly cosmetic fixes pulled out of upcoming seer-grouping-related PRs to make them easier to review. Included changes (none behavior-changing):

- Add a docstring to `get_relevant_metrics_calls` test helper.
- Remove empty `if TYPE_CHECKING` block accidentally leftover from splitting up the `grouping.ingest` module.
- Add parentheses to clarify a ternary in `assign_to_group` tests.
- Add a comment about removing `extract_hashes` once the hierarchical grouping code is gone and one about the locking outcome metric.
- Copy the explanation of the locking mechanism from `_save_aggregate_new` into `_save_aggregate`.
- Rename `create_and_seek_grouphashes` to `get_hashes_and_grouphashes`.
- Make the linter chill out by:
  - using `startswith` rather than `[:1]` in `get_grouping_component_variants`,
  - removing the unused `projects` parameter in both `_get_or_create_group_environment_many` and `_get_or_create_group_release_many`, and
  - renaming variables in `get_default_grouping_config_dict` and `get_grouping_component_variants` to shop shadowing built-ins.

Author: lobsterkatie

src/sentry/event_manager.py

@@ -570,10 +570,10 @@ def save_error_events(
         job["event"].data.bind_ref(job["event"])
 
         _get_or_create_environment_many(jobs, projects)
-        _get_or_create_group_environment_many(jobs, projects)
+        _get_or_create_group_environment_many(jobs)
         _get_or_create_release_associated_models(jobs, projects)
         _increment_release_associated_counts_many(jobs, projects)
-        _get_or_create_group_release_many(jobs, projects)
+        _get_or_create_group_release_many(jobs)
         _tsdb_record_all_metrics(jobs)
 
         UserReport.objects.filter(project_id=project.id, event_id=job["event"].event_id).update(
@@ -946,7 +946,7 @@ def _get_or_create_environment_many(jobs: Sequence[Job], projects: ProjectsMappi
 
 
 @sentry_sdk.tracing.trace
-def _get_or_create_group_environment_many(jobs: Sequence[Job], projects: ProjectsMapping) -> None:
+def _get_or_create_group_environment_many(jobs: Sequence[Job]) -> None:
     for job in jobs:
         _get_or_create_group_environment(job["environment"], job["release"], job["groups"])
 
@@ -1029,7 +1029,7 @@ def _increment_release_associated_counts(
         )
 
 
-def _get_or_create_group_release_many(jobs: Sequence[Job], projects: ProjectsMapping) -> None:
+def _get_or_create_group_release_many(jobs: Sequence[Job]) -> None:
     for job in jobs:
         _get_or_create_group_release(
             job["environment"], job["release"], job["event"], job["groups"]
@@ -1443,19 +1443,35 @@ def _save_aggregate(
             metrics.timer("event_manager.create_group_transaction") as metric_tags,
             transaction.atomic(router.db_for_write(GroupHash)),
         ):
+            # These values will get overridden with whatever happens inside the lock if we do manage
+            # to acquire it, so it should only end up with `wait-for-lock` if we don't
             span.set_tag("outcome", "wait_for_lock")
             metric_tags["outcome"] = "wait_for_lock"
 
             all_grouphash_ids = [h.id for h in flat_grouphashes]
             if root_hierarchical_grouphash is not None:
                 all_grouphash_ids.append(root_hierarchical_grouphash.id)
 
+            # If we're in this branch, we checked our grouphashes and didn't find one with a group
+            # attached. We thus want to create a new group, but we need to guard against another
+            # event with the same hash coming in before we're done here and also thinking it needs
+            # to create a new group. To prevent this, we're using double-checked locking
+            # (https://en.wikipedia.org/wiki/Double-checked_locking).
+
+            # First, try to lock the relevant rows in the `GroupHash` table. If another (identically
+            # hashed) event is also in the process of creating a group and has grabbed the lock
+            # before us, we'll block here until it's done. If not, we've now got the lock and other
+            # identically-hashed events will have to wait for us.
             all_grouphashes = list(
                 GroupHash.objects.filter(id__in=all_grouphash_ids).select_for_update()
             )
 
             flat_grouphashes = [gh for gh in all_grouphashes if gh.hash in hashes.hashes]
 
+            # Now check again to see if any of our grouphashes have a group. In the first race
+            # condition scenario above, we'll have been blocked long enough for the other event to
+            # have created the group and updated our grouphashes with a group id, which means this
+            # time, we'll find something.
             existing_grouphash, root_hierarchical_hash = find_existing_grouphash(
                 project, flat_grouphashes, hashes.hierarchical_hashes
             )
@@ -1467,6 +1483,8 @@ def _save_aggregate(
             else:
                 root_hierarchical_grouphash = None
 
+            # If we still haven't found a matching grouphash, we're now safe to go ahead and create
+            # the group.
             if existing_grouphash is None:
                 group = _create_group(project, event, **group_creation_kwargs)
 
@@ -1606,7 +1624,7 @@ def _save_aggregate_new(
     group_processing_kwargs = _get_group_processing_kwargs(job)
 
     # Try looking for an existing group using the current grouping config
-    primary = create_and_seek_grouphashes(job, run_primary_grouping, metric_tags)
+    primary = get_hashes_and_grouphashes(job, run_primary_grouping, metric_tags)
 
     # If we've found one, great. No need to do any more calculations
     if primary.existing_grouphash:
@@ -1616,7 +1634,7 @@ def _save_aggregate_new(
         result = "found_primary"
     # If we haven't, try again using the secondary config
     else:
-        secondary = create_and_seek_grouphashes(job, maybe_run_secondary_grouping, metric_tags)
+        secondary = get_hashes_and_grouphashes(job, maybe_run_secondary_grouping, metric_tags)
         all_grouphashes = primary.grouphashes + secondary.grouphashes
 
         # Now we know for sure whether or not a group already exists, so handle both cases
@@ -1659,7 +1677,7 @@ def _save_aggregate_new(
     return group_info
 
 
-def create_and_seek_grouphashes(
+def get_hashes_and_grouphashes(
     job: Job,
     hash_calculation_function: Callable[
         [Project, Job, MutableTags],
@@ -1768,6 +1786,8 @@ def create_group_with_grouphashes(
         metrics.timer("event_manager.create_group_transaction") as metrics_timer_tags,
         transaction.atomic(router.db_for_write(GroupHash)),
     ):
+        # These values will get overridden with whatever happens inside the lock if we do manage to
+        # acquire it, so it should only end up with `wait-for-lock` if we don't
         span.set_tag("outcome", "wait_for_lock")
         metrics_timer_tags["outcome"] = "wait_for_lock"
 

src/sentry/grouping/api.py

@@ -199,13 +199,13 @@ def get_projects_default_fingerprinting_bases(
     return bases
 
 
-def get_default_grouping_config_dict(id=None) -> GroupingConfig:
+def get_default_grouping_config_dict(config_id=None) -> GroupingConfig:
     """Returns the default grouping config."""
-    if id is None:
+    if config_id is None:
         from sentry.projectoptions.defaults import DEFAULT_GROUPING_CONFIG
 
-        id = DEFAULT_GROUPING_CONFIG
-    return {"id": id, "enhancements": get_default_enhancements(id)}
+        config_id = DEFAULT_GROUPING_CONFIG
+    return {"id": config_id, "enhancements": get_default_enhancements(config_id)}
 
 
 def load_grouping_config(config_dict=None) -> StrategyConfiguration:

src/sentry/grouping/ingest/config.py

@@ -3,7 +3,7 @@
 import logging
 import time
 from collections.abc import MutableMapping
-from typing import TYPE_CHECKING, Any
+from typing import Any
 
 from django.conf import settings
 from django.core.cache import cache
@@ -13,9 +13,6 @@
 from sentry.models.project import Project
 from sentry.projectoptions.defaults import BETA_GROUPING_CONFIG, DEFAULT_GROUPING_CONFIG
 
-if TYPE_CHECKING:
-    pass
-
 logger = logging.getLogger("sentry.events.grouping")
 
 Job = MutableMapping[str, Any]

src/sentry/grouping/ingest/utils.py

@@ -20,6 +20,7 @@
 Job = MutableMapping[str, Any]
 
 
+# TODO This can go away once hierarchical grouping is gone
 def extract_hashes(calculated_hashes: CalculatedHashes | None) -> list[str]:
     return [] if not calculated_hashes else list(calculated_hashes.hashes)
 

src/sentry/grouping/strategies/base.py

@@ -217,7 +217,7 @@ def get_grouping_component_variants(
         prevent_contribution = None
 
         for variant, component in variants.items():
-            is_mandatory = variant[:1] == "!"
+            is_mandatory = variant.startswith("!")
             variant = variant.lstrip("!")
 
             if is_mandatory:
@@ -252,8 +252,8 @@ def get_grouping_component_variants(
                     ),
                 )
             else:
-                hash = component.get_hash()
-                duplicate_of = mandatory_contributing_hashes.get(hash)
+                hash_value = component.get_hash()
+                duplicate_of = mandatory_contributing_hashes.get(hash_value)
                 if duplicate_of is not None:
                     component.update(
                         contributes=False,

tests/sentry/event_manager/grouping/test_assign_to_group.py

@@ -218,7 +218,7 @@ def get_results_from_saving_event(
 
         if existing_group_id:
             event_assigned_to_given_existing_group = (
-                new_event.group_id == existing_group_id if existing_group_id else None
+                (new_event.group_id == existing_group_id) if existing_group_id else None
             )
 
         if secondary_hash_calculated:

tests/sentry/event_manager/test_event_manager_grouping.py

@@ -19,6 +19,9 @@
 
 
 def get_relevant_metrics_calls(mock_fn: MagicMock, key: str) -> list[mock._Call]:
+    """
+    Given a mock metrics function, grab only the calls which record the metric with the given key.
+    """
     return [call for call in mock_fn.call_args_list if call.args[0] == key]