Skip to content

Commit

Permalink
Merge pull request #449 from rollbar/fixed/shortener-multi-level-shor…
Browse files Browse the repository at this point in the history
…tening

Fixed shortener multi level shortening
  • Loading branch information
danielmorell authored May 24, 2024
2 parents ad596ac + 7ece3cb commit 00cd4d5
Show file tree
Hide file tree
Showing 5 changed files with 192 additions and 79 deletions.
14 changes: 7 additions & 7 deletions rollbar/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -402,12 +402,6 @@ def init(access_token, environment='production', scrub_fields=None, url_fields=N
# trace frame values using the ShortReprTransform.
_serialize_transform = SerializableTransform(safe_repr=SETTINGS['locals']['safe_repr'],
safelist_types=SETTINGS['locals']['safelisted_types'])
_transforms = [
ScrubRedactTransform(),
_serialize_transform,
ScrubTransform(suffixes=[(field,) for field in SETTINGS['scrub_fields']], redact_char='*'),
ScrubUrlTransform(suffixes=[(field,) for field in SETTINGS['url_fields']], params_to_scrub=SETTINGS['scrub_fields'])
]

# A list of key prefixes to apply our shortener transform to. The request
# being included in the body key is old behavior and is being retained for
Expand All @@ -431,7 +425,13 @@ def init(access_token, environment='production', scrub_fields=None, url_fields=N
shortener = ShortenerTransform(safe_repr=SETTINGS['locals']['safe_repr'],
keys=shortener_keys,
**SETTINGS['locals']['sizes'])
_transforms.append(shortener)
_transforms = [
shortener,
ScrubRedactTransform(),
_serialize_transform,
ScrubTransform(suffixes=[(field,) for field in SETTINGS['scrub_fields']], redact_char='*'),
ScrubUrlTransform(suffixes=[(field,) for field in SETTINGS['url_fields']], params_to_scrub=SETTINGS['scrub_fields'])
]
_threads = queue.Queue()
events.reset()
filters.add_builtin_filters(SETTINGS)
Expand Down
27 changes: 19 additions & 8 deletions rollbar/lib/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,36 @@ def prefix_match(key, prefixes):
return False


def key_in(key, keys):
def key_in(key, canonicals):
if not key:
return False

for k in keys:
if key_match(k, key):
for c in canonicals:
if key_match(key, c):
return True

return False


def key_match(key1, key2):
if len(key1) != len(key2):
def key_depth(key, canonicals) -> int:
if not key:
return 0

for c in canonicals:
if key_match(key, c):
return len(c)

return 0


def key_match(key, canonical):
if len(key) < len(canonical):
return False

for p1, p2 in zip(key1, key2):
if '*' == p1 or '*' == p2:
for k, c in zip(key, canonical):
if '*' == c:
continue
if p1 == p2:
if c == k:
continue
return False

Expand Down
54 changes: 19 additions & 35 deletions rollbar/lib/transforms/shortener.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from collections.abc import Mapping

from rollbar.lib import (
integer_types, key_in, number_types, sequence_types,
integer_types, key_in, key_depth, number_types, sequence_types,
string_types)
from rollbar.lib.transform import Transform

Expand Down Expand Up @@ -47,8 +47,6 @@ def _get_max_size(self, obj):

return self._repr.maxother

def _get_max_level(self):
return getattr(self._repr, 'maxlevel')
def _shorten_sequence(self, obj, max_keys):
_len = len(obj)
if _len <= max_keys:
Expand Down Expand Up @@ -79,44 +77,14 @@ def _shorten_other(self, obj):

return self._repr.repr(obj)

def traverse_obj(self, obj, level=1):
def seq_iter(o):
return o if isinstance(o, dict) else range(len(o))

for k in seq_iter(obj):
max_size = self._get_max_size(obj[k])
if isinstance(obj[k], dict):
obj[k] = self._shorten_mapping(obj[k], max_size)
if level == self._get_max_level():
del obj[k]
return
self.traverse_obj(obj[k], level + 1)
elif isinstance(obj[k], sequence_types):
obj[k] = self._shorten_sequence(obj[k], max_size)
if level == self._get_max_level():
del obj[k]
return
self.traverse_obj(obj[k], level + 1)
else:
obj[k] = self._shorten(obj[k])
return obj

def _shorten(self, val):
max_size = self._get_max_size(val)

if isinstance(val, dict):
val = self._shorten_mapping(val, max_size)
return self.traverse_obj(val)

if isinstance(val, string_types):
return self._shorten_mapping(val, max_size)
if isinstance(val, (string_types, sequence_types)):
return self._shorten_sequence(val, max_size)

if isinstance(val, sequence_types):
val = self._shorten_sequence(val, max_size)
if isinstance(val, string_types):
return val
return self.traverse_obj(val)

if isinstance(val, number_types):
return self._shorten_basic(val, self._repr.maxlong)

Expand All @@ -128,7 +96,23 @@ def _should_shorten(self, val, key):

return key_in(key, self.keys)

def _should_drop(self, val, key) -> bool:
if not key:
return False

maxdepth = key_depth(key, self.keys)
if maxdepth == 0:
return False

return (maxdepth + self._repr.maxlevel) <= len(key)

def default(self, o, key=None):
if self._should_drop(o, key):
if isinstance(o, dict):
return '{...}'
if isinstance(o, sequence_types):
return '[...]'

if self._should_shorten(o, key):
return self._shorten(o)

Expand Down
60 changes: 51 additions & 9 deletions rollbar/test/test_lib.py
Original file line number Diff line number Diff line change
@@ -1,24 +1,66 @@
from rollbar.lib import dict_merge, prefix_match
from rollbar.lib import dict_merge, prefix_match, key_match, key_depth

from rollbar.test import BaseTest


class RollbarLibTest(BaseTest):
def test_dict_merge_not_dict(self):
a = {'a': {'b': 42}}
b = 99
result = dict_merge(a, b)

self.assertEqual(99, result)

def test_prefix_match(self):
key = ['password', 'argspec', '0']
self.assertTrue(prefix_match(key, [['password']]))

def test_prefix_match(self):
def test_prefix_match_dont_match(self):
key = ['environ', 'argspec', '0']
self.assertFalse(prefix_match(key, [['password']]))

def test_key_match(self):
canonical = ['body', 'trace', 'frames', '*', 'locals', '*']
key = ['body', 'trace', 'frames', 5, 'locals', 'foo']

self.assertTrue(key_match(key, canonical))

def test_key_match_dont_match(self):
canonical = ['body', 'trace', 'frames', '*', 'locals', '*']
key = ['body', 'trace', 'frames', 5, 'bar', 'foo']

self.assertFalse(key_match(key, canonical))

def test_key_match_wildcard_end(self):
canonical = ['body', 'trace', 'frames', '*', 'locals', '*']
key = ['body', 'trace', 'frames', 5, 'locals', 'foo', 'bar']

self.assertTrue(key_match(key, canonical))

def test_key_match_too_short(self):
canonical = ['body', 'trace', 'frames', '*', 'locals', '*']
key = ['body', 'trace', 'frames', 5, 'locals']

self.assertFalse(key_match(key, canonical))

def test_key_depth(self):
canonicals = [['body', 'trace', 'frames', '*', 'locals', '*']]
key = ['body', 'trace', 'frames', 5, 'locals', 'foo']

self.assertEqual(6, key_depth(key, canonicals))

def test_key_depth_dont_match(self):
canonicals = [['body', 'trace', 'frames', '*', 'locals', '*']]
key = ['body', 'trace', 'frames', 5, 'bar', 'foo']

self.assertEqual(0, key_depth(key, canonicals))

def test_key_depth_wildcard_end(self):
canonicals = [['body', 'trace', 'frames', '*']]
key = ['body', 'trace', 'frames', 5, 'locals', 'foo', 'bar']

self.assertEqual(4, key_depth(key, canonicals))

def test_dict_merge_not_dict(self):
a = {'a': {'b': 42}}
b = 99
result = dict_merge(a, b)

self.assertEqual(99, result)

def test_dict_merge_dicts_independent(self):
a = {'a': {'b': 42}}
b = {'x': {'y': 99}}
Expand Down
116 changes: 96 additions & 20 deletions rollbar/test/test_shortener_transform.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,27 +37,22 @@ def setUp(self):
'frozenset': frozenset([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]),
'array': array('l', [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]),
'deque': deque([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11], 15),
'other': TestClassWithAVeryVeryVeryVeryVeryVeryVeryLongName(),
'list_max_level': [1, [2, [3, [4, ["good_5", ["bad_6", ["bad_7"]]]]]]],
'dict_max_level': {1: 1, 2: {3: {4: {"level4": "good", "level5": {"toplevel": "ok", 6: {7: {}}}}}}},
'list_multi_level': [1, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]]
'other': TestClassWithAVeryVeryVeryVeryVeryVeryVeryLongName()
}

