ref(grouping): Small parameterization refactor (#109690)

This is a small refactor to our message parameterization code, to make it a little easier to understand. Aside from a few renames and some added/tweaked comments, the main changes here are:

- Make it so that `ParameterizationRegex.experimental_pattern` is `None` if there's no experimental pattern, rather than having it fall back to using the regular pattern. The fallback is now done in the creation of `EXPERIMENTAL_PARAMETERIZATION_REGEXES_MAP`.

- Make the behavior of `Parameterizer.parametrize_w_regex` match its docstring by a) making it take in a regex (as its docstring says it does), and b) removing the (non-existent) callback parameter from the docstring.

- Add a few test cases.

- Skip the test testing experimental patterns if there are none.

Author: lobsterkatie

src/sentry/grouping/parameterization.py

@@ -3,12 +3,6 @@
 from collections import defaultdict
 from collections.abc import Callable, Iterable, Sequence
 
-__all__ = [
-    "ParameterizationCallable",
-    "ParameterizationRegex",
-    "Parameterizer",
-]
-
 
 @dataclasses.dataclass
 class ParameterizationRegex:
@@ -24,20 +18,21 @@ class ParameterizationRegex:
 
     @property
     def pattern(self) -> str:
-        return self._pattern(False)
+        return self._get_pattern(self.raw_pattern)
 
     @property
-    def experimental_pattern(self) -> str:
-        return self._pattern(self.raw_pattern_experimental is not None)
+    def experimental_pattern(self) -> str | None:
+        if not self.raw_pattern_experimental:
+            return None
+        return self._get_pattern(self.raw_pattern_experimental)
 
-    def _pattern(self, experimental: bool = False) -> str:
+    def _get_pattern(self, raw_pattern: str) -> str:
         """
         Returns the regex pattern with a named matching group and lookbehind/lookahead if needed.
         """
-        pattern = self.raw_pattern_experimental if experimental else self.raw_pattern
         prefix = rf"(?<={self.lookbehind})" if self.lookbehind else ""
         postfix = rf"(?={self.lookahead})" if self.lookahead else ""
-        return rf"{prefix}(?P<{self.name}>{pattern}){postfix}"
+        return rf"{prefix}(?P<{self.name}>{raw_pattern}){postfix}"
 
 
 DEFAULT_PARAMETERIZATION_REGEXES = [
@@ -198,37 +193,46 @@ def _pattern(self, experimental: bool = False) -> str:
     ParameterizationRegex(name="int", raw_pattern=r"""-\d+\b | \b\d+\b"""),
     ParameterizationRegex(
         name="quoted_str",
-        raw_pattern=r"""# Using `=`lookbehind which guarantees we'll only match the value half of key-value pairs,
-            # rather than all quoted strings
+        raw_pattern=r"""
             '([^']+)' | "([^"]+)"
         """,
+        # Using an `=` lookbehind guarantees we'll only match the value half of key-value pairs,
+        # rather than all quoted strings
         lookbehind="=",
     ),
     ParameterizationRegex(
         name="bool",
-        raw_pattern=r"""# Using `=`lookbehind which guarantees we'll only match the value half of key-value pairs,
-            # rather than all instances of the words 'true' and 'false'.
+        raw_pattern=r"""
             True |
             true |
             False |
             false
         """,
+        # Using an `=` lookbehind guarantees we'll only match the value half of key-value pairs,
+        # rather than all instances of the words 'true' and 'false'.
         lookbehind="=",
     ),
 ]
 
 
+# Patterns to use for each match type when not in experimental mode.
 DEFAULT_PARAMETERIZATION_REGEXES_MAP = {r.name: r.pattern for r in DEFAULT_PARAMETERIZATION_REGEXES}
+
+# Patterns to use when in experimental mode. If no experimental pattern exists for a given type of
+# match, falls back to the default pattern.
 EXPERIMENTAL_PARAMETERIZATION_REGEXES_MAP = {
-    r.name: r.experimental_pattern for r in DEFAULT_PARAMETERIZATION_REGEXES
+    r.name: r.experimental_pattern if r.experimental_pattern else r.pattern
+    for r in DEFAULT_PARAMETERIZATION_REGEXES
 }
 
 
 @dataclasses.dataclass
 class ParameterizationCallable:
     """
-    Represents a callable that can be used to modify a string, which can give
-    us more flexibility than just using regex.
+    Represents a callable that can be used to modify a string, which can give us more flexibility
+    than just using regex.
+
+    Note: Future-proofing. Not currently in use.
     """
 
     name: str  # name of the pattern (also used as group name in combined regex)
@@ -239,7 +243,11 @@ class ParameterizationCallable:
 class Parameterizer:
     def __init__(
         self,
+        # List of `ParameterizationRegex.name` values, used to selectively enable pattern types. To
+        # use all available parameterization, omit this argument.
         regex_pattern_keys: Sequence[str] | None = None,
+        # Whether to use experimental patterns, if available. (Pattern types without an experimental
+        # pattern will fall back to the standard pattern.)
         experimental: bool = False,
     ):
         self._experimental = experimental
