From 737c0c5f9ac8fb6a35344b29d181909b2b783054 Mon Sep 17 00:00:00 2001 From: Benjamin Wohlwend Date: Tue, 20 Feb 2018 18:26:19 +0100 Subject: [PATCH] ignore elasticapm frames from frame collection logic for spans (#167) this should clean up the collected stack traces quite a bit Note, for Django this has been done a bit differently, this specialization has been removed. closes #167 --- elasticapm/base.py | 11 +++++++---- elasticapm/contrib/django/client.py | 11 +---------- elasticapm/instrumentation/packages/base.py | 2 -- elasticapm/utils/stacks.py | 17 +++++++++++------ tests/client/client_tests.py | 6 +++--- tests/contrib/django/django_tests.py | 2 +- tests/instrumentation/base_tests.py | 10 ++++++++++ 7 files changed, 33 insertions(+), 26 deletions(-) diff --git a/elasticapm/base.py b/elasticapm/base.py index 2aec7c8653..7e0d1bdaec 100644 --- a/elasticapm/base.py +++ b/elasticapm/base.py @@ -129,9 +129,9 @@ def __init__(self, config=None, **defaults): self.processors = [import_string(p) for p in self.config.processors] if self.config.processors else [] - self.instrumentation_store = TransactionsStore( - lambda: self._get_stack_info_for_trace( - stacks.iter_stack_frames(), + def frames_collector_func(): + return self._get_stack_info_for_trace( + stacks.iter_stack_frames(ignore_modules=('elasticapm.',)), library_frame_context_lines=self.config.source_lines_span_library_frames, in_app_frame_context_lines=self.config.source_lines_span_app_frames, with_locals=self.config.collect_local_variables in ('all', 'transactions'), @@ -140,7 +140,10 @@ def __init__(self, config=None, **defaults): list_length=self.config.local_var_list_max_length, string_length=self.config.local_var_max_length, ), local_var) - ), + ) + + self.instrumentation_store = TransactionsStore( + frames_collector_func=frames_collector_func, collect_frequency=self.config.flush_interval, sample_rate=self.config.transaction_sample_rate, max_spans=self.config.transaction_max_spans, diff --git a/elasticapm/contrib/django/client.py b/elasticapm/contrib/django/client.py index 82e4d38172..0252ee1f4f 100644 --- a/elasticapm/contrib/django/client.py +++ b/elasticapm/contrib/django/client.py @@ -179,7 +179,7 @@ def _get_stack_info_for_trace(self, frames, locals_processor_func=None): """If the stacktrace originates within the elasticapm module, it will skip frames until some other module comes up.""" - frames = list(iterate_with_template_sources( + return list(iterate_with_template_sources( frames, with_locals=with_locals, library_frame_context_lines=library_frame_context_lines, @@ -188,15 +188,6 @@ def _get_stack_info_for_trace(self, frames, exclude_paths_re=self.exclude_paths_re, locals_processor_func=locals_processor_func, )) - i = 0 - while len(frames) > i: - if 'module' in frames[i] and not ( - frames[i]['module'].startswith('elasticapm.') or - frames[i]['module'] == 'contextlib' - ): - return frames[i:] - i += 1 - return frames def send(self, url, **kwargs): """ diff --git a/elasticapm/instrumentation/packages/base.py b/elasticapm/instrumentation/packages/base.py index 7826921a9e..876f0b94c6 100644 --- a/elasticapm/instrumentation/packages/base.py +++ b/elasticapm/instrumentation/packages/base.py @@ -25,7 +25,6 @@ def __call__(self, *args, **kwargs): # the bound function rather than a bound wrapper for the bound # function. When evaluating enabled, if it is callable we call # it, otherwise we evaluate it as a boolean. - if self._self_enabled is not None: if callable(self._self_enabled): if not self._self_enabled(): @@ -98,7 +97,6 @@ def __call__(self, *args, **kwargs): # the bound function rather than a bound wrapper for the bound # function. When evaluating enabled, if it is callable we call # it, otherwise we evaluate it as a boolean. - if self._self_enabled is not None: if callable(self._self_enabled): if not self._self_enabled(): diff --git a/elasticapm/utils/stacks.py b/elasticapm/utils/stacks.py index c2117ebc7c..60c86d1432 100644 --- a/elasticapm/utils/stacks.py +++ b/elasticapm/utils/stacks.py @@ -138,7 +138,7 @@ def to_dict(dictish): return dict((k, dictish[k]) for k in m()) -def iter_traceback_frames(tb): +def iter_traceback_frames(tb, ignore_modules=()): """ Given a traceback object, it will iterate over all frames that do not contain the ``__traceback_hide__`` @@ -147,13 +147,16 @@ def iter_traceback_frames(tb): while tb: # support for __traceback_hide__ which is used by a few libraries # to hide internal frames. - f_locals = getattr(tb.tb_frame, 'f_locals', {}) - if not _getitem_from_frame(f_locals, '__traceback_hide__'): - yield tb.tb_frame, getattr(tb, 'tb_lineno', None) + frame = tb.tb_frame + f_locals = getattr(frame, 'f_locals', {}) + f_globals = getattr(frame, 'f_globals', {}) + if not (f_globals.get('__name__', '').startswith(ignore_modules) or + _getitem_from_frame(f_locals, '__traceback_hide__')): + yield frame, getattr(tb, 'tb_lineno', None) tb = tb.tb_next -def iter_stack_frames(frames=None, skip=0): +def iter_stack_frames(frames=None, skip=0, ignore_modules=()): """ Given an optional list of frames (defaults to current stack), iterates over all frames that do not contain the ``__traceback_hide__`` @@ -166,7 +169,9 @@ def iter_stack_frames(frames=None, skip=0): if i < skip: continue f_locals = getattr(frame, 'f_locals', {}) - if not _getitem_from_frame(f_locals, '__traceback_hide__'): + f_globals = getattr(frame, 'f_globals', {}) + if not (f_globals.get('__name__', '').startswith(ignore_modules) or + _getitem_from_frame(f_locals, '__traceback_hide__')): yield frame, frame.f_lineno diff --git a/tests/client/client_tests.py b/tests/client/client_tests.py index 11aebc6d41..e3f659e519 100644 --- a/tests/client/client_tests.py +++ b/tests/client/client_tests.py @@ -552,7 +552,7 @@ def test_collect_local_variables_transactions(should_collect, elasticapm_client) pass elasticapm_client.end_transaction('test', 'ok') transaction = elasticapm_client.instrumentation_store.get_all()[0] - frame = transaction['spans'][0]['stacktrace'][5] + frame = transaction['spans'][0]['stacktrace'][0] if mode in ('transactions', 'all'): assert 'vars' in frame, mode assert frame['vars']['a_local_var'] == 1 @@ -578,8 +578,8 @@ def test_collect_source_transactions(should_collect, elasticapm_client): pass elasticapm_client.end_transaction('test', 'ok') transaction = elasticapm_client.instrumentation_store.get_all()[0] - in_app_frame = transaction['spans'][0]['stacktrace'][5] - library_frame = transaction['spans'][0]['stacktrace'][6] + in_app_frame = transaction['spans'][0]['stacktrace'][0] + library_frame = transaction['spans'][0]['stacktrace'][1] assert not in_app_frame['library_frame'] assert library_frame['library_frame'] if library_frame_context: diff --git a/tests/contrib/django/django_tests.py b/tests/contrib/django/django_tests.py index 1c6c15b079..f2cf3a611d 100644 --- a/tests/contrib/django/django_tests.py +++ b/tests/contrib/django/django_tests.py @@ -966,7 +966,7 @@ def test_stacktrace_filtered_for_elasticapm(client, django_elasticapm_client): assert spans[1]['name'] == 'list_users.html' # Top frame should be inside django rendering - assert spans[1]['stacktrace'][0]['module'].startswith('django.template') + assert spans[1]['stacktrace'][0]['module'].startswith('django.template'), spans[1]['stacktrace'][0]['function'] @pytest.mark.parametrize('django_elasticapm_client', [{'_wait_to_first_send': 100}], indirect=True) diff --git a/tests/instrumentation/base_tests.py b/tests/instrumentation/base_tests.py index 89f1b02d07..c10e08f0d0 100644 --- a/tests/instrumentation/base_tests.py +++ b/tests/instrumentation/base_tests.py @@ -4,6 +4,7 @@ import mock import pytest +import elasticapm from elasticapm.instrumentation.packages.base import (AbstractInstrumentedModule, OriginalNamesBoundFunctionWrapper) from elasticapm.utils import compat @@ -79,3 +80,12 @@ def test_skip_instrument_env_var(): with mock.patch.dict('os.environ', {'SKIP_INSTRUMENT_TEST_DUMMY_INSTRUMENT': 'foo'}): instrumentation.instrument() assert not instrumentation.instrumented + + +def test_skip_ignored_frames(elasticapm_client): + elasticapm_client.begin_transaction('test') + with elasticapm.capture_span('test'): + pass + transaction = elasticapm_client.end_transaction('test', 'test') + for frame in transaction.spans[0].frames: + assert not frame['module'].startswith('elasticapm')