Repository: scoutapp/scout_apm_python
Pull request #481 (2020-02-17 16:38:05)

Convert shutdown_timeout_seconds to float
Fix new value from #474 being set as an environment variable, which will only be a string.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1799677..fd0f653 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,12 @@
   This ensures that all traces are recorded, rather than lost, especially
   useful for single-run background commands.
   ([Issue # 473](https://github.com/scoutapp/scout_apm_python/issues/473)).
+- Move from **requests** to underlying **urllib3** for downloads to reduce
+  dependency footprint
+  ([PR #480](https://github.com/scoutapp/scout_apm_python/pull/480)).
+- Retry downloading the core agent a few times, and use a timeout to prevent
+  startup hangs
+  ([PR #480](https://github.com/scoutapp/scout_apm_python/pull/480)).
 
 ### Fixed
 
diff --git a/setup.py b/setup.py
index df5892c..d6eeaf8 100644
--- a/setup.py
+++ b/setup.py
@@ -59,7 +59,8 @@ setup(
         'asgiref ; python_version >= "3.5"',
         'importlib-metadata ; python_version < "3.8"',
         "psutil>=5,<6",
-        "requests>=2,<3",
+        'urllib3[secure] < 1.25 ; python_version < "3.5"',
+        'urllib3[secure] < 2 ; python_version >= "3.5"',
         "wrapt>=1.10,<2.0",
     ],
     keywords="apm performance monitoring development",
diff --git a/src/scout_apm/compat.py b/src/scout_apm/compat.py
index ad7a83d..6327c12 100644
--- a/src/scout_apm/compat.py
+++ b/src/scout_apm/compat.py
@@ -6,6 +6,9 @@ import inspect
 import sys
 from functools import wraps
 
+import certifi
+import urllib3
+
 string_type = str if sys.version_info[0] >= 3 else basestring  # noqa: F821
 text_type = str if sys.version_info[0] >= 3 else unicode  # noqa: F821
 string_types = tuple({string_type, text_type})
@@ -119,6 +122,14 @@ def kwargs_only(func):
     return wrapper
 
 
+def urllib3_cert_pool_manager(**kwargs):
+    if sys.version_info >= (3, 0):
+        CERT_REQUIRED = "CERT_REQUIRED"
+    else:
+        CERT_REQUIRED = b"CERT_REQUIRED"
+    return urllib3.PoolManager(cert_reqs=CERT_REQUIRED, ca_certs=certifi.where())
+
+
 __all__ = [
     "ContextDecorator",
     "datetime_to_timestamp",
diff --git a/src/scout_apm/core/config.py b/src/scout_apm/core/config.py
index 1e3832c..a2f73a2 100644
--- a/src/scout_apm/core/config.py
+++ b/src/scout_apm/core/config.py
@@ -221,7 +221,7 @@ class Defaults(object):
             "name": "",
             "revision_sha": self._git_revision_sha(),
             "scm_subdirectory": "",
-            "shutdown_timeout_seconds": 2,
+            "shutdown_timeout_seconds": 2.0,
             "uri_reporting": "filtered_params",
         }
 
@@ -260,6 +260,13 @@ def convert_to_bool(value):
     return False
 
 
+def convert_to_float(value):
+    try:
+        return float(value)
+    except ValueError:
+        return 0.0
+
+
 def convert_to_list(value):
     if isinstance(value, list):
         return value
@@ -275,9 +282,10 @@ def convert_to_list(value):
 CONVERSIONS = {
     "core_agent_download": convert_to_bool,
     "core_agent_launch": convert_to_bool,
-    "monitor": convert_to_bool,
     "disabled_instruments": convert_to_list,
     "ignore": convert_to_list,
+    "monitor": convert_to_bool,
+    "shutdown_timeout_seconds": convert_to_float,
 }
 
 
diff --git a/src/scout_apm/core/core_agent_manager.py b/src/scout_apm/core/core_agent_manager.py
index f213906..df0eb50 100644
--- a/src/scout_apm/core/core_agent_manager.py
+++ b/src/scout_apm/core/core_agent_manager.py
@@ -10,8 +10,9 @@ import tarfile
 import time
 import warnings
 
-import requests
+from urllib3.exceptions import HTTPError
 
+from scout_apm.compat import urllib3_cert_pool_manager
 from scout_apm.core.config import scout_config
 
 logger = logging.getLogger(__name__)
@@ -152,9 +153,10 @@ class CoreAgentDownloader(object):
         self.obtain_download_lock()
         if self.download_lock_fd is not None:
             try:
-                self.download_package()
-                self.untar()
-            except OSError:
+                downloaded = self.download_package()
+                if downloaded:
+                    self.untar()
+            except (OSError, HTTPError):
                 logger.exception("Exception raised while downloading Core Agent")
             finally:
                 self.release_download_lock()
@@ -195,11 +197,21 @@ class CoreAgentDownloader(object):
             os.close(self.download_lock_fd)
 
     def download_package(self):
-        logger.debug("Downloading: %s to %s", self.full_url(), self.package_location)
-        req = requests.get(self.full_url(), stream=True)
-        with open(self.package_location, "wb") as f:
-            for chunk in req.iter_content(1024 * 1000):
-                f.write(chunk)
+        full_url = self.full_url()
+        logger.debug("Downloading: %s to %s", full_url, self.package_location)
+        http = urllib3_cert_pool_manager()
+        response = http.request(
+            "GET", full_url, preload_content=False, timeout=10.0, retries=3
+        )
+        try:
+            if response.status != 200:
+                return False
+            with open(self.package_location, "wb") as fp:
+                for chunk in response.stream():
+                    fp.write(chunk)
+        finally:
+            response.release_conn()
+        return True
 
     def untar(self):
         t = tarfile.open(self.package_location, "r")
diff --git a/tox.ini b/tox.ini
index 3774d5c..18dc158 100644
--- a/tox.ini
+++ b/tox.ini
@@ -70,7 +70,6 @@ deps =
     six
     sqlalchemy
     starlette ; python_version >= "3.6"
-    urllib3[secure]
     webtest
 commands =
     pytest {posargs}


Test patch:
diff --git a/pytest.ini b/pytest.ini
index c949d22..059443e 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -52,3 +52,5 @@ filterwarnings =
     ; Appears in both gettext and Django:
     ignore:parameter codeset is deprecated:DeprecationWarning
     ignore:"errors" is deprecated. Use "encoding_errors" instead:DeprecationWarning:redis.client
+    ; Triggered on Python 2.7 only:
+    ignore:unicode for buf is no longer accepted, use bytes:DeprecationWarning:urllib3.contrib.pyopenssl
diff --git a/tests/integration/instruments/test_urllib3.py b/tests/integration/instruments/test_urllib3.py
index 8a07e53..8e76582 100644
--- a/tests/integration/instruments/test_urllib3.py
+++ b/tests/integration/instruments/test_urllib3.py
@@ -2,13 +2,12 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
 import logging
-import sys
 
-import certifi
 import httpretty
 import pytest
 import urllib3
 
+from scout_apm.compat import urllib3_cert_pool_manager
 from scout_apm.instruments.urllib3 import ensure_installed
 from tests.compat import mock
 from tests.tools import delete_attributes
@@ -17,11 +16,6 @@ mock_not_attempted = mock.patch(
     "scout_apm.instruments.urllib3.have_patched_pool_urlopen", new=False
 )
 
-if sys.version_info >= (3, 0):
-    CERT_REQUIRED = "CERT_REQUIRED"
-else:
-    CERT_REQUIRED = b"CERT_REQUIRED"
-
 
 def test_ensure_installed_twice(caplog):
     ensure_installed()
@@ -86,7 +80,7 @@ def test_request(tracked_request):
             httpretty.GET, "https://example.com/", body="Hello World!"
         )
 
-        http = urllib3.PoolManager(cert_reqs=CERT_REQUIRED, ca_certs=certifi.where())
+        http = urllib3_cert_pool_manager()
         response = http.request("GET", "https://example.com")
 
     assert response.status == 200
@@ -100,7 +94,7 @@ def test_request(tracked_request):
 def test_request_type_error(tracked_request):
     ensure_installed()
     with pytest.raises(TypeError):
-        http = urllib3.PoolManager(cert_reqs=CERT_REQUIRED, ca_certs=certifi.where())
+        http = urllib3_cert_pool_manager()
         connection = http.connection_from_host("example.com", scheme="https")
         connection.urlopen()
 
@@ -118,7 +112,7 @@ def test_request_no_absolute_url(caplog, tracked_request):
             httpretty.GET, "https://example.com/", body="Hello World!"
         )
 
-        http = urllib3.PoolManager(cert_reqs=CERT_REQUIRED, ca_certs=certifi.where())
+        http = urllib3_cert_pool_manager()
         response = http.request("GET", "https://example.com")
 
     assert response.status == 200
diff --git a/tests/unit/core/test_config.py b/tests/unit/core/test_config.py
index ca4bae6..68a59ad 100644
--- a/tests/unit/core/test_config.py
+++ b/tests/unit/core/test_config.py
@@ -186,6 +186,26 @@ def test_boolean_conversion_from_python(original, converted):
         ScoutConfig.reset_all()
 
 
+def test_float_conversion_from_env():
+    config = ScoutConfig()
+    with mock.patch.dict(os.environ, {"SCOUT_SHUTDOWN_TIMEOUT_SECONDS": "0"}):
+        value = config.value("shutdown_timeout_seconds")
+    assert isinstance(value, float)
+    assert value == 0.0
+
+
+@pytest.mark.parametrize(
+    "original, converted", [("0", 0.0), ("2", 2.0), ("x", 0.0)],
+)
+def test_float_conversion_from_python(original, converted):
+    ScoutConfig.set(shutdown_timeout_seconds=original)
+    config = ScoutConfig()
+    try:
+        assert config.value("shutdown_timeout_seconds") == converted
+    finally:
+        ScoutConfig.reset_all()
+
+
 def test_list_conversion_from_env():
     config = ScoutConfig()
     with mock.patch.dict(os.environ, {"SCOUT_DISABLED_INSTRUMENTS": "pymongo, redis"}):