fix(grouping): Store original in_app for idempotent regrouping (#13540)

Fixes grouping enhancement display to always operate on the original value of the `in_app` flag. Eventually, this will allow to regroup events based on their original payload.

requirements-base.txt

@@ -56,7 +56,7 @@ redis>=2.10.3,<2.10.6
 requests-oauthlib==0.3.3
 requests[security]>=2.20.0,<2.21.0
 selenium==3.141.0
-semaphore>=0.4.36,<0.5.0
+semaphore>=0.4.37,<0.5.0
 sentry-sdk>=0.9.0
 setproctitle>=1.1.7,<1.2.0
 simplejson>=3.2.0,<3.9.0

src/sentry/api/endpoints/event_grouping_info.py

@@ -41,7 +41,8 @@ class EventGroupingInfoEndpoint(ProjectEndpoint):
         hashes = event.get_hashes()
 
         try:
-            variants = event.get_grouping_variants(config_name)
+            variants = event.get_grouping_variants(force_config=config_name,
+                                                   normalize_stacktraces=True)
         except GroupingConfigNotFound:
             raise ResourceDoesNotExist(detail='Unknown grouping config')
 

src/sentry/grouping/api.py

@@ -97,6 +97,10 @@ def load_grouping_config(config_dict=None):
     return CONFIGURATIONS[config_id](**config_dict)
 
 
+def load_default_grouping_config():
+    return load_grouping_config(config_dict=None)
+
+
 def get_fingerprinting_config_for_project(project):
     from sentry.grouping.fingerprinting import FingerprintingRules, \
         InvalidFingerprintingConfig
@@ -201,9 +205,11 @@ def get_grouping_variants_for_event(event, config=None):
             'custom-fingerprint': CustomFingerprintVariant(fingerprint),
         }
 
+    if config is None:
+        config = load_default_grouping_config()
+
     # At this point we need to calculate the default event values.  If the
     # fingerprint is salted we will wrap it.
-    config = load_grouping_config(config)
     components = _get_calculated_grouping_variants_for_event(event, config)
     rv = {}
 

src/sentry/grouping/enhancer.py

@@ -11,10 +11,12 @@ from parsimonious.grammar import Grammar, NodeVisitor
 from parsimonious.exceptions import ParseError
 
 from sentry import projectoptions
+from sentry.stacktraces.functions import set_in_app
 from sentry.stacktraces.platform import get_behavior_family_for_platform
 from sentry.grouping.utils import get_rule_bool
 from sentry.utils.compat import implements_to_string
 from sentry.utils.glob import glob_match
+from sentry.utils.safe import get_path
 
 
 # Grammar is defined in EBNF syntax.
@@ -166,7 +168,7 @@ class Action(object):
     def apply_modifications_to_frame(self, frames, idx):
         pass
 
-    def update_frame_components_contributions(self, components, idx, rule=None):
+    def update_frame_components_contributions(self, components, frames, idx, rule=None):
         pass
 
     def modify_stack_state(self, state, rule):
@@ -207,15 +209,25 @@ class FlagAction(Action):
             return seq[idx + 1:]
         return []
 
+    def _in_app_changed(self, frame, component):
+        orig_in_app = get_path(frame, 'data', 'orig_in_app')
+
+        if orig_in_app is not None:
+            if orig_in_app == -1:
+                orig_in_app = None
+            return orig_in_app != frame.get('in_app')
+        else:
+            return self.flag == component.contributes
+
     def apply_modifications_to_frame(self, frames, idx):
         # Grouping is not stored on the frame
         if self.key == 'group':
             return
         for frame in self._slice_to_range(frames, idx):
             if self.key == 'app':
-                frame['in_app'] = self.flag
+                set_in_app(frame, self.flag)
 
-    def update_frame_components_contributions(self, components, idx, rule=None):
+    def update_frame_components_contributions(self, components, frames, idx, rule=None):
         rule_hint = 'grouping enhancement rule'
         if rule:
             rule_hint = '%s (%s)' % (
@@ -223,7 +235,9 @@ class FlagAction(Action):
                 rule.matcher_description,
             )
 
-        for component in self._slice_to_range(components, idx):
+        sliced_components = self._slice_to_range(components, idx)
+        sliced_frames = self._slice_to_range(frames, idx)
+        for component, frame in izip(sliced_components, sliced_frames):
             if self.key == 'group' and self.flag != component.contributes:
                 component.update(
                     contributes=self.flag,
@@ -232,9 +246,7 @@ class FlagAction(Action):
                 )
             # The in app flag was set by `apply_modifications_to_frame`
             # but we want to add a hint if there is none yet.