def _assert_shortened(self, key, expected):
shortener = ShortenerTransform(keys=[(key,)], **DEFAULT_LOCALS_SIZES)
result = transforms.transform(self.data, shortener)

if key == 'dict':
self.assertEqual(expected, len(result[key]))
elif key in ('list_max_level', 'dict_max_level', 'list_multi_level'):
self.assertEqual(expected, result[key])
self.assertEqual(expected, len(result))
else:
# the repr output can vary between Python versions
stripped_result_key = result[key].strip("'\"u")

if key == 'other':
self.assertIn(expected, stripped_result_key)
elif key not in ('dict', 'list_max_level', 'dict_max_level', 'list_multi_level'):
elif key != 'dict':
self.assertEqual(expected, stripped_result_key)

# make sure nothing else was shortened
Expand Down Expand Up @@ -87,18 +82,6 @@ def test_shorten_list(self):
expected = '[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...]'
self._assert_shortened('list', expected)

def test_shorten_list_max_level(self):
expected = [1, [2, [3, [4, ['good_5']]]]]
self._assert_shortened('list_max_level', expected)

def test_shorten_list_multi_level(self):
expected = [1, '[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...]']
self._assert_shortened('list_multi_level', expected)

def test_shorten_dict_max_level(self):
expected = {1: 1, 2: {3: {4: {'level4': 'good', 'level5': {'toplevel': 'ok'}}}}}
self._assert_shortened('dict_max_level', expected)

def test_shorten_tuple(self):
expected = '(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...)'
self._assert_shortened('tuple', expected)
Expand Down Expand Up @@ -145,3 +128,96 @@ def test_shorten_object(self):
self.assertEqual(type(result), dict)
self.assertEqual(len(result['request']['POST']), 10)

def test_shorten_frame(self):
data = {
'body': {
'trace': {
'frames': [
{
"filename": "/path/to/app.py",
"lineno": 82,
"method": "sub_func",
"code": "extra(**kwargs)",
"keywordspec": "kwargs",
"locals": {
"kwargs": {
"app": ["foo", "bar", "baz", "qux", "quux", "corge", "grault", "garply", "waldo",
"fred", "plugh", "xyzzy", "thud"],
"extra": {
"request": "<class 'some.package.MyClass'>"
}
},
"one": {
"two": {
"three": {
"four": {
"five": {
"six": {
"seven": 8,
"eight": "nine"
},
"ten": "Yep! this should still be here, but it is a little on the "
"long side, so we might want to cut it down a bit."
}
}
}
},
"a": ["foo", "bar", "baz", "qux", 5, 6, 7, 8, 9, 10, 11, 12],
"b": 14071504106566481658450568387453168916351054663,
"app_id": 140715046161904,
"bar": "im a bar",
}
}
}
]
}
}
}
keys = [('body', 'trace', 'frames', '*', 'locals', '*')]
shortener = ShortenerTransform(keys=keys, **DEFAULT_LOCALS_SIZES)
result = transforms.transform(data, shortener)
expected = {
'body': {
'trace': {
'frames': [
{
"filename": "/path/to/app.py",
"lineno": 82,
"method": "sub_func",
"code": "extra(**kwargs)",
"keywordspec": "kwargs",
"locals": {
"kwargs": {
# Shortened
"app": "['foo', 'bar', 'baz', 'qux', 'quux', 'corge', 'grault', 'garply', 'waldo', "
"'fred', ...]",
"extra": {
"request": "<class 'some.package.MyClass'>"
}
},
"one": {
"two": {
"three": {
"four": {
"five": {
"six": '{...}', # Dropped because it is past the maxlevel.
# Shortened
"ten": "'Yep! this should still be here, but it is a lit...ong "
"side, so we might want to cut it down a bit.'"
}
}
}
},
"a": "['foo', 'bar', 'baz', 'qux', 5, 6, 7, 8, 9, 10, ...]", # Shortened
"b": '140715041065664816...7453168916351054663', # Shortened
"app_id": 140715046161904,
"bar": "im a bar",
}
}
}
]
}
}
}

self.assertEqual(result, expected)

0 comments on commit 00cd4d5

Please sign in to comment.