@@ -268,15 +276,14 @@ def _make_regex_from_patterns(self, pattern_keys: Iterable[str]) -> re.Pattern[s
 
         return re.compile(rf"(?x){'|'.join(regexes_map[k] for k in pattern_keys)}")
 
-    def parametrize_w_regex(self, content: str) -> str:
+    def parametrize_w_regex(self, input_str: str, parameterization_regex: re.Pattern[str]) -> str:
         """
-        Replace all matches of the given regex in the content with a placeholder string.
+        Replace all matches of the given regex in the input string with a placeholder.
 
-        @param content: The string to replace matches in.
+        @param input_str: The string to replace matches in.
         @param parameterization_regex: The compiled regex pattern to match.
-        @param match_callback: An optional callback function to call with the key of the matched pattern.
 
-        @returns: The content with all matches replaced with placeholders.
+        @returns: The input string with all matches replaced with placeholders.
         """
 
         def _handle_regex_match(match: re.Match[str]) -> str:
@@ -289,7 +296,7 @@ def _handle_regex_match(match: re.Match[str]) -> str:
                     return f"<{key}>"
             return ""
 
-        return self._parameterization_regex.sub(_handle_regex_match, content)
+        return parameterization_regex.sub(_handle_regex_match, input_str)
 
-    def parameterize_all(self, content: str) -> str:
-        return self.parametrize_w_regex(content)
+    def parameterize_all(self, input_str: str) -> str:
+        return self.parametrize_w_regex(input_str, self._parameterization_regex)

tests/sentry/grouping/test_parameterization.py

@@ -1,6 +1,10 @@
 import pytest
 
-from sentry.grouping.parameterization import Parameterizer
+from sentry.grouping.parameterization import (
+    DEFAULT_PARAMETERIZATION_REGEXES_MAP,
+    EXPERIMENTAL_PARAMETERIZATION_REGEXES_MAP,
+    Parameterizer,
+)
 
 
 @pytest.fixture
@@ -58,13 +62,16 @@ def experimental_parameterizer() -> Parameterizer:
     ("date - RFC3339Nano without offset", "2006-01-02T15:04:05.999999999Z", "<date>"),
     ("date - plain", "2006-01-02", "<date>"),
     ("date - long", "Jan 18, 2019", "<date>"),
-    ("date - datetime", "2006-01-02 15:04:05", "<date>"),
+    ("date - datetime space-separated", "2006-01-02 15:04:05", "<date>"),
+    ("date - datetime T-separated", "2006-01-02T15:04:05", "<date>"),
+    ("date - datetime T-separated UTC", "2006-01-02T15:04:05Z", "<date>"),
+    ("date - datetime T-separated w offset", "2006-01-02T15:04:05+01:00", "<date>"),
     ("date - kitchen", "3:04PM", "<date>"),
     ("date - time", "15:04:05", "<date>"),
     ("date - basic", "Mon Jan 02, 1999", "<date>"),
-    ("date - compressed", "20240220 11:55:33.546593", "<date>"),
+    ("date - datetime compressed date", "20240220 11:55:33.546593", "<date>"),
     ("date - datestamp", "2024-02-23 02:13:53.418", "<date>"),
-    ("date - datetime", "datetime.datetime(2025, 6, 24, 18, 33, 0, 447640)", "<date>"),
+    ("date - datetime object", "datetime.datetime(2025, 6, 24, 18, 33, 0, 447640)", "<date>"),
     ("duration - 0ms", "0ms", "<duration>"),
     ("duration - 1ms", "1ms", "<duration>"),
     ("duration - 10ms", "10ms", "<duration>"),
@@ -112,6 +119,10 @@ def experimental_parameterizer() -> Parameterizer:
     ("hex without prefix - uppercase, 128 digits", "B0" * 64, "<hex>"),
     ("hex without prefix - lowercase, no numbers", "deadbeef", "deadbeef"),
     ("hex without prefix - uppercase, no numbers", "DEADBEEF", "DEADBEEF"),
+    ("hex without prefix - no letters, < 8 digits", "1234567", "<int>"),
+    ("hex without prefix - no letters, 8+ digits", "12345678", "<hex>"),
+    ("git sha - all letters", "commit deadbeef", "commit deadbeef"),
+    ("git sha - all numbers", "commit 4150908", "commit <int>"),
     ("float", "0.23", "<float>"),
     ("int", "23", "<int>"),
     ("int - separator", "0:17502", "<int>:<int>"),
@@ -145,7 +156,7 @@ def experimental_parameterizer() -> Parameterizer:
 
 
 @pytest.mark.parametrize(("name", "input", "expected"), standard_cases)
-def test_parameterize_standard(
+def test_default_parameterization(
     name: str, input: str, expected: str, parameterizer: Parameterizer
 ) -> None:
     assert parameterizer.parameterize_all(input) == expected
@@ -155,7 +166,7 @@ def test_parameterize_standard(
 
 
 @pytest.mark.parametrize(("name", "input", "expected"), experimental_cases)
-def test_parameterize_standard_not_experimental(
+def test_default_parameterizer_misses_experimental_cases(
     name: str, input: str, expected: str, parameterizer: Parameterizer
 ) -> None:
     assert parameterizer.parameterize_all(input) != expected
@@ -164,8 +175,12 @@ def test_parameterize_standard_not_experimental(
     assert parameterizer.parameterize_all(f"prefix {input} suffix") != f"prefix {expected} suffix"
 
 
+@pytest.mark.skipif(
+    EXPERIMENTAL_PARAMETERIZATION_REGEXES_MAP == DEFAULT_PARAMETERIZATION_REGEXES_MAP,
+    reason="no experimental regexes to test",
+)
 @pytest.mark.parametrize(("name", "input", "expected"), standard_cases + experimental_cases)
-def test_parameterize_experimental(
+def test_experimental_parameterization(
     name: str, input: str, expected: str, experimental_parameterizer: Parameterizer
 ) -> None:
     assert experimental_parameterizer.parameterize_all(input) == expected