Repository: scoutapp/scout_apm_python
Pull request #189 (2019-06-21 15:56:12)

Move all instrument installed tracking to class level
I saw some test failures like this while writing #188:

```
/tmp/tox/py36-django111/lib/python3.6/site-packages/scout_apm/instruments/elasticsearch.py:179: in perform_request
    return original(*args, **kwargs)
/tmp/tox/py36-django111/lib/python3.6/site-packages/scout_apm/core/monkey.py:199: in __call__
    self.__subject__, self._eop_instance_, *args, **kwargs
/tmp/tox/py36-django111/lib/python3.6/site-packages/scout_apm/instruments/elasticsearch.py:179: in perform_request
    return original(*args, **kwargs)
/tmp/tox/py36-django111/lib/python3.6/site-packages/scout_apm/core/monkey.py:199: in __call__
    self.__subject__, self._eop_instance_, *args, **kwargs
/tmp/tox/py36-django111/lib/python3.6/site-packages/scout_apm/instruments/elasticsearch.py:179: in perform_request
    return original(*args, **kwargs)
/tmp/tox/py36-django111/lib/python3.6/site-packages/elasticsearch/transport.py:353: in perform_request
    timeout=timeout,
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <Urllib3HttpConnection: http://localhost:9200>, method = 'GET'
url = '/_all/_search', params = {}, body = None, timeout = None, ignore = ()
headers = None
    def perform_request(
        self, method, url, params=None, body=None, timeout=None, ignore=(), headers=None
    ):
...
>           raise ConnectionError("N/A", str(e), e)
E           elasticsearch.exceptions.ConnectionError: ConnectionError(maximum recursion depth exceeded) caused by: RecursionError(maximum recursion depth exceeded)
```

The elasticsearch instrument was not fully idempotent - it tried to be, but because it used an instance variable, the "installed" status was forgotten between runs of `scout_apm.core.install()`. Thus in tests it got installed many times, and I think the number of tests on that PR simply hit a point where the `RecursionError` became possible.

Fix this by making *every* instrument install *fully* idempotent with class level variables, similar to the fix for the Django ones in #171.


Patch:
diff --git a/src/scout_apm/instruments/elasticsearch.py b/src/scout_apm/instruments/elasticsearch.py
index f4c63e5..82f215a 100644
--- a/src/scout_apm/instruments/elasticsearch.py
+++ b/src/scout_apm/instruments/elasticsearch.py
@@ -3,7 +3,6 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 
 import logging
 
-# Used in the exec() call below.
 from scout_apm.core.monkey import monkeypatch_method, unpatch_method
 from scout_apm.core.tracked_request import TrackedRequest
 
@@ -11,6 +10,8 @@ logger = logging.getLogger(__name__)
 
 
 class Instrument(object):
+    installed = False
+
     CLIENT_METHODS = [
         "bulk",
         "count",
@@ -38,9 +39,6 @@ class Instrument(object):
         "update_by_query",
     ]
 
-    def __init__(self):
-        self.installed = False
-
     def installable(self):
         try:
             from elasticsearch import Elasticsearch  # noqa: F401
@@ -58,7 +56,7 @@ class Instrument(object):
             logger.info("Elasticsearch instruments are not installable. Skipping.")
             return False
 
-        self.installed = True
+        self.__class__.installed = True
 
         self.instrument_client()
         self.instrument_transport()
@@ -68,7 +66,7 @@ class Instrument(object):
             logger.info("Elasticsearch instruments are not installed. Skipping.")
             return False
 
-        self.installed = False
+        self.__class__.installed = False
 
         self.uninstrument_client()
         self.uninstrument_transport()
diff --git a/src/scout_apm/instruments/jinja2.py b/src/scout_apm/instruments/jinja2.py
index 38e2d54..e1d4ef4 100644
--- a/src/scout_apm/instruments/jinja2.py
+++ b/src/scout_apm/instruments/jinja2.py
@@ -10,8 +10,7 @@ logger = logging.getLogger(__name__)
 
 
 class Instrument(object):
