Repository: scoutapp/scout_apm_python
Pull request #366 (2019-11-06 09:41:57)

Tidy jinja2 and urllib3 import patterns
Modify the last two instruments to do import-time importing, rather than re-importing in several different places. This is the same as done for Redis in #341.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 81ea265..97c3868 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,8 @@
   ([PR #247](https://github.com/scoutapp/scout_apm_python/pull/247)).
 - Fix the `RemovedInDjango20Warning` for `django.core.urlresolvers` on Django
   1.11 ([PR #359](https://github.com/scoutapp/scout_apm_python/pull/359)).
+- Correctly disable Bottle if the agent fails to launch
+  ([PR #364](https://github.com/scoutapp/scout_apm_python/pull/364)).
 
 ## [2.7.0] 2019-11-03
 
diff --git a/src/scout_apm/bottle.py b/src/scout_apm/bottle.py
index ca16689..2b92462 100644
--- a/src/scout_apm/bottle.py
+++ b/src/scout_apm/bottle.py
@@ -30,10 +30,11 @@ class ScoutPlugin(object):
 
     def setup(self, app):
         self.set_config_from_bottle(app)
-        scout_apm.core.install()
+        installed = scout_apm.core.install()
+        self._do_nothing = not installed
 
     def apply(self, callback, context):
-        if not scout_config.value("monitor"):
+        if self._do_nothing:
             return callback
         return wrap_callback(callback)
 
diff --git a/src/scout_apm/instruments/jinja2.py b/src/scout_apm/instruments/jinja2.py
index 063572d..d2c50d4 100644
--- a/src/scout_apm/instruments/jinja2.py
+++ b/src/scout_apm/instruments/jinja2.py
@@ -7,6 +7,11 @@ import wrapt
 
 from scout_apm.core.tracked_request import TrackedRequest
 
+try:
+    from jinja2 import Template
+except ImportError:
+    Template = None
+
 logger = logging.getLogger(__name__)
 
 
@@ -14,9 +19,7 @@ class Instrument(object):
     installed = False
 
     def installable(self):
-        try:
-            from jinja2 import Template  # noqa: F401
-        except ImportError:
+        if Template is None:
             logger.info("Unable to import for Jinja2 instruments")
             return False
         if self.installed:
@@ -32,25 +35,13 @@ class Instrument(object):
         self.__class__.installed = True
 
         try:
-            from jinja2 import Template
-
-            @wrapt.decorator
-            def wrapped_render(wrapped, instance, args, kwargs):
-                tracked_request = TrackedRequest.instance()
-                span = tracked_request.start_span(operation="Template/Render")
-                span.tag("name", instance.name)
-                try:
-                    return wrapped(*args, **kwargs)
-                finally:
-                    tracked_request.stop_span()
-
             Template.render = wrapped_render(Template.render)
-
-            logger.info("Instrumented Jinja2")
-
-        except Exception as e:
-            logger.warning("Unable to instrument for Jinja2 Template.render: %r", e)
+        except Exception as exc:
+            logger.warning(
+                "Unable to instrument for Jinja2 Template.render: %r", exc, exc_info=exc
+            )
             return False
+        logger.info("Instrumented Jinja2")
         return True
 
     def uninstall(self):
@@ -60,6 +51,15 @@ class Instrument(object):
 
         self.__class__.installed = False
 
-        from jinja2 import Template
-
         Template.render = Template.render.__wrapped__
+
+
+@wrapt.decorator
+def wrapped_render(wrapped, instance, args, kwargs):
+    tracked_request = TrackedRequest.instance()
+    span = tracked_request.start_span(operation="Template/Render")
+    span.tag("name", instance.name)
+    try:
+        return wrapped(*args, **kwargs)
+    finally:
+        tracked_request.stop_span()
diff --git a/src/scout_apm/instruments/urllib3.py b/src/scout_apm/instruments/urllib3.py
index f2d4de1..ab21608 100644
--- a/src/scout_apm/instruments/urllib3.py
+++ b/src/scout_apm/instruments/urllib3.py
@@ -8,6 +8,11 @@ import wrapt
 from scout_apm.compat import text_type
 from scout_apm.core.tracked_request import TrackedRequest
 
+try:
+    from urllib3 import HTTPConnectionPool
+except ImportError:
+    HTTPConnectionPool = None
+
 logger = logging.getLogger(__name__)
 
 
@@ -15,9 +20,7 @@ class Instrument(object):
     installed = False
 
     def installable(self):
-        try:
-            from urllib3 import HTTPConnectionPool, PoolManager  # noqa: F401
-        except ImportError:
+        if HTTPConnectionPool is None:
             logger.info("Unable to import for Urllib3 instruments")
             return False
         if self.installed:
@@ -33,42 +36,16 @@ class Instrument(object):
         self.__class__.installed = True
 
         try:
-            from urllib3 import HTTPConnectionPool
-
-            @wrapt.decorator
-            def wrapped_urlopen(wrapped, instance, args, kwargs):
-                def _extract_method(method, *args, **kwargs):
-                    return method
-
-                try:
-                    method = _extract_method(*args, **kwargs)
-                except TypeError:
-                    method = "Unknown"
-
-                try:
-                    url = text_type(self._absolute_url("/"))
-                except Exception:
-                    logger.exception("Could not get URL for HTTPConnectionPool")
-                    url = "Unknown"
-
-                tracked_request = TrackedRequest.instance()
-                span = tracked_request.start_span(operation="HTTP/{}".format(method))
-                span.tag("url", text_type(url))
-
-                try:
-                    return wrapped(*args, **kwargs)
-                finally:
-                    tracked_request.stop_span()
-
             HTTPConnectionPool.urlopen = wrapped_urlopen(HTTPConnectionPool.urlopen)
-
-            logger.info("Instrumented Urllib3")
-
-        except Exception as e:
+        except Exception as exc:
             logger.warning(
-                "Unable to instrument for Urllib3 HTTPConnectionPool.urlopen: %r", e
+                "Unable to instrument for Urllib3 HTTPConnectionPool.urlopen: %r",
+                exc,
+                exc_info=exc,
             )
             return False
+
+        logger.info("Instrumented Urllib3")
         return True
 
     def uninstall(self):
@@ -78,6 +55,30 @@ class Instrument(object):
 
         self.__class__.installed = False
 
-        from urllib3 import HTTPConnectionPool
-
         HTTPConnectionPool.urlopen = HTTPConnectionPool.urlopen.__wrapped__
+
+
+@wrapt.decorator
+def wrapped_urlopen(wrapped, instance, args, kwargs):
+    def _extract_method(method, *args, **kwargs):
+        return method
+
+    try:
+        method = _extract_method(*args, **kwargs)
+    except TypeError:
+        method = "Unknown"
+
+    try:
+        url = text_type(instance._absolute_url("/"))
+    except Exception:
+        logger.exception("Could not get URL for HTTPConnectionPool")
+        url = "Unknown"
+
+    tracked_request = TrackedRequest.instance()
+    span = tracked_request.start_span(operation="HTTP/{}".format(method))
+    span.tag("url", text_type(url))
+
+    try:
+        return wrapped(*args, **kwargs)
+    finally:
+        tracked_request.stop_span()


Test patch:
diff --git a/tests/integration/instruments/test_elasticsearch.py b/tests/integration/instruments/test_elasticsearch.py
index c2b9616..93d98e7 100644
--- a/tests/integration/instruments/test_elasticsearch.py
+++ b/tests/integration/instruments/test_elasticsearch.py
@@ -170,10 +170,11 @@ def test_instrument_client_install_missing_attribute():
         "scout_apm.instruments.elasticsearch.Elasticsearch"
     ) as mock_elasticsearch:
         del mock_elasticsearch.bulk
-    try:
-        instrument.instrument_client()  # no crash
-    finally:
-        instrument.uninstrument_client()
+
+        try:
+            instrument.instrument_client()  # no crash
+        finally:
+            instrument.uninstrument_client()
 
 
 @mock.patch(
diff --git a/tests/integration/instruments/test_jinja2.py b/tests/integration/instruments/test_jinja2.py
index f71f038..424a6f4 100644
--- a/tests/integration/instruments/test_jinja2.py
+++ b/tests/integration/instruments/test_jinja2.py
@@ -7,7 +7,6 @@ import jinja2
 
 from scout_apm.instruments.jinja2 import Instrument
 from tests.compat import mock
-from tests.tools import pretend_package_unavailable
 
 instrument = Instrument()
 
@@ -46,23 +45,25 @@ def test_installable():
 
 
 def test_installable_no_jinja2_module():
-    with pretend_package_unavailable("jinja2"):
+    with mock.patch("scout_apm.instruments.jinja2.Template", new=None):
         assert not instrument.installable()
 
 
 def test_install_no_jinja2_module():
-    with pretend_package_unavailable("jinja2"):
+    with mock.patch("scout_apm.instruments.jinja2.Template", new=None):
         assert not instrument.install()
         assert not Instrument.installed
 
 
-@mock.patch("scout_apm.instruments.jinja2.wrapt.decorator", side_effect=RuntimeError)
-def test_install_failure(mock_decorator):
-    try:
-        assert not instrument.install()  # doesn't crash
-    finally:
-        # Currently installed = True even if installing failed.
-        Instrument.installed = False
+def test_install_failure_no_render_attribute():
+    with mock.patch("scout_apm.instruments.jinja2.Template") as mock_template:
+        del mock_template.render
+
+        try:
+            assert not instrument.install()  # doesn't crash
+        finally:
+            # Currently installed = True even if installing failed.
+            Instrument.installed = False
 
 
 def test_install_is_idempotent():
diff --git a/tests/integration/instruments/test_urllib3.py b/tests/integration/instruments/test_urllib3.py
index 1d0a432..c4d21fe 100644
--- a/tests/integration/instruments/test_urllib3.py
+++ b/tests/integration/instruments/test_urllib3.py
@@ -9,7 +9,6 @@ import urllib3
 
 from scout_apm.instruments.urllib3 import Instrument
 from tests.compat import mock
-from tests.tools import pretend_package_unavailable
 
 # e.g. export URLLIB3_URL="http://httpbin.org/"
 # or export URLLIB3_URL="http://localhost:9200/" (re-use Elasticsearch!)
@@ -66,23 +65,26 @@ def test_installable():
 
 
 def test_installable_no_urllib3_module():
-    with pretend_package_unavailable("urllib3"):
+    with mock.patch("scout_apm.instruments.urllib3.HTTPConnectionPool", new=None):
         assert not instrument.installable()
 
 
 def test_install_no_urllib3_module():
-    with pretend_package_unavailable("urllib3"):
+    with mock.patch("scout_apm.instruments.urllib3.HTTPConnectionPool", new=None):
         assert not instrument.install()
         assert not Instrument.installed
 
 
-@mock.patch("scout_apm.instruments.urllib3.wrapt.decorator", side_effect=RuntimeError)
-def test_install_failure(mock_decorator):
-    try:
-        assert not instrument.install()  # doesn't crash
-    finally:
-        # Currently installed = True even if installing failed.
-        Instrument.installed = False
+def test_install_failure_no_urlopen_attribute():
+    with mock.patch(
+        "scout_apm.instruments.urllib3.HTTPConnectionPool"
+    ) as mock_http_connection_pool:
+        del mock_http_connection_pool.urlopen
+        try:
+            assert not instrument.install()  # doesn't crash
+        finally:
+            # Currently installed = True even if installing failed.
+            Instrument.installed = False
 
 
 def test_install_is_idempotent():