replace subclassing of wrapt wrappers by using a partial (#164)

We need to pass along the method and module names to the function
wrapper. This has so far been done by subclassing wrapt.FunctionWrapper.
While this works, it adds a lot of duplicate code that we need to sync
up every time we update wrapt. Using a partial should mean much less
maintenance work.

closes #164

elasticapm/base.py

@@ -129,9 +129,16 @@ def __init__(self, config=None, **defaults):
 
         self.processors = [import_string(p) for p in self.config.processors] if self.config.processors else []
 
+        if platform.python_implementation() == 'PyPy':
+            # PyPy introduces a `_functools.partial.__call__` frame due to our use
+            # of `partial` in AbstractInstrumentedModule
+            skip_modules = ('elasticapm.', '_functools')
+        else:
+            skip_modules = ('elasticapm.',)
+
         def frames_collector_func():
             return self._get_stack_info_for_trace(
-                stacks.iter_stack_frames(skip_top_modules=('elasticapm.',)),
+                stacks.iter_stack_frames(skip_top_modules=skip_modules),
                 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'),

elasticapm/instrumentation/packages/base.py

@@ -8,129 +8,6 @@
 logger = logging.getLogger(__name__)
 
 
-# We have our own `BoundFunctionWrapper` and `FunctionWrapper` here because
-# we want them to be able to know about `module` and `method` and supply it in
-# the call to the wrapper.
-
-class OriginalNamesBoundFunctionWrapper(wrapt.BoundFunctionWrapper):
-
-    def __init__(self, *args, **kwargs):
-        super(OriginalNamesBoundFunctionWrapper, self).__init__(*args, **kwargs)
-        self._self_module = self._self_parent._self_module
-        self._self_method = self._self_parent._self_method
-
-    def __call__(self, *args, **kwargs):
-        # If enabled has been specified, then evaluate it at this point
-        # and if the wrapper is not to be executed, then simply return
-        # 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():
-                    return self.__wrapped__(*args, **kwargs)
-            elif not self._self_enabled:
-                return self.__wrapped__(*args, **kwargs)
-
-        # We need to do things different depending on whether we are
-        # likely wrapping an instance method vs a static method or class
-        # method.
-
-        if self._self_binding == 'function':
-            if self._self_instance is None:
-                # This situation can occur where someone is calling the
-                # instancemethod via the class type and passing the instance
-                # as the first argument. We need to shift the args before
-                # making the call to the wrapper and effectively bind the
-                # instance to the wrapped function using a partial so the
-                # wrapper doesn't see anything as being different.
-
-                if not args:
-                    raise TypeError(
-                        'missing 1 required positional argument')
-
-                instance, args = args[0], args[1:]
-                wrapped = functools.partial(self.__wrapped__, instance)
-                return self._self_wrapper(self._self_module,
-                                          self._self_method,
-                                          wrapped, instance, args, kwargs)
-
-            return self._self_wrapper(self._self_module,
-                                      self._self_method,
-                                      self.__wrapped__, self._self_instance,
-                                      args, kwargs)
-
-        else:
-            # As in this case we would be dealing with a classmethod or
-            # staticmethod, then _self_instance will only tell us whether
-            # when calling the classmethod or staticmethod they did it via an
-            # instance of the class it is bound to and not the case where
-            # done by the class type itself. We thus ignore _self_instance
-            # and use the __self__ attribute of the bound function instead.
-            # For a classmethod, this means instance will be the class type
-            # and for a staticmethod it will be None. This is probably the
-            # more useful thing we can pass through even though we loose
-            # knowledge of whether they were called on the instance vs the
-            # class type, as it reflects what they have available in the
-            # decoratored function.
-
-            instance = getattr(self.__wrapped__, '__self__', None)
-
-            return self._self_wrapper(self._self_module,
-                                      self._self_method,
-                                      self.__wrapped__, instance, args,
-                                      kwargs)
-
-
-class OriginalNamesFunctionWrapper(wrapt.FunctionWrapper):
-
-    __bound_function_wrapper__ = OriginalNamesBoundFunctionWrapper
-
-    def __init__(self, wrapped, wrapper, module, method):
-        super(OriginalNamesFunctionWrapper, self).__init__(wrapped, wrapper)
-        self._self_module = module
-        self._self_method = method
-
-    def __call__(self, *args, **kwargs):
-        # If enabled has been specified, then evaluate it at this point
-        # and if the wrapper is not to be executed, then simply return
-        # 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():
-                    return self.__wrapped__(*args, **kwargs)
-            elif not self._self_enabled:
-                return self.__wrapped__(*args, **kwargs)
-
-        # This can occur where initial function wrapper was applied to
-        # a function that was already bound to an instance. In that case
-        # we want to extract the instance from the function and use it.
-
-        if self._self_binding == 'function':
-            if self._self_instance is None:
-                instance = getattr(self.__wrapped__, '__self__', None)
-                if instance is not None:
-                    return self._self_wrapper(self._self_module,
-                                              self._self_method,
-                                              self.__wrapped__, instance,
-                                              args, kwargs)
-
-        # This is generally invoked when the wrapped function is being
-        # called as a normal function and is not bound to a class as an
-        # instance method. This is also invoked in the case where the
-        # wrapped function was a method, but this wrapper was in turn
-        # wrapped using the staticmethod decorator.
-
-        return self._self_wrapper(self._self_module,
-                                  self._self_method,
-                                  self.__wrapped__, self._self_instance,
-                                  args, kwargs)
-
-
 class AbstractInstrumentedModule(object):
     name = None
 
@@ -188,11 +65,9 @@ def instrument(self):
                     # `module`/`method` in the call to `call_if_sampling`
                     parent, attribute, original = wrapt.resolve_path(module, method)
                     self.originals[(module, method)] = original
-                    wrapper = OriginalNamesFunctionWrapper(
+                    wrapper = wrapt.FunctionWrapper(
                         original,
-                        self.call_if_sampling,
-                        module,
-                        method
+                        functools.partial(self.call_if_sampling, module, method),
                     )
                     wrapt.apply_patch(parent, attribute, wrapper)
                 except ImportError:

tests/instrumentation/base_tests.py

@@ -5,14 +5,13 @@
 import pytest
 
 import elasticapm
-from elasticapm.instrumentation.packages.base import (AbstractInstrumentedModule,
-                                                      OriginalNamesBoundFunctionWrapper)
-from elasticapm.utils import compat
+from elasticapm.instrumentation.packages.base import AbstractInstrumentedModule
+from elasticapm.utils import compat, wrapt
 
 
 class Dummy(object):
-    def dummy(self):
-        pass
+    def dummy(self, call_args=None):
+        return call_args
 
 
 class _TestInstrumentNonExistingFunctionOnModule(AbstractInstrumentedModule):
@@ -36,6 +35,12 @@ class _TestDummyInstrumentation(AbstractInstrumentedModule):
         ("tests.instrumentation.base_tests", "Dummy.dummy"),
     ]
 
+    def call(self, module, method, wrapped, instance, args, kwargs):
+        kwargs = kwargs or {}
+        kwargs['call_args'] = (module, method)
+        return wrapped(*args, **kwargs)
+
+
 
 def test_instrument_nonexisting_method_on_module():
     _TestInstrumentNonExistingFunctionOnModule().instrument()
@@ -48,31 +53,47 @@ def test_instrument_nonexisting_method():
 @pytest.mark.skipif(compat.PY3, reason="different object model")
 def test_uninstrument_py2():
     assert isinstance(Dummy.dummy, types.MethodType)
-    assert not isinstance(Dummy.dummy, OriginalNamesBoundFunctionWrapper)
+    assert not isinstance(Dummy.dummy, wrapt.BoundFunctionWrapper)
 
     instrumentation = _TestDummyInstrumentation()
     instrumentation.instrument()
-    assert isinstance(Dummy.dummy, OriginalNamesBoundFunctionWrapper)
+    assert isinstance(Dummy.dummy, wrapt.BoundFunctionWrapper)
 
     instrumentation.uninstrument()
     assert isinstance(Dummy.dummy, types.MethodType)
-    assert not isinstance(Dummy.dummy, OriginalNamesBoundFunctionWrapper)
+    assert not isinstance(Dummy.dummy, wrapt.BoundFunctionWrapper)
 
 
 @pytest.mark.skipif(compat.PY2, reason="different object model")
 def test_uninstrument_py3():
     original = Dummy.dummy
-    assert not isinstance(Dummy.dummy, OriginalNamesBoundFunctionWrapper)
+    assert not isinstance(Dummy.dummy, wrapt.BoundFunctionWrapper)
 
     instrumentation = _TestDummyInstrumentation()
     instrumentation.instrument()
 
     assert Dummy.dummy is not original
-    assert isinstance(Dummy.dummy, OriginalNamesBoundFunctionWrapper)
+    assert isinstance(Dummy.dummy, wrapt.BoundFunctionWrapper)
 
     instrumentation.uninstrument()
     assert Dummy.dummy is original
-    assert not isinstance(Dummy.dummy, OriginalNamesBoundFunctionWrapper)
+    assert not isinstance(Dummy.dummy, wrapt.BoundFunctionWrapper)
+
+
+def test_module_method_args(elasticapm_client):
+    """
+    Test that the module/method arguments are correctly passed to
+    the _TestDummyInstrumentation.call method
+    """
+    instrumentation = _TestDummyInstrumentation()
+    instrumentation.instrument()
+    elasticapm_client.begin_transaction('test')
+    dummy = Dummy()
+    call_args = dummy.dummy()
+    elasticapm_client.end_transaction('test', 'test')
+    instrumentation.uninstrument()
+
+    assert call_args == ('tests.instrumentation.base_tests', 'Dummy.dummy')
 
 
 def test_skip_instrument_env_var():