-            elif self.key == 'app' and \
-                    self.flag == component.contributes and \
-                    component.hint is None:
+            elif self.key == 'app' and self._in_app_changed(frame, component):
                 component.update(
                     hint='marked %s by %s' % (
                         self.flag and 'in-app' or 'out of app', rule_hint)
@@ -311,7 +323,7 @@ class Enhancements(object):
                 actions = rule.get_matching_frame_actions(frame, platform)
                 for action in actions or ():
                     action.update_frame_components_contributions(
-                        components, idx, rule=rule)
+                        components, frames, idx, rule=rule)
                     action.modify_stack_state(stack_state, rule)
 
         # Use the stack state to update frame contributions again
@@ -406,8 +418,8 @@ class Rule(object):
     @property
     def matcher_description(self):
         rv = ' '.join(x.description for x in self.matchers)
-        if any(x.range is not None for x in self.actions):
-            rv += ' - ranged'
+        for action in self.actions:
+            rv = '%s %s' % (rv, action)
         return rv
 
     def as_dict(self):

src/sentry/models/event.py

@@ -184,12 +184,19 @@ class EventCommon(object):
         return filter(None, [
             x.get_hash() for x in self.get_grouping_variants(force_config).values()])
 
-    def get_grouping_variants(self, force_config=None):
+    def get_grouping_variants(self, force_config=None, normalize_stacktraces=False):
         """
         This is similar to `get_hashes` but will instead return the
         grouping components for each variant in a dictionary.
+
+        If `normalize_stacktraces` is set to `True` then the event data will be
+        modified for `in_app` in addition to event variants being created.  This
+        means that after calling that function the event data has been modified
+        in place.
         """
-        from sentry.grouping.api import get_grouping_variants_for_event
+        from sentry.grouping.api import get_grouping_variants_for_event, \
+            load_grouping_config
+        from sentry.stacktraces.processing import normalize_stacktraces_for_grouping
 
         # Forcing configs has two separate modes.  One is where just the
         # config ID is given in which case it's merged with the stored or
@@ -208,6 +215,10 @@ class EventCommon(object):
         else:
             config = self.data.get('grouping_config')
 
+        config = load_grouping_config(config)
+        if normalize_stacktraces:
+            normalize_stacktraces_for_grouping(self.data, config)
+
         return get_grouping_variants_for_event(self, config)
 
     def get_primary_hash(self):

src/sentry/stacktraces/functions.py

@@ -4,6 +4,7 @@ from __future__ import absolute_import
 import re
 
 from sentry.stacktraces.platform import get_behavior_family_for_platform
+from sentry.utils.safe import setdefault_path
 
 
 _windecl_hash = re.compile(r'^@?(.*?)@[0-9]+$')
@@ -109,7 +110,7 @@ def trim_function_name(function, platform, normalize_lambdas=True):
         return function
 
     # Chop off C++ trailers
-    while 1:
+    while True:
         match = _cpp_trailer_re.search(function)
         if match is None:
             break
@@ -199,3 +200,13 @@ def get_function_name_for_frame(frame, platform=None):
     rv = frame.get('function')
     if rv:
         return trim_function_name(rv, frame.get('platform') or platform)
+
+
+def set_in_app(frame, value):
+    orig_in_app = frame.get('in_app')
+    if orig_in_app == value:
+        return
+
+    orig_in_app = int(orig_in_app) if orig_in_app is not None else -1
+    setdefault_path(frame, 'data', 'orig_in_app', value=orig_in_app)
+    frame['in_app'] = value

src/sentry/stacktraces/processing.py

@@ -11,7 +11,7 @@ from sentry.models import Project, Release
 from sentry.utils.cache import cache
 from sentry.utils.hashlib import hash_values
 from sentry.utils.safe import get_path, safe_execute
-from sentry.stacktraces.functions import trim_function_name
+from sentry.stacktraces.functions import set_in_app, trim_function_name
 
 
 logger = logging.getLogger(__name__)
@@ -218,12 +218,12 @@ def _normalize_in_app(stacktrace, platform=None, sdk_info=None):
     for frame in stacktrace:
         # If all frames are in_app, flip all of them. This is expected by the UI
         if not has_system_frames:
-            frame['in_app'] = False
+            set_in_app(frame, False)
 
         # Default to false in all cases where processors or grouping enhancers
         # have not yet set in_app.
         elif frame.get('in_app') is None:
-            frame['in_app'] = False
+            set_in_app(frame, False)
 
 
 def normalize_stacktraces_for_grouping(data, grouping_config=None):
@@ -250,6 +250,13 @@ def normalize_stacktraces_for_grouping(data, grouping_config=None):
     # unnecessarily.
     for frames in stacktraces:
         for frame in frames:
+            # Restore the original in_app value before the first grouping
+            # enhancers have been run. This allows to re-apply grouping
+            # enhancers on the original frame data.
+            orig_in_app = get_path(frame, 'data', 'orig_in_app')
+            if orig_in_app is not None:
+                frame['in_app'] = None if orig_in_app == -1 else bool(orig_in_app)
+
             if frame.get('raw_function') is not None:
                 continue
             raw_func = frame.get('function')

tests/sentry/event_manager/interfaces/snapshots/test_exception/test_context_with_only_app_frames.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-04-30T08:25:08.407188Z'
+created: '2019-06-05T20:07:56.986283Z'
 creator: sentry
 source: tests/sentry/event_manager/interfaces/test_exception.py
 ---
@@ -70,6 +70,8 @@ to_json:
     stacktrace:
       frames:
       - abs_path: foo/baz.py
+        data:
+          orig_in_app: 1
         filename: foo/baz.py
         in_app: false
         lineno: 1
@@ -79,6 +81,8 @@ to_json:
     stacktrace:
       frames:
       - abs_path: foo/baz.py
+        data:
+          orig_in_app: 1
         filename: foo/baz.py
         in_app: false
         lineno: 1

tests/sentry/grouping/snapshots/test_variants/test_event_hash_variant/combined@2019_04_07/native_limit_frames.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-05-16T11:55:53.361870Z'
+created: '2019-06-05T11:45:12.934622Z'
 creator: sentry
 source: tests/sentry/grouping/test_variants.py
 ---
@@ -27,10 +27,10 @@ system:
     system*
       exception*
         stacktrace*
-          frame (ignored because only 1 frame is considered by grouping enhancement rule (family:native))
+          frame (ignored because only 1 frame is considered by grouping enhancement rule (family:native max-frames=1))
             function* (isolated function)
               u'Scaleform::GFx::IME::GImeNamesManagerVista::OnActivated'
-          frame (ignored because only 1 frame is considered by grouping enhancement rule (family:native))
+          frame (ignored because only 1 frame is considered by grouping enhancement rule (family:native max-frames=1))
             function* (isolated function)
               u'Scaleform::GFx::AS3::IMEManager::DispatchEvent'
           frame*

tests/sentry/grouping/snapshots/test_variants/test_event_hash_variant/combined@2019_04_07/python_grouping_enhancer_away_from_crash.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-05-10T11:49:03.468450Z'
+created: '2019-06-05T11:45:13.045473Z'
 creator: sentry
 source: tests/sentry/grouping/test_variants.py
 ---
@@ -64,7 +64,7 @@ app:
               u'bound_func'
             lineno (line number is used only if module or filename are available)
               25
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'sentry.web.frontend.release_webhook'
             filename (module takes precedence)
@@ -130,7 +130,7 @@ system:
     system*
       exception*
         stacktrace*
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'django.core.handlers.base'
             filename (module takes precedence)
@@ -141,7 +141,7 @@ system:
               u'get_response'
             lineno (line number is used only if module or filename are available)
               112
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'django.views.generic.base'
             filename (module takes precedence)
@@ -152,7 +152,7 @@ system:
               u'view'
             lineno (line number is used only if module or filename are available)
               69
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'django.utils.decorators'
             filename (module takes precedence)
@@ -163,7 +163,7 @@ system:
               u'_wrapper'
             lineno (line number is used only if module or filename are available)
               29
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'django.views.decorators.csrf'
             filename (module takes precedence)
@@ -174,7 +174,7 @@ system:
               u'wrapped_view'
             lineno (line number is used only if module or filename are available)
               57
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'django.utils.decorators'
             filename (module takes precedence)
@@ -185,7 +185,7 @@ system:
               u'bound_func'
             lineno (line number is used only if module or filename are available)
               25
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'sentry.web.frontend.release_webhook'
             filename (module takes precedence)
@@ -196,7 +196,7 @@ system:
               u'dispatch'
             lineno (line number is used only if module or filename are available)
               37
-          frame (ignored by grouping enhancement rule (path:**/release_webhook.py - ranged))
+          frame (ignored by grouping enhancement rule (path:**/release_webhook.py v-group))
             module*
               u'django.views.generic.base'
             filename (module takes precedence)