-    def __init__(self):
-        self.installed = False
+    installed = False
 
     def installable(self):
         try:
@@ -29,7 +28,7 @@ class Instrument(object):
             logger.info("Jinja2 instruments are not installable. Skipping.")
             return False
 
-        self.installed = True
+        self.__class__.installed = True
 
         try:
             from jinja2 import Template
@@ -57,7 +56,7 @@ class Instrument(object):
             logger.info("Jinja2 instruments are not installed. Skipping.")
             return False
 
-        self.installed = False
+        self.__class__.installed = False
 
         from jinja2 import Template
 
diff --git a/src/scout_apm/instruments/pymongo.py b/src/scout_apm/instruments/pymongo.py
index 0160c49..4e496b2 100644
--- a/src/scout_apm/instruments/pymongo.py
+++ b/src/scout_apm/instruments/pymongo.py
@@ -11,6 +11,8 @@ logger = logging.getLogger(__name__)
 
 
 class Instrument(object):
+    installed = False
+
     PYMONGO_METHODS = [
         "aggregate",
         "bulk_write",
@@ -45,9 +47,6 @@ class Instrument(object):
         "update_one",
     ]
 
-    def __init__(self):
-        self.installed = False
-
     def installable(self):
         try:
             from pymongo.collection import Collection  # noqa: F401
@@ -64,7 +63,7 @@ class Instrument(object):
             logger.info("PyMongo instruments are not installable. Skipping.")
             return False
 
-        self.installed = True
+        self.__class__.installed = True
 
         try:
             from pymongo.collection import Collection  # noqa: F401
@@ -109,7 +108,7 @@ def {method_str}(original, self, *args, **kwargs):
             logger.info("PyMongo instruments are not installed. Skipping.")
             return False
 
-        self.installed = False
+        self.__class__.installed = False
 
         from pymongo.collection import Collection
 
diff --git a/src/scout_apm/instruments/redis.py b/src/scout_apm/instruments/redis.py
index e2ef0bc..76902ce 100644
--- a/src/scout_apm/instruments/redis.py
+++ b/src/scout_apm/instruments/redis.py
@@ -23,8 +23,7 @@ def import_Redis_and_Pipeline():
 
 
 class Instrument(object):
-    def __init__(self):
-        self.installed = False
+    installed = False
 
     def installable(self):
         try:
@@ -42,7 +41,7 @@ class Instrument(object):
             logger.info("Redis instruments are not installable. Skipping.")
             return False
 
-        self.installed = True
+        self.__class__.installed = True
 
         self.patch_redis()
         self.patch_pipeline()
@@ -55,7 +54,7 @@ class Instrument(object):
             logger.info("Redis instruments are not installed. Skipping.")
             return False
 
-        self.installed = False
+        self.__class__.installed = False
 
         self.unpatch_redis()
         self.unpatch_pipeline()
diff --git a/src/scout_apm/instruments/urllib3.py b/src/scout_apm/instruments/urllib3.py
index 7645d6f..bf3392a 100644
--- a/src/scout_apm/instruments/urllib3.py
+++ b/src/scout_apm/instruments/urllib3.py
@@ -10,8 +10,7 @@ logger = logging.getLogger(__name__)
 
 
 class Instrument(object):
-    def __init__(self):
-        self.installed = False
+    installed = False
 
     def installable(self):
         try:
@@ -29,7 +28,7 @@ class Instrument(object):
             logger.info("Urllib3 instruments are not installable. Skipping.")
             return False
 
-        self.installed = True
+        self.__class__.installed = True
 
         try:
             from urllib3 import HTTPConnectionPool
@@ -72,7 +71,7 @@ class Instrument(object):
             logger.info("Urllib3 instruments are not installed. Skipping.")
             return False
 
-        self.installed = False
+        self.__class__.installed = False
 
         from urllib3 import HTTPConnectionPool
 


Test patch:
diff --git a/tests/integration/instruments/test_elasticsearch.py b/tests/integration/instruments/test_elasticsearch.py
index a7e8d9b..1dcf37e 100644
--- a/tests/integration/instruments/test_elasticsearch.py
+++ b/tests/integration/instruments/test_elasticsearch.py
@@ -81,10 +81,10 @@ def test_perform_request_unknown_url():
 
 
 def test_installed():
-    assert not instrument.installed
+    assert not Instrument.installed
     with es_with_scout():
-        assert instrument.installed
-    assert not instrument.installed
+        assert Instrument.installed
+    assert not Instrument.installed
 
 
 def test_installable():
@@ -102,7 +102,7 @@ def test_installable_no_elasticsearch_module():
 def test_install_no_elasticsearch_module():
     with no_elasticsearch():
         assert not instrument.install()
-        assert not instrument.installed
+        assert not Instrument.installed
 
 
 def test_instrument_client_no_elasticsearch_module():
@@ -131,10 +131,12 @@ def test_instrument_transport_install_failure(monkeypatch_method):
 
 def test_install_is_idempotent():
     with es_with_scout():
-        assert instrument.installed
+        assert Instrument.installed
         instrument.install()  # does nothing, doesn't crash
+        assert Instrument.installed
 
 
 def test_uninstall_is_idempotent():
-    assert not instrument.installed
+    assert not Instrument.installed
     instrument.uninstall()  # does nothing, doesn't crash
+    assert not Instrument.installed
diff --git a/tests/integration/instruments/test_jinja2.py b/tests/integration/instruments/test_jinja2.py
index 4002c66..b14e7e3 100644
--- a/tests/integration/instruments/test_jinja2.py
+++ b/tests/integration/instruments/test_jinja2.py
@@ -41,10 +41,10 @@ def test_render():
 
 
 def test_installed():
-    assert not instrument.installed
+    assert not Instrument.installed
     with jinja2_with_scout():
-        assert instrument.installed
-    assert not instrument.installed
+        assert Instrument.installed
+    assert not Instrument.installed
 
 
 def test_installable():
@@ -62,7 +62,7 @@ def test_installable_no_jinja2_module():
 def test_install_no_jinja2_module():
     with no_jinja2():
         assert not instrument.install()
-        assert not instrument.installed
+        assert not Instrument.installed
 
 
 @mock.patch("scout_apm.instruments.jinja2.monkeypatch_method", side_effect=RuntimeError)
@@ -71,15 +71,17 @@ def test_install_failure(monkeypatch_method):
         assert not instrument.install()  # doesn't crash
     finally:
         # Currently installed = True even if installing failed.
-        instrument.installed = False
+        Instrument.installed = False
 
 
 def test_install_is_idempotent():
     with jinja2_with_scout():
-        assert instrument.installed
+        assert Instrument.installed
         instrument.install()  # does nothing, doesn't crash
+        assert Instrument.installed
 
 
 def test_uninstall_is_idempotent():
-    assert not instrument.installed
+    assert not Instrument.installed
     instrument.uninstall()  # does nothing, doesn't crash
+    assert not Instrument.installed
diff --git a/tests/integration/instruments/test_pymongo.py b/tests/integration/instruments/test_pymongo.py
index cff6972..7abc8b5 100644
--- a/tests/integration/instruments/test_pymongo.py
+++ b/tests/integration/instruments/test_pymongo.py
@@ -49,10 +49,10 @@ def test_find_one():
 
 
 def test_installed():
-    assert not instrument.installed
+    assert not Instrument.installed
     with client_with_scout():
-        assert instrument.installed
-    assert not instrument.installed
+        assert Instrument.installed
+    assert not Instrument.installed
 
 
 def test_installable():
@@ -70,7 +70,7 @@ def test_installable_no_pymongo_module():
 def test_install_no_pymongo_module():
     with no_pymongo():
         assert not instrument.install()
-        assert not instrument.installed
+        assert not Instrument.installed
 
 
 @mock.patch(
@@ -81,15 +81,17 @@ def test_install_failure(monkeypatch_method):
         assert not instrument.install()  # doesn't crash
     finally:
         # Currently installed = True even if installing failed.
-        instrument.installed = False
+        Instrument.installed = False
 
 
 def test_install_is_idempotent():
     with client_with_scout():
-        assert instrument.installed
+        assert Instrument.installed
         instrument.install()  # does nothing, doesn't crash
+        assert Instrument.installed
 
 
 def test_uninstall_is_idempotent():
-    assert not instrument.installed
+    assert not Instrument.installed
     instrument.uninstall()  # does nothing, doesn't crash
+    assert not Instrument.installed
diff --git a/tests/integration/instruments/test_redis.py b/tests/integration/instruments/test_redis.py
index f0e0fa0..8d870d9 100644
--- a/tests/integration/instruments/test_redis.py
+++ b/tests/integration/instruments/test_redis.py
@@ -76,10 +76,10 @@ def test_perform_request_bad_url():
 
 
 def test_installed():
-    assert not instrument.installed
+    assert not Instrument.installed
     with redis_with_scout():
-        assert instrument.installed
-    assert not instrument.installed
+        assert Instrument.installed
+    assert not Instrument.installed
 
 
 def test_installable():
@@ -97,7 +97,7 @@ def test_installable_no_redis_module():
 def test_install_no_redis_module():
     with no_redis():
         assert not instrument.install()
-        assert not instrument.installed
+        assert not Instrument.installed
 
 
 def test_patch_redis_no_redis_module():
@@ -122,10 +122,12 @@ def test_patch_pipeline_install_failure(monkeypatch_method):
 
 def test_install_is_idempotent():
     with redis_with_scout():
-        assert instrument.installed
+        assert Instrument.installed
         instrument.install()  # does nothing, doesn't crash
+        assert Instrument.installed
 
 
 def test_uninstall_is_idempotent():
-    assert not instrument.installed
+    assert not Instrument.installed
     instrument.uninstall()  # does nothing, doesn't crash
+    assert not Instrument.installed
diff --git a/tests/integration/instruments/test_urllib3.py b/tests/integration/instruments/test_urllib3.py
index 3453708..41ef169 100644
--- a/tests/integration/instruments/test_urllib3.py
+++ b/tests/integration/instruments/test_urllib3.py
@@ -61,10 +61,10 @@ def test_urlopen_exception(_absolute_url):
 
 
 def test_installed():
-    assert not instrument.installed
+    assert not Instrument.installed
     with urllib3_with_scout():
-        assert instrument.installed
-    assert not instrument.installed
+        assert Instrument.installed
+    assert not Instrument.installed
 
 
 def test_installable():
@@ -82,7 +82,7 @@ def test_installable_no_urllib3_module():
 def test_install_no_urllib3_module():
     with no_urllib3():
         assert not instrument.install()
-        assert not instrument.installed
+        assert not Instrument.installed
 
 
 @mock.patch(
@@ -93,15 +93,17 @@ def test_install_failure(monkeypatch_method):
         assert not instrument.install()  # doesn't crash
     finally:
         # Currently installed = True even if installing failed.
-        instrument.installed = False
+        Instrument.installed = False
 
 
 def test_install_is_idempotent():
     with urllib3_with_scout():
-        assert instrument.installed
+        assert Instrument.installed
         instrument.install()  # does nothing, doesn't crash
+        assert Instrument.installed
 
 
 def test_uninstall_is_idempotent():
-    assert not instrument.installed
+    assert not Instrument.installed
     instrument.uninstall()  # does nothing, doesn't crash
+    assert not Instrument.installed