Repository: scoutapp/scout_apm_python
Pull request #224 (2019-08-10 08:33:06)

Flatten PlatformDetection class methods into module
Unnecessary class methods moved to module functions, as per Aymeric's list. Also add caching on `get_libc()` since it uses a subprocess and might be called in cases where the core agent is started more than once, and add `close_fds=True` to the call to `ldd` like #219 did for launching the core agent, just in case.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d8fe91a..f451b86 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,8 @@
   ([PR #223](https://github.com/scoutapp/scout_apm_python/pull/223)).
 - Track Celery task `is_eager`, `exchange`, `routing_key` and `queue` tags
   ([PR #205](https://github.com/scoutapp/scout_apm_python/pull/205)).
+- Track Celery task time in queue with context tag `queue_time`
+  ([PR #206](https://github.com/scoutapp/scout_apm_python/pull/206)).
 
 ## [2.3.0] 2019-08-04
 
diff --git a/src/scout_apm/celery.py b/src/scout_apm/celery.py
index ca2100a..9b6aa02 100644
--- a/src/scout_apm/celery.py
+++ b/src/scout_apm/celery.py
@@ -1,16 +1,34 @@
 # coding=utf-8
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-from celery.signals import task_postrun, task_prerun
+import datetime as dt
+
+from celery.signals import before_task_publish, task_postrun, task_prerun
 
 import scout_apm.core
+from scout_apm.compat import datetime_to_timestamp
 from scout_apm.core.tracked_request import TrackedRequest
 
 
+def before_publish_callback(headers=None, properties=None, **kwargs):
+    if "scout_task_start" not in headers:
+        headers["scout_task_start"] = datetime_to_timestamp(dt.datetime.utcnow())
+
+
 def prerun_callback(task=None, **kwargs):
     tracked_request = TrackedRequest.instance()
     tracked_request.mark_real_request()
 
+    start = getattr(task.request, "scout_task_start", None)
+    if start is not None:
+        now = datetime_to_timestamp(dt.datetime.utcnow())
+        try:
+            queue_time = now - start
+        except TypeError:
+            pass
+        else:
+            tracked_request.tag("queue_time", queue_time)
+
     delivery_info = task.request.delivery_info
     tracked_request.tag("is_eager", delivery_info.get("is_eager", False))
     tracked_request.tag("exchange", delivery_info.get("exchange", "unknown"))
@@ -30,10 +48,12 @@ def install():
     if not installed:
         return
 
+    before_task_publish.connect(before_publish_callback)
     task_prerun.connect(prerun_callback)
     task_postrun.connect(postrun_callback)
 
 
 def uninstall():
+    before_task_publish.disconnect(before_publish_callback)
     task_prerun.disconnect(prerun_callback)
     task_postrun.disconnect(postrun_callback)
diff --git a/src/scout_apm/core/config.py b/src/scout_apm/core/config.py
index f45591c..d957458 100644
--- a/src/scout_apm/core/config.py
+++ b/src/scout_apm/core/config.py
@@ -5,7 +5,7 @@ import logging
 import os
 
 from scout_apm.compat import string_type
-from scout_apm.core.platform_detection import PlatformDetection
+from scout_apm.core import platform_detection
 from scout_apm.core.util import octal
 
 logger = logging.getLogger(__name__)
@@ -32,11 +32,8 @@ class ScoutConfig(object):
     def value(self, key):
         value = self.locate_layer_for_key(key).value(key)
         if key in CONVERSIONS:
-            converted_value = CONVERSIONS[key].convert(value)
-        else:
-            converted_value = value
-
-        return converted_value
+            return CONVERSIONS[key](value)
+        return value
 
     def locate_layer_for_key(self, key):
         for layer in self.layers:
@@ -202,7 +199,7 @@ class ScoutConfigDerived(object):
         )
 
     def derive_core_agent_triple(self):
-        return PlatformDetection.get_triple()
+        return platform_detection.get_triple()
 
 
 class ScoutConfigDefaults(object):
@@ -265,35 +262,31 @@ class ScoutConfigNull(object):
         return None
 
 
-class BooleanConversion(object):
-    @classmethod
-    def convert(cls, value):
-        if isinstance(value, bool):
-            return value
-        if isinstance(value, string_type):
-            return value.lower() in ("yes", "true", "t", "1")
-        # Unknown type - default to false?
-        return False
+def convert_to_bool(value):
+    if isinstance(value, bool):
+        return value
+    if isinstance(value, string_type):
+        return value.lower() in ("yes", "true", "t", "1")
+    # Unknown type - default to false?
+    return False
 
 
-class ListConversion(object):
-    @classmethod
-    def convert(cls, value):
-        if isinstance(value, list):
-            return value
-        if isinstance(value, tuple):
-            return list(value)
-        if isinstance(value, string_type):
-            # Split on commas
-            return [item.strip() for item in value.split(",") if item]
-        # Unknown type - default to empty?
-        return []
+def convert_to_list(value):
+    if isinstance(value, list):
+        return value
+    if isinstance(value, tuple):
+        return list(value)
+    if isinstance(value, string_type):
+        # Split on commas
+        return [item.strip() for item in value.split(",") if item]
+    # Unknown type - default to empty?
+    return []
 
 
 CONVERSIONS = {
-    "core_agent_download": BooleanConversion,
-    "core_agent_launch": BooleanConversion,
-    "monitor": BooleanConversion,
-    "disabled_instruments": ListConversion,
-    "ignore": ListConversion,
+    "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,
 }
diff --git a/src/scout_apm/core/platform_detection.py b/src/scout_apm/core/platform_detection.py
index 94b40a8..56a3b46 100644
--- a/src/scout_apm/core/platform_detection.py
+++ b/src/scout_apm/core/platform_detection.py
@@ -5,59 +5,58 @@ import platform
 import subprocess
 
 
-class PlatformDetection(object):
+def get_triple():
+    return "{arch}-{platform}".format(arch=get_arch(), platform=get_platform())
+
+
+def get_arch():
     """
-    This helps figuring out what platform we're running on, so we can download
-    the correct binary build of Core Agent.
+    What CPU are we on?
     """
+    arch = platform.machine()
+    if arch == "i686":
+        return "i686"
+    elif arch == "x86_64":
+        return "x86_64"
+    else:
+        return "unknown"
 
-    @classmethod
-    def get_triple(cls):
-        return "{arch}-{platform}".format(arch=cls.arch(), platform=cls.platform())
-
-    @classmethod
-    def arch(cls):
-        """
-        What CPU are we on?
-        """
-        arch = platform.machine()
-        if arch == "i686":
-            return "i686"
-        elif arch == "x86_64":
-            return "x86_64"
-        else:
-            return "unknown"
-
-    @classmethod
-    def platform(cls):
-        """
-        What Operating System (and sub-system like glibc / musl)
-        """
-        system_name = platform.system()
-        if system_name == "Linux":
-            libc = cls.libc()
-            return "unknown-linux-{libc}".format(libc=libc)
-        elif system_name == "Darwin":
-            return "apple-darwin"
-        else:
-            return "unknown"
-
-    @classmethod
-    def libc(cls):
-        """
-        Alpine linux uses a non glibc version of the standard library, it uses
-        the stripped down musl instead. The core agent can be built against it,
-        but which one is running must be detected. Shelling out to `ldd`
-        appears to be the most reliable way to do this.
-        """
+
+def get_platform():
+    """
+    What Operating System (and sub-system like glibc / musl)
+    """
+    system_name = platform.system()
+    if system_name == "Linux":
+        libc = get_libc()
+        return "unknown-linux-{libc}".format(libc=libc)
+    elif system_name == "Darwin":
+        return "apple-darwin"
+    else:
+        return "unknown"
+
+
+_libc = None
+
+
+def get_libc():
+    """
+    Alpine linux uses a non glibc version of the standard library, it uses
+    the stripped down musl instead. The core agent can be built against it,
+    but which one is running must be detected. Shelling out to `ldd`
+    appears to be the most reliable way to do this.
+    """
+    global _libc
+    if _libc is None:
         try:
             output = subprocess.check_output(
-                ["ldd", "--version"], stderr=subprocess.STDOUT
+                ["ldd", "--version"], stderr=subprocess.STDOUT, close_fds=True
             )
         except (OSError, subprocess.CalledProcessError):
-            return "gnu"
+            _libc = "gnu"
         else:
             if b"musl" in output:
-                return "musl"
+                _libc = "musl"
             else:
-                return "gnu"
+                _libc = "gnu"
+    return _libc


Test patch:
diff --git a/tests/integration/test_celery.py b/tests/integration/test_celery.py
index d2f3128..8089e85 100644
--- a/tests/integration/test_celery.py
+++ b/tests/integration/test_celery.py
@@ -84,12 +84,34 @@ def test_hello_worker(celery_app, celery_worker, tracked_requests):
     assert tracked_request.tags["exchange"] == ""
     assert tracked_request.tags["routing_key"] == "celery"
     assert tracked_request.tags["queue"] == "unknown"
+    assert (
+        0.0 <= tracked_request.tags["queue_time"] < 60.0
+    )  # Assume test took <60 seconds
     assert tracked_request.active_spans == []
     assert len(tracked_request.complete_spans) == 1
     span = tracked_request.complete_spans[0]
     assert span.operation == "Job/tests.integration.test_celery.hello"
 
 
+@skip_unless_celery_4_plus
+def test_hello_worker_header_preset(celery_app, celery_worker, tracked_requests):
+    with app_with_scout(app=celery_app) as app:
+        result = (
+            app.tasks["tests.integration.test_celery.hello"]
+            .apply_async(headers={"scout_task_start": "an evil string"})
+            .get()
+        )
+
+    assert result == "Hello World!"
+    assert len(tracked_requests) == 1
+    tracked_request = tracked_requests[0]
+    assert tracked_request.active_spans == []
+    assert len(tracked_request.complete_spans) == 1
+    span = tracked_request.complete_spans[0]
+    assert span.operation == "Job/tests.integration.test_celery.hello"
+    assert "queue_time" not in span.tags
+
+
 def test_no_monitor(tracked_requests):
     # With an empty config, "monitor" defaults to False.
     with app_with_scout(config={}) as app:
diff --git a/tests/unit/core/test_config.py b/tests/unit/core/test_config.py
index ab5508c..1119303 100644
--- a/tests/unit/core/test_config.py
+++ b/tests/unit/core/test_config.py
@@ -142,12 +142,9 @@ def test_null_config_name():
 
 
 def test_boolean_conversion_from_env():
-    os.environ["SCOUT_MONITOR"] = "True"
     config = ScoutConfig()
-    try:
+    with mock.patch.dict(os.environ, {"SCOUT_MONITOR": "True"}):
         assert config.value("monitor") is True
-    finally:
-        del os.environ["SCOUT_MONITOR"]
 
 
 @pytest.mark.parametrize(
@@ -174,12 +171,9 @@ def test_boolean_conversion_from_python(original, converted):
 
 
 def test_list_conversion_from_env():
-    os.environ["SCOUT_DISABLED_INSTRUMENTS"] = "pymongo, redis"
     config = ScoutConfig()
-    try:
+    with mock.patch.dict(os.environ, {"SCOUT_DISABLED_INSTRUMENTS": "pymongo, redis"}):
         assert config.value("disabled_instruments") == ["pymongo", "redis"]
-    finally:
-        del os.environ["SCOUT_DISABLED_INSTRUMENTS"]
 
 
 @pytest.mark.parametrize(
diff --git a/tests/unit/core/test_platform_detection.py b/tests/unit/core/test_platform_detection.py
index a5bd83e..5f02097 100644
--- a/tests/unit/core/test_platform_detection.py
+++ b/tests/unit/core/test_platform_detection.py
@@ -4,12 +4,12 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 import pytest
 
 from scout_apm.compat import string_type
-from scout_apm.core.platform_detection import PlatformDetection
+from scout_apm.core import platform_detection
 from tests.compat import mock
 
 
 def test_get_triple():
-    assert isinstance(PlatformDetection.get_triple(), string_type)
+    assert isinstance(platform_detection.get_triple(), string_type)
 
 
 @pytest.mark.parametrize(
@@ -23,9 +23,9 @@ def test_get_triple():
     ],
 )
 @mock.patch("platform.machine")
-def test_arch(platform_machine, machine, arch):
+def test_get_arch(platform_machine, machine, arch):
     platform_machine.return_value = machine
-    assert PlatformDetection.arch() == arch
+    assert platform_detection.get_arch() == arch
 
 
 @pytest.mark.parametrize(
@@ -38,9 +38,9 @@ def test_arch(platform_machine, machine, arch):
     ],
 )
 @mock.patch("platform.system")
-def test_platform(platform_system, system, platform):
+def test_get_platform(platform_system, system, platform):
     platform_system.return_value = system
-    assert PlatformDetection.platform() == platform
+    assert platform_detection.get_platform() == platform
 
 
 @pytest.mark.parametrize(
@@ -52,12 +52,14 @@ def test_platform(platform_system, system, platform):
     ],
 )
 @mock.patch("subprocess.check_output")
-def test_libc(check_output, output, libc):
+def test_get_libc(check_output, output, libc):
+    platform_detection._libc = None  # reset cache
     check_output.return_value = output
-    assert PlatformDetection.libc() == libc
+    assert platform_detection.get_libc() == libc
 
 
 @mock.patch("subprocess.check_output")
-def test_libc_no_ldd(check_output):
+def test_get_libc_no_ldd(check_output):
+    platform_detection._libc = None  # reset cache
     check_output.side_effect = OSError
-    assert PlatformDetection.libc() == "gnu"
+    assert platform_detection.get_libc() == "gnu"