Merge pull request #2592 from getsentry/improve-stacktrace-slimming

Prioritize pruning system frames

src/sentry/interfaces/exception.py

@@ -13,7 +13,7 @@
 from django.conf import settings
 
 from sentry.interfaces.base import Interface, InterfaceValidationError
-from sentry.interfaces.stacktrace import Stacktrace
+from sentry.interfaces.stacktrace import Stacktrace, slim_frame_data
 from sentry.utils import json
 from sentry.utils.safe import trim
 
@@ -40,14 +40,15 @@ class SingleException(Interface):
     display_score = 1200
 
     @classmethod
-    def to_python(cls, data, has_system_frames=None):
+    def to_python(cls, data, has_system_frames=None, slim_frames=True):
         if not (data.get('type') or data.get('value')):
             raise InterfaceValidationError("No 'type' or 'value' present")
 
         if data.get('stacktrace') and data['stacktrace'].get('frames'):
             stacktrace = Stacktrace.to_python(
                 data['stacktrace'],
                 has_system_frames=has_system_frames,
+                slim_frames=slim_frames,
             )
         else:
             stacktrace = None
@@ -164,15 +165,14 @@ def to_python(cls, data):
         if not data['values']:
             raise InterfaceValidationError("No 'values' present")
 
-        slim_frame_data(data)
-
         has_system_frames = cls.data_has_system_frames(data)
 
         kwargs = {
             'values': [
                 SingleException.to_python(
                     v,
                     has_system_frames=has_system_frames,
+                    slim_frames=False,
                 )
                 for v in data['values']
             ],
@@ -185,7 +185,10 @@ def to_python(cls, data):
         else:
             kwargs['exc_omitted'] = None
 
-        return cls(**kwargs)
+        instance = cls(**kwargs)
+        # we want to wait to slim things til we've reconciled in_app
+        slim_exception_data(instance)
+        return instance
 
     @classmethod
     def data_has_system_frames(cls, data):
@@ -285,33 +288,17 @@ def get_stacktrace(self, *args, **kwargs):
         return ''
 
 
-def slim_frame_data(data,
-                    frame_allowance=settings.SENTRY_MAX_STACKTRACE_FRAMES):
+def slim_exception_data(instance, frame_allowance=settings.SENTRY_MAX_STACKTRACE_FRAMES):
     """
     Removes various excess metadata from middle frames which go beyond
     ``frame_allowance``.
     """
     # TODO(dcramer): it probably makes sense to prioritize a certain exception
-    # rather than keeping the top and bottom frames from the entire stack
-    frames_len = 0
-    for exception in data['values']:
-        if not exception.get('stacktrace'):
+    # rather than distributing allowance among all exceptions
+    frames = []
+    for exception in instance.values:
+        if not exception.stacktrace:
             continue
-        frames_len += len(exception['stacktrace']['frames'])
-
-    if frames_len <= frame_allowance:
-        return
+        frames.extend(exception.stacktrace.frames)
 
-    half_max = frame_allowance / 2
-
-    pos = 0
-    for exception in data['values']:
-        if not exception.get('stacktrace'):
-            continue
-        for frame in exception['stacktrace']['frames']:
-            pos += 1
-            if pos > half_max and pos <= frames_len - half_max:
-                # remove heavy components
-                frame.pop('vars', None)
-                frame.pop('pre_context', None)
-                frame.pop('post_context', None)
+    slim_frame_data(frames, frame_allowance)

src/sentry/interfaces/stacktrace.py

@@ -127,25 +127,53 @@ def remove_module_outliers(module):
     return _java_enhancer_re.sub(r'\1<auto>', module)
 
 
-def slim_frame_data(stacktrace,
-                    frame_allowance=settings.SENTRY_MAX_STACKTRACE_FRAMES):
+def slim_frame_data(frames, frame_allowance=settings.SENTRY_MAX_STACKTRACE_FRAMES):
     """
     Removes various excess metadata from middle frames which go beyond
     ``frame_allowance``.
     """
-    frames = stacktrace['frames']
-    frames_len = len(frames)
+    frames_len = 0
+    app_frames = []
+    system_frames = []
+    for frame in frames:
+        frames_len += 1
+        if frame.in_app:
+            app_frames.append(frame)
+        else:
+            system_frames.append(frame)
 
     if frames_len <= frame_allowance:
         return
 
-    half_max = frame_allowance / 2
+    remaining = frames_len - frame_allowance
+    app_count = len(app_frames)
+    system_allowance = max(frame_allowance - app_count, 0)
+    if system_allowance:
+        half_max = system_allowance / 2
+        # prioritize trimming system frames
+        for frame in system_frames[half_max:-half_max]:
+            frame.vars = None
+            frame.pre_context = None
+            frame.post_context = None
+            remaining -= 1
+
+    else:
+        for frame in system_frames:
+            frame.vars = None
+            frame.pre_context = None
+            frame.post_context = None
+            remaining -= 1
+
+    if not remaining:
+        return
+
+    app_allowance = app_count - remaining
+    half_max = app_allowance / 2
 
-    for n in xrange(half_max, frames_len - half_max):
-        # remove heavy components
-        frames[n].pop('vars', None)
-        frames[n].pop('pre_context', None)
-        frames[n].pop('post_context', None)
+    for frame in app_frames[half_max:-half_max]:
+        frame.vars = None
+        frame.pre_context = None
+        frame.post_context = None
 
 
 def validate_bool(value, required=True):
@@ -505,12 +533,10 @@ def __iter__(self):
         return iter(self.frames)
 
     @classmethod
-    def to_python(cls, data, has_system_frames=None):
+    def to_python(cls, data, has_system_frames=None, slim_frames=True):
         if not data.get('frames'):
             raise InterfaceValidationError("No 'frames' present")
 
-        slim_frame_data(data)
-
         if has_system_frames is None:
             has_system_frames = cls.data_has_system_frames(data)
 
@@ -539,7 +565,10 @@ def to_python(cls, data, has_system_frames=None):
 
         kwargs['has_system_frames'] = has_system_frames
 
-        return cls(**kwargs)
+        instance = cls(**kwargs)
+        if slim_frames:
+            slim_frame_data(instance)
+        return instance
 
     @classmethod
     def data_has_system_frames(cls, data):

tests/sentry/interfaces/test_exception.py

@@ -5,7 +5,7 @@
 from exam import fixture
 
 from sentry.interfaces.exception import (
-    SingleException, Exception, slim_frame_data
+    SingleException, Exception, slim_exception_data
 )
 from sentry.testutils import TestCase
 
@@ -235,46 +235,47 @@ def test_handles_type_in_value(self):
         assert result.value == 'unauthorized'
 
 
-class TrimExceptionsTest(TestCase):
+class SlimExceptionDataTest(TestCase):
     def test_under_max(self):
-        value = {'values': [
+        interface = Exception.to_python({'values': [
             {'value': 'foo',
              'stacktrace': {'frames': [{'filename': 'foo'}]},
             }
-        ]}
-        slim_frame_data(value)
-        assert len(value['values'][0]['stacktrace']['frames']) == 1
+        ]})
+        slim_exception_data(interface)
+        assert len(interface.values[0].stacktrace.frames) == 1
 
     def test_over_max(self):
         values = []
-        data = {'values': values}
         for x in xrange(5):
             exc = {'value': 'exc %d' % x, 'stacktrace': {'frames': []}}
             values.append(exc)
             for y in xrange(5):
                 exc['stacktrace']['frames'].append({
-                    'filename': 'frame %d' % y,
-                    'vars': {},
-                    'pre_context': [],
-                    'post_context': [],
+                    'filename': 'exc %d frame %d' % (x, y),
+                    'vars': {'foo': 'bar'},
+                    'context_line': 'b',
+                    'pre_context': ['a'],
+                    'post_context': ['c'],
                 })
 
+        interface = Exception.to_python({'values': values})
+
         # slim to 10 frames to make tests easier
-        slim_frame_data(data, 10)
-
-        assert len(values) == 5
-        for e_num, value in enumerate(values):
-            assert value['value'] == 'exc %d' % e_num
-            assert len(value['stacktrace']['frames']) == 5
-            for f_num, frame in enumerate(value['stacktrace']['frames']):
-                assert frame['filename'] == 'frame %d' % f_num
+        slim_exception_data(interface, 10)
+
+        assert len(interface.values) == 5
+        for e_num, value in enumerate(interface.values):
+            assert value.value == 'exc %d' % e_num
+            assert len(value.stacktrace.frames) == 5
+            for f_num, frame in enumerate(value.stacktrace.frames):
+                assert frame.filename == 'exc %d frame %d' % (e_num, f_num)
+                print(frame.filename)
                 if e_num in (0, 4):
-                    assert frame['filename'] == 'frame %d' % f_num
-                    assert frame['vars'] is not None
-                    assert frame['pre_context'] is not None
-                    assert frame['post_context'] is not None
+                    assert frame.vars is not None
+                    assert frame.pre_context is not None
+                    assert frame.post_context is not None
                 else:
-                    assert frame['filename'] == 'frame %d' % f_num
-                    assert 'vars' not in frame
-                    assert 'pre_context' not in frame
-                    assert 'post_context' not in frame
+                    assert frame.vars is None
+                    assert frame.pre_context is None
+                    assert frame.post_context is None

tests/sentry/interfaces/test_stacktrace.py

@@ -430,41 +430,43 @@ def test_context_with_nan(self):
 
 class SlimFrameDataTest(TestCase):
     def test_under_max(self):
-        value = {'frames': [{'filename': 'foo'}]}
-        slim_frame_data(value)
-        assert len(value['frames']) == 1
-        assert value.get('frames_omitted') is None
+        interface = Stacktrace.to_python({'frames': [{'filename': 'foo'}]})
+        slim_frame_data(interface, 4)
+        assert len(interface.frames) == 1
+        assert not interface.frames_omitted
 
     def test_over_max(self):
         values = []
         for n in xrange(5):
             values.append({
                 'filename': 'frame %d' % n,
-                'vars': {},
-                'pre_context': [],
-                'post_context': [],
+                'vars': {'foo': 'bar'},
+                'context_line': 'b',
+                'pre_context': ['a'],
+                'post_context': ['c'],
             })
-        value = {'frames': values}
-        slim_frame_data(value, 4)
-
-        assert len(value['frames']) == 5
-
-        for value, num in zip(values[:2], xrange(2)):
-            assert value['filename'] == 'frame %d' % num
-            assert value['vars'] is not None
-            assert value['pre_context'] is not None
-            assert value['post_context'] is not None
-
-        assert values[2]['filename'] == 'frame 2'
-        assert 'vars' not in values[2]
-        assert 'pre_context' not in values[2]
-        assert 'post_context' not in values[2]
-
-        for value, num in zip(values[3:], xrange(3, 5)):
-            assert value['filename'] == 'frame %d' % num
-            assert value['vars'] is not None
-            assert value['pre_context'] is not None
-            assert value['post_context'] is not None
+        interface = Stacktrace.to_python({'frames': values})
+        slim_frame_data(interface, 4)
+
+        assert len(interface.frames) == 5
+
+        for value, num in zip(interface.frames[:2], xrange(2)):
+            assert value.filename == 'frame %d' % num
+            assert value.vars is not None
+            assert value.pre_context is not None
+            assert value.post_context is not None
+
+        for value, num in zip(interface.frames[3:], xrange(3, 5)):
+            assert value.filename == 'frame %d' % num
+            assert value.vars is not None
+            assert value.pre_context is not None
+            assert value.post_context is not None
+
+        value = interface.frames[2]
+        assert value.filename == 'frame 2'
+        assert not value.vars
+        assert not value.pre_context
+        assert not value.post_context
 
 
 def test_java_frame_rendering():