feat(grouping): Small grouping code refactor (#23857)

Author: mitsuhiko

src/sentry/grouping/component.py

@@ -114,6 +114,13 @@ def update(self, hint=None, contributes=None, contributes_to_similarity=None, va
         if contributes_to_similarity is not None:
             self.contributes_to_similarity = contributes_to_similarity
 
+    def shallow_copy(self):
+        """Creates a shallow copy."""
+        rv = object.__new__(self.__class__)
+        rv.__dict__.update(self.__dict__)
+        rv.values = list(self.values)
+        return rv
+
     def iter_values(self):
         """Recursively walks the component and flattens it into a list of
         values.

src/sentry/grouping/strategies/base.py

@@ -15,8 +15,8 @@
 
 def strategy(id=None, ids=None, variants=None, interfaces=None, name=None, score=None):
     """Registers a strategy"""
-    if interfaces is None or variants is None:
-        raise TypeError("interfaces and variants are required")
+    if interfaces is None:
+        raise TypeError("interfaces is required")
 
     if name is None:
         if len(interfaces) != 1:
@@ -87,6 +87,15 @@ def lookup_strategy(strategy_id):
         raise LookupError("Unknown strategy %r" % strategy_id)
 
 
+def flatten_variants_from_component(component):
+    """Given a component extracts variants from it if that component is
+    a variant provider.  Otherwise this just returns the root component.
+    """
+    if not component.variant_provider:
+        return {component.id: component}
+    return {c.id: c for c in component.values}
+
+
 class Strategy:
     """Baseclass for all strategies."""
 
@@ -97,13 +106,16 @@ def __init__(self, id, name, interfaces, variants, score, func):
         self.interfaces = interfaces
         self.mandatory_variants = []
         self.optional_variants = []
-        self.variants = []
-        for variant in variants:
-            if variant[:1] == "!":
-                self.mandatory_variants.append(variant[1:])
-            else:
-                self.optional_variants.append(variant)
-            self.variants.append(variant)
+        if variants is not None:
+            self.variants = []
+            for variant in variants:
+                if variant[:1] == "!":
+                    self.mandatory_variants.append(variant[1:])
+                else:
+                    self.optional_variants.append(variant)
+                self.variants.append(variant)
+        else:
+            self.variants = None
         self.score = score
         self.func = func
         self.variant_processor_func = None
@@ -128,7 +140,7 @@ def variant_processor(self, func):
         self.variant_processor_func = func
         return func
 
-    def get_grouping_component(self, event, variant, context):
+    def get_grouping_component(self, event, context, variant=None):
         """Given a specific variant this calculates the grouping component."""
         args = []
         for iface_path in self.interfaces:
@@ -146,12 +158,22 @@ def get_grouping_component_variants(self, event, context):
         """This returns a dictionary of all components by variant that this
         strategy can produce.
         """
+        # If no variants are provided we skip over the default logic and
+        # go exclusively by the variant_processor_func. This also makes this
+        # code a noop for delegate only strategies.
+        if not self.variants:
+            component = self.get_grouping_component(event, context)
+            rv = flatten_variants_from_component(component)
+            if self.variant_processor_func is not None:
+                rv = self._invoke(self.variant_processor_func, rv, event=event, context=context)
+            return rv
+
         rv = {}
         # trivial case: we do not have mandatory variants and can handle
         # them all the same.
         if not self.mandatory_variants:
             for variant in self.variants:
-                component = self.get_grouping_component(event, variant, context)
+                component = self.get_grouping_component(event, context, variant)
                 if component is not None:
                     rv[variant] = component
 
@@ -160,7 +182,7 @@ def get_grouping_component_variants(self, event, context):
             prevent_contribution = None
 
             for variant in self.mandatory_variants:
-                component = self.get_grouping_component(event, variant, context)
+                component = self.get_grouping_component(event, context, variant)
                 if component is None:
                     continue
                 if component.contributes:
@@ -172,7 +194,7 @@ def get_grouping_component_variants(self, event, context):
             for variant in self.optional_variants:
                 # We also only want to create another variant if it
                 # produces different results than the mandatory components
-                component = self.get_grouping_component(event, variant, context)
+                component = self.get_grouping_component(event, context, variant)
                 if component is None:
                     continue
 
@@ -273,7 +295,6 @@ class NewStrategyConfiguration(StrategyConfiguration):
 
     NewStrategyConfiguration.id = id
     NewStrategyConfiguration.base = base
-    NewStrategyConfiguration.config_class = id.split(":", 1)[0]
     NewStrategyConfiguration.strategies = dict(base.strategies) if base else {}
     NewStrategyConfiguration.delegates = dict(base.delegates) if base else {}
     NewStrategyConfiguration.initial_context = dict(base.initial_context) if base else {}

src/sentry/grouping/strategies/configurations.py

@@ -5,19 +5,63 @@
 )
 
 
-# The classes of grouping algorithms
-CLASSES = []
-
 # The full mapping of all known configurations.
 CONFIGURATIONS = {}
 
+# The implied base strategy *every* strategy inherits from if no
+# base is defined.
+BASE_STRATEGY = create_strategy_configuration(
+    None,
+    strategies=[
+        "expect-ct:v1",
+        "expect-staple:v1",
+        "hpkp:v1",
+        "csp:v1",
+        "threads:v1",
+        "stacktrace:v1",
+        "chained-exception:v1",
+        "template:v1",
+        "message:v1",
+    ],
+    delegates=["frame:v1", "stacktrace:v1", "single-exception:v1"],
+    initial_context={
+        # This key in the context tells the system which variant should
+        # be produced.  TODO: phase this out.
+        "variant": None,
+        # This is a flag that can be used by any delegate to respond to
+        # a detected recursion.  This is currently used by the frame
+        # strategy to disable itself.  Recursion is detected by the outer
+        # strategy.
+        "is_recursion": False,
+        # This turns on the automatic message trimming by the message
+        # strategy.
+        "trim_message": False,
+        # newstyle: enables the legacy function logic.  This is only used
+        # by the newstyle:2019-04-05 strategy.  Once this is no longer used
+        # this can go away entirely.
+        "legacy_function_logic": False,
+        # newstyle: turns on some javascript fuzzing features.
+        "javascript_fuzzing": False,
+        # newstyle: platforms for which context line should be taken into
+        # account when grouping.
+        "contextline_platforms": (),
+        # newstyle: this detects anonymous classes in PHP code.
+        "php_detect_anonymous_classes": False,
+        # newstyle: turns on a bug that was present in some variants
+        "with_context_line_file_origin_bug": False,
+        # newstyle: turns on falling back to exception values when there
+        # is no stacktrace.
+        "with_exception_value_fallback": False,
+    },
+)
+
 
 def register_strategy_config(id, **kwargs):
     if kwargs.get("base") is not None:
         kwargs["base"] = CONFIGURATIONS[kwargs["base"]]
+    else:
+        kwargs["base"] = BASE_STRATEGY
     rv = create_strategy_configuration(id, **kwargs)
-    if rv.config_class not in CLASSES:
-        CLASSES.append(rv.config_class)
     CONFIGURATIONS[rv.id] = rv
     return rv
 
@@ -30,15 +74,9 @@ def register_strategy_config(id, **kwargs):
 register_strategy_config(
     id="legacy:2019-03-12",
     strategies=[
-        "expect-ct:v1",
-        "expect-staple:v1",
-        "hpkp:v1",
-        "csp:v1",
         "threads:legacy",
         "stacktrace:legacy",
         "chained-exception:legacy",
-        "template:v1",
-        "message:v1",
     ],
     delegates=["frame:legacy", "stacktrace:legacy", "single-exception:legacy"],
     changelog="""
@@ -58,19 +96,7 @@ def register_strategy_config(id, **kwargs):
 
 register_strategy_config(
     id="newstyle:2019-05-08",
-    strategies=[
-        "expect-ct:v1",
-        "expect-staple:v1",
-        "hpkp:v1",
-        "csp:v1",
-        "threads:v1",
-        "stacktrace:v1",
-        "chained-exception:v1",
-        "template:v1",
-        "message:v1",
-    ],
     risk=RISK_LEVEL_HIGH,
-    delegates=["frame:v1", "stacktrace:v1", "single-exception:v1"],
     changelog="""
         * Uses source code information all platforms with reliable sources
           for grouping (JavaScript, Python, PHP and Ruby) and function
@@ -84,10 +110,8 @@ def register_strategy_config(id, **kwargs):
         * C/C++ and other native stacktraces are more reliably grouped.
     """,
     initial_context={
-        "legacy_function_logic": False,
         "javascript_fuzzing": True,
         "contextline_platforms": ("javascript", "node", "python", "php", "ruby"),
-        "php_detect_anonymous_classes": False,
         "with_context_line_file_origin_bug": True,
         "trim_message": True,
         "with_exception_value_fallback": True,
@@ -97,7 +121,6 @@ def register_strategy_config(id, **kwargs):
 register_strategy_config(
     id="newstyle:2019-10-29",
     base="newstyle:2019-05-08",
-    delegates=["frame:v1"],
     risk=RISK_LEVEL_MEDIUM,
     changelog="""
         * Better rules for when to take context lines into account for
@@ -118,39 +141,19 @@ def register_strategy_config(id, **kwargs):
 
 register_strategy_config(
     id="newstyle:2019-04-05",
-    strategies=[
-        "expect-ct:v1",
-        "expect-staple:v1",
-        "hpkp:v1",
-        "csp:v1",
-        "threads:v1",
-        "stacktrace:v1",
-        "chained-exception:v1",
-        "template:v1",
-        "message:v1",
-    ],
-    delegates=["frame:v1", "stacktrace:v1", "single-exception:v1"],
     risk=RISK_LEVEL_HIGH,
     changelog="""
         * Experimental grouping algorithm (should not be used)
     """,
     hidden=True,
     initial_context={
         "legacy_function_logic": True,
-        "javascript_fuzzing": False,
-        "contextline_platforms": (),
-        "php_detect_anonymous_classes": False,
-        "with_context_line_file_origin_bug": False,
-        "trim_message": False,
-        "with_exception_value_fallback": False,
     },
 )
 
 register_strategy_config(
     id="newstyle:2019-04-17",
     base="newstyle:2019-04-05",
-    strategies=["message:v1"],
-    delegates=["frame:v1", "single-exception:v1"],
     risk=RISK_LEVEL_HIGH,
     changelog="""
         * Experimental grouping algorithm (should not be used)

src/sentry/grouping/strategies/legacy.py

@@ -150,7 +150,7 @@ def remove_function_outliers_legacy(function):
     return new_function, None
 
 
-@strategy(id="single-exception:legacy", interfaces=["singleexception"], variants=["!system", "app"])
+@strategy(id="single-exception:legacy", interfaces=["singleexception"])
 def single_exception_legacy(exception, context, **meta):
     type_component = GroupingComponent(
         id="type",
@@ -223,7 +223,7 @@ def chained_exception_legacy_variant_processor(variants, context, **meta):
     return remove_non_stacktrace_variants(variants)
 
 
-@strategy(id="frame:legacy", interfaces=["frame"], variants=["!system", "app"])
+@strategy(id="frame:legacy", interfaces=["frame"])
 def frame_legacy(frame, event, **meta):
     platform = frame.platform or event.platform
 

src/sentry/grouping/strategies/newstyle.py

@@ -53,6 +53,9 @@
 
 def is_recursion_v1(frame1, frame2):
     "Returns a boolean indicating whether frames are recursive calls."
+    if frame2 is None:
+        return False
+
     for field in RECURSION_COMPARISON_FIELDS:
         if getattr(frame1, field, None) != getattr(frame2, field, None):
             return False
@@ -230,13 +233,10 @@ def get_function_component(
 @strategy(
     ids=["frame:v1"],
     interfaces=["frame"],
-    variants=["!system", "app"],
 )
 def frame(frame, event, context, **meta):
     platform = frame.platform or event.platform
 
-    platform = frame.platform or event.platform
-
     # Safari throws [native code] frames in for calls like ``forEach``
     # whereas Chrome ignores these. Let's remove it from the hashing algo
     # so that they're more likely to group together
@@ -308,6 +308,9 @@ def frame(frame, event, context, **meta):
         ):
             rv.update(contributes=False, hint="ignored low quality javascript frame")
 
+    if context["is_recursion"]:
+        rv.update(contributes=False, hint="ignored due to recursion")
+
     return rv
 
 
@@ -343,19 +346,16 @@ def get_contextline_component(frame, platform, function, context):
 def stacktrace(stacktrace, context, **meta):
     variant = context["variant"]
     frames = stacktrace.frames
-    all_frames_considered_in_app = False
 
     values = []
     prev_frame = None
     frames_for_filtering = []
     for frame in frames:
-        frame_component = context.get_grouping_component(frame, **meta)
-        if variant == "app" and not frame.in_app and not all_frames_considered_in_app:
+        with context:
+            context["is_recursion"] = is_recursion_v1(frame, prev_frame)
+            frame_component = context.get_grouping_component(frame, **meta)
+        if variant == "app" and not frame.in_app:
             frame_component.update(contributes=False, hint="non app frame")
-        elif prev_frame is not None and is_recursion_v1(frame, prev_frame):
-            frame_component.update(contributes=False, hint="ignored due to recursion")
-        elif variant == "app" and not frame.in_app and all_frames_considered_in_app:
-            frame_component.update(hint="frame considered in-app because no frame is in-app")
         values.append(frame_component)
         frames_for_filtering.append(frame.get_raw_data())
         prev_frame = frame
@@ -419,7 +419,6 @@ def _stacktrace_encoder(id, stacktrace):
 @strategy(
     ids=["single-exception:v1"],
     interfaces=["singleexception"],
-    variants=["!system", "app"],
 )
 def single_exception(exception, context, **meta):
     if exception.stacktrace is not None: