Repository: scoutapp/scout_apm_python (python)

Issue:
Default to TCP socket for core agent communication
Too many installs are having issues with file permissions for the core agent unix socket. The core agent has the ability to listen on a TCP socket. Change the python agent code to default to using only the TCP socket, but make sure we can still use the unix socket option too.

Change the default `core_agent_socket_path` to `tcp://127.0.0.1:6590`.

In the core agent socket code, when a path starts with `tcp://`, create the socket as `AF_INET` instead of `AF_UNIX`, and use this to create a tuple for connecting the socket to that ip/port and to pass when launching the core agent with the `--socket` option.

Ensure the socket behaves the same as with unix socket - e.g. reconnecting after unexpected disconnect, etc.

Pull request:
Connect to core agent over TCP by default
Fixes #553.

Because the `probe` and `shutdown` core-agent commands don't work with TCP mode, this moves the relevant test functions to instead check for the process, and terminate it for shutdown.

Patch:
diff --git a/.python-version b/.python-version
index 7b8d5ec..e446773 100644
--- a/.python-version
+++ b/.python-version
@@ -1,8 +1,8 @@
-3.8.3
-3.7.7
-3.6.10
+3.8.5
+3.7.8
+3.6.11
 3.5.9
 3.4.10
-2.7.17
+2.7.18
 pypy3.6-7.1.1
 pypy2.7-7.1.1
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 66a2e21..3f934b1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,15 @@
 
 ## Pending
 
+### Added
+
+- Changed default agent connection to use TCP rather than a unix socket. By
+  default the agent will be run on 127.0.0.1, port 6590. This should resolve
+  a number of connection issues customers have seen. To continue using a socket
+  connection, [configure
+  `core_agent_socket_path`](https://docs.scoutapm.com/#python-configuration).
+  ([Issue #553](https://github.com/scoutapp/scout_apm_python/issues/553))
+
 ### Fixed
 
 - Fixed validation check on length of key.
diff --git a/src/scout_apm/core/config.py b/src/scout_apm/core/config.py
index 24bcc2c..f378475 100644
--- a/src/scout_apm/core/config.py
+++ b/src/scout_apm/core/config.py
@@ -219,6 +219,7 @@ class Defaults(object):
             "core_agent_launch": True,
             "core_agent_log_level": "info",
             "core_agent_permissions": 700,
+            "core_agent_socket_path": "tcp://127.0.0.1:6590",
             "core_agent_version": "v1.2.10",  # can be an exact tag name, or 'latest'
             "disabled_instruments": [],
             "download_url": "https://s3-us-west-1.amazonaws.com/scout-public-downloads/apm_core_agent/release",  # noqa: E501
diff --git a/src/scout_apm/core/core_agent_manager.py b/src/scout_apm/core/core_agent_manager.py
index 6702be0..c139d1a 100644
--- a/src/scout_apm/core/core_agent_manager.py
+++ b/src/scout_apm/core/core_agent_manager.py
@@ -86,7 +86,11 @@ class CoreAgentManager(object):
         return ["--daemonize", "true"]
 
     def socket_path(self):
-        return ["--socket", get_socket_path()]
+        path = get_socket_path()
+        if path.is_tcp:
+            return ["--tcp", path.tcp_address]
+        else:
+            return ["--socket", path]
 
     def log_level(self):
         # Old deprecated name "log_level"
@@ -286,16 +290,19 @@ def sha256_digest(filename, block_size=65536):
         return None
 
 
+class SocketPath(text_type):
+    @property
+    def is_tcp(self):
+        return self.startswith("tcp://")
+
+    @property
+    def tcp_address(self):
+        return self[len("tcp://") :]
+
+
 def get_socket_path():
     # Old deprecated name "socket_path"
     socket_path = scout_config.value("socket_path")
-    # New name, but not set with any default so it can be derived
     if socket_path is None:
         socket_path = scout_config.value("core_agent_socket_path")
-    if socket_path is None:
-        # Derive it
-        socket_path = "{}/{}/scout-agent.sock".format(
-            scout_config.value("core_agent_dir"),
-            scout_config.value("core_agent_full_name"),
-        )
-    return socket_path
+    return SocketPath(socket_path)
diff --git a/src/scout_apm/core/socket.py b/src/scout_apm/core/socket.py
index 0d41f6d..08029b5 100644
--- a/src/scout_apm/core/socket.py
+++ b/src/scout_apm/core/socket.py
@@ -6,6 +6,7 @@ import logging
 import os
 import socket
 import struct
+import sys
 import threading
 import time
 
@@ -67,7 +68,7 @@ class CoreAgentSocketThread(SingletonThread):
 
     def run(self):
         self.socket_path = get_socket_path()
-        self.socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+        self.socket = self.make_socket()
 
         try:
             self._connect()
@@ -174,7 +175,7 @@ class CoreAgentSocketThread(SingletonThread):
                 threading.current_thread(),
             )
             try:
-                self.socket.connect(self.socket_path)
+                self.socket.connect(self.get_socket_address())
                 self.socket.settimeout(3 * SECOND)
                 logger.debug("CoreAgentSocketThread connected")
                 return True
@@ -196,4 +197,19 @@ class CoreAgentSocketThread(SingletonThread):
                 "CoreAgentSocketThread exception on disconnect: %r", exc, exc_info=exc
             )
         finally:
-            self.socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+            self.socket = self.make_socket()
+
+    def make_socket(self):
+        if self.socket_path.is_tcp:
+            family = socket.AF_INET
+        else:
+            family = socket.AF_UNIX
+        return socket.socket(family, socket.SOCK_STREAM)
+
+    def get_socket_address(self):
+        if self.socket_path.is_tcp:
+            host, _, port = self.socket_path.tcp_address.partition(":")
+            if sys.version_info[0] == 2:
+                host = bytes(host)
+            return host, int(port)
+        return self.socket_path
diff --git a/src/scout_apm/core/threading.py b/src/scout_apm/core/threading.py
index 90b7ab0..d7873c6 100644
--- a/src/scout_apm/core/threading.py
+++ b/src/scout_apm/core/threading.py
@@ -23,7 +23,6 @@ class SingletonThread(threading.Thread):
             if cls._instance is None or not cls._instance.is_alive():
                 cls._instance = cls()
                 cls._instance.start()
-            return
 
     @classmethod
     def ensure_stopped(cls):


Test patch:
diff --git a/tests/compat.py b/tests/compat.py
index 51688f5..35dd36c 100644
--- a/tests/compat.py
+++ b/tests/compat.py
@@ -26,8 +26,8 @@ else:
     from tempfile import mkdtemp
 
     @contextmanager
-    def TemporaryDirectory():
-        tempdir = mkdtemp()
+    def TemporaryDirectory(*args, **kwargs):
+        tempdir = mkdtemp(*args, **kwargs)
         try:
             yield tempdir
         finally:
diff --git a/tests/conftest.py b/tests/conftest.py
index bb7f2d3..4511cc7 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -3,9 +3,9 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 
 import logging
 import os
-import subprocess
 import sys
 
+import psutil
 import pytest
 import wrapt
 from webtest import TestApp
@@ -106,46 +106,42 @@ def isolate_global_state():
                 )
 
 
+@pytest.fixture(autouse=True, scope="session")
+def terminate_core_agent_processes_at_start_of_tests():
+    terminate_core_agent_processes()
+    yield
+
+
 # Create a temporary directory for isolation between test sessions.
 # Do it once per test session to avoid downloading the core agent repeatedly.
 @pytest.fixture(autouse=True, scope="session")
 def core_agent_dir():
-    with TemporaryDirectory() as temp_dir:
+    # Use /tmp/ to avoid core agent startup error:
+    #   [socket::server][ERROR] Error opening listener on socket: Custom
+    #   { kind: InvalidInput, error: "path must be shorter than SUN_LEN" }
+    with TemporaryDirectory(dir="/tmp/") as temp_dir:
         yield temp_dir
 
 
 @pytest.fixture
 def core_agent_manager(core_agent_dir):
-    # Shorten path to socket to prevent core-agent from failing with:
-    #   Error opening listener on socket: Custom { kind: InvalidInput,
-    #   error: StringError("path must be shorter than SUN_LEN") }
-    socket_path = "{}/test.sock".format(core_agent_dir)
-    scout_config.set(core_agent_dir=core_agent_dir, core_agent_socket_path=socket_path)
+    scout_config.set(core_agent_dir=core_agent_dir)
     core_agent_manager = CoreAgentManager()
     try:
         yield core_agent_manager
     finally:
-        assert not is_running(core_agent_manager)
+        assert not core_agent_is_running()
         scout_config.reset_all()
 
 
-def is_running(core_agent_manager):
-    if core_agent_manager.core_agent_bin_path is None:
-        return False
-    agent_binary = [core_agent_manager.core_agent_bin_path, "probe"]
-    socket_path = core_agent_manager.socket_path()
-    probe = subprocess.check_output(agent_binary + socket_path)
-    if b"Agent found" in probe:
-        return True
-    if b"Agent Not Running" in probe:
-        return False
-    raise AssertionError("cannot tell if the core agent is running")
+def core_agent_is_running():
+    return any(p.name() == "core-agent" for p in psutil.process_iter(["name"]))
 
 
-def shutdown(core_agent_manager):
-    agent_binary = [core_agent_manager.core_agent_bin_path, "shutdown"]
-    socket_path = core_agent_manager.socket_path()
-    subprocess.check_call(agent_binary + socket_path)
+def terminate_core_agent_processes():
+    for process in psutil.process_iter(["name"]):
+        if process.name() == "core-agent":
+            process.terminate()
 
 
 # Make all timeouts shorter so that tests exercising them run faster.
diff --git a/tests/integration/core/test_core_agent_manager.py b/tests/integration/core/test_core_agent_manager.py
index 34b663a..4b917cd 100644
--- a/tests/integration/core/test_core_agent_manager.py
+++ b/tests/integration/core/test_core_agent_manager.py
@@ -4,9 +4,12 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 import logging
 import time
 
+import pytest
+
 from scout_apm.core.config import scout_config
+from scout_apm.core.core_agent_manager import CoreAgentManager
 from tests.compat import mock
-from tests.conftest import is_running, shutdown
+from tests.conftest import core_agent_is_running, terminate_core_agent_processes
 
 # Tests must execute in the order in which they are defined.
 
@@ -20,7 +23,7 @@ def test_no_launch(caplog, core_agent_manager):
         scout_config.set(core_agent_launch=True)
 
     assert not result
-    assert not is_running(core_agent_manager)
+    assert not core_agent_is_running()
     assert caplog.record_tuples == [
         (
             "scout_apm.core.core_agent_manager",
@@ -42,7 +45,7 @@ def test_no_verify(caplog, core_agent_manager):
         scout_config.set(core_agent_download=True)
 
     assert not result
-    assert not is_running(core_agent_manager)
+    assert not core_agent_is_running()
     assert (
         "scout_apm.core.core_agent_manager",
         logging.DEBUG,
@@ -53,11 +56,34 @@ def test_no_verify(caplog, core_agent_manager):
     ) in caplog.record_tuples
 
 
-def test_download_and_launch(core_agent_manager):
-    assert core_agent_manager.launch()
-    time.sleep(0.01)  # wait for agent to start running
-    assert is_running(core_agent_manager)
-    shutdown(core_agent_manager)
+@pytest.mark.parametrize(
+    "path",
+    [
+        None,
+        "tcp://127.0.0.1:5678",
+        "/tmp/scout-tests.sock",
+    ],
+)
+def test_download_and_launch(path, core_agent_manager):
+    if path is not None:
+        scout_config.set(core_agent_socket_path=path)
+
+    try:
+        result = core_agent_manager.launch()
+
+        assert result is True
+
+        time.sleep(0.10)  # wait for agent to start running
+        for _ in range(10):
+            if core_agent_is_running():
+                break
+            time.sleep(0.1)
+        else:
+            raise AssertionError("Could not find core agent running")
+
+        terminate_core_agent_processes()
+    finally:
+        scout_config.reset_all()
 
 
 def test_verify_error(caplog, core_agent_manager):
@@ -75,7 +101,7 @@ def test_verify_error(caplog, core_agent_manager):
         result = core_agent_manager.launch()
 
     assert not result
-    assert not is_running(core_agent_manager)
+    assert not core_agent_is_running()
     assert (
         "scout_apm.core.core_agent_manager",
         logging.DEBUG,
@@ -86,79 +112,16 @@ def test_verify_error(caplog, core_agent_manager):
 def test_launch_error(caplog, core_agent_manager):
     caplog.set_level(logging.ERROR)
     exception = ValueError("Hello Fail")
-    with mock.patch(
-        "scout_apm.core.core_agent_manager.CoreAgentManager.agent_binary",
+    with mock.patch.object(
+        CoreAgentManager,
+        "agent_binary",
         side_effect=exception,
     ):
         result = core_agent_manager.launch()
 
     assert not result
-    assert not is_running(core_agent_manager)
+    assert not core_agent_is_running()
     assert caplog.record_tuples == [
         ("scout_apm.core.core_agent_manager", logging.ERROR, "Error running Core Agent")
     ]
     assert caplog.records[0].exc_info[1] is exception
-
-
-def test_socket_path(core_agent_manager):
-    scout_config.set(core_agent_socket_path="foo")
-
-    result = core_agent_manager.socket_path()
-
-    assert result == ["--socket", "foo"]
-
-
-def test_socket_path_old_name_takes_precedence(core_agent_manager):
-    scout_config.set(socket_path="foo", core_agent_socket_path="bar")
-
-    result = core_agent_manager.socket_path()
-
-    assert result == ["--socket", "foo"]
-
-
-def test_log_level(core_agent_manager):
-    scout_config.set(core_agent_log_level="foo")
-
-    result = core_agent_manager.log_level()
-
-    assert result == ["--log-level", "foo"]
-
-
-def test_log_level_old_name_takes_precedence(core_agent_manager):
-    scout_config.set(log_level="foo", core_agent_log_level="bar")
-
-    result = core_agent_manager.log_level()
-
-    assert result == ["--log-level", "foo"]
-
-
-def test_log_file(core_agent_manager):
-    scout_config.set(core_agent_log_file="foo")
-
-    result = core_agent_manager.log_file()
-
-    assert result == ["--log-file", "foo"]
-
-
-def test_log_file_old_name_takes_precedence(core_agent_manager):
-    scout_config.set(log_file="foo", core_agent_log_file="bar")
-
-    result = core_agent_manager.log_file()
-
-    assert result == ["--log-file", "foo"]
-
-
-def test_config_file(core_agent_manager):
-    scout_config.set(core_agent_config_file="foo")
-
-    result = core_agent_manager.config_file()
-
-    assert result == ["--config-file", "foo"]
-
-
-def test_config_file_old_name_takes_precedence(core_agent_manager):
-    scout_config.set(config_file="foo", core_agent_config_file="bar")
-
-    result = core_agent_manager.config_file()
-
-    assert result == ["--config-file", "foo"]
diff --git a/tests/integration/core/test_socket.py b/tests/integration/core/test_socket.py
index afbe3c1..89bd877 100644
--- a/tests/integration/core/test_socket.py
+++ b/tests/integration/core/test_socket.py
@@ -7,20 +7,27 @@ import pytest
 
 from scout_apm.core.socket import CoreAgentSocketThread
 from tests.compat import mock
-from tests.conftest import is_running, shutdown
+from tests.conftest import core_agent_is_running, terminate_core_agent_processes
 
 
 @pytest.fixture
 def running_agent(core_agent_manager):
-    assert not is_running(core_agent_manager)
+    assert not core_agent_is_running()
     assert core_agent_manager.launch()
-    time.sleep(0.01)  # wait for agent to start running
-    assert is_running(core_agent_manager)
+
+    # Wait for agent to start running
+    for _ in range(400):
+        if core_agent_is_running():
+            break
+        time.sleep(0.01)
+    else:
+        raise AssertionError("Core agent did not start in 4 second")
+
     try:
         yield
     finally:
-        shutdown(core_agent_manager)
-        assert not is_running(core_agent_manager)
+        terminate_core_agent_processes()
+        assert not core_agent_is_running()
 
 
 @pytest.fixture
@@ -58,8 +65,6 @@ def test_send_network_error(sendall, socket):
 
 
 def test_wait_until_drained_empty(socket):
-    CoreAgentSocketThread.ensure_stopped()
-
     empty = CoreAgentSocketThread.wait_until_drained()
     assert empty
 
@@ -67,7 +72,7 @@ def test_wait_until_drained_empty(socket):
 def test_wait_until_drained_one_item(socket):
     CoreAgentSocketThread._command_queue.put(Command(), False)
 
-    empty = CoreAgentSocketThread.wait_until_drained(timeout_seconds=0.1)
+    empty = CoreAgentSocketThread.wait_until_drained()
     assert empty
 
 
diff --git a/tests/unit/core/test_core_agent_manager.py b/tests/unit/core/test_core_agent_manager.py
index 18ff0b0..3af685a 100644
--- a/tests/unit/core/test_core_agent_manager.py
+++ b/tests/unit/core/test_core_agent_manager.py
@@ -6,10 +6,96 @@ import logging
 import sys
 
 from scout_apm.core.config import scout_config
-from scout_apm.core.core_agent_manager import get_socket_path, parse_manifest
+from scout_apm.core.core_agent_manager import (
+    CoreAgentManager,
+    get_socket_path,
+    parse_manifest,
+)
 from tests.compat import mock
 
 
+class TestCoreAgentManager(object):
+    def test_socket_path_path(self):
+        scout_config.set(core_agent_socket_path="/tmp/foo.sock")
+
+        try:
+            result = CoreAgentManager().socket_path()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--socket", "/tmp/foo.sock"]
+
+    def test_socket_path_tcp(self):
+        scout_config.set(core_agent_socket_path="tcp://127.0.0.1:7894")
+
+        try:
+            result = CoreAgentManager().socket_path()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--tcp", "127.0.0.1:7894"]
+
+    def test_log_level(self):
+        scout_config.set(core_agent_log_level="foo")
+
+        try:
+            result = CoreAgentManager().log_level()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--log-level", "foo"]
+
+    def test_log_level_old_name_takes_precedence(self):
+        scout_config.set(log_level="foo", core_agent_log_level="bar")
+
+        try:
+            result = CoreAgentManager().log_level()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--log-level", "foo"]
+
+    def test_log_file(self):
+        scout_config.set(core_agent_log_file="foo")
+
+        try:
+            result = CoreAgentManager().log_file()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--log-file", "foo"]
+
+    def test_log_file_old_name_takes_precedence(self):
+        scout_config.set(log_file="foo", core_agent_log_file="bar")
+
+        try:
+            result = CoreAgentManager().log_file()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--log-file", "foo"]
+
+    def test_config_file(self):
+        scout_config.set(core_agent_config_file="foo")
+
+        try:
+            result = CoreAgentManager().config_file()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--config-file", "foo"]
+
+    def test_config_file_old_name_takes_precedence(self):
+        scout_config.set(config_file="foo", core_agent_config_file="bar")
+
+        try:
+            result = CoreAgentManager().config_file()
+        finally:
+            scout_config.reset_all()
+
+        assert result == ["--config-file", "foo"]
+
+
 class TestParseManifest(object):
     def test_fail_does_not_exist(self, caplog, tmp_path):
         filename = str(tmp_path / "does-not-exist.json")
@@ -127,9 +213,16 @@ class TestGetSocketPath(object):
         finally:
             scout_config.reset_all()
 
-    def test_derived(self):
-        scout_config.set(core_agent_dir="/tmp/mydir", core_agent_full_name="my-agent")
+    def test_not_tcp(self):
+        scout_config.set(core_agent_socket_path="/tmp/that.sock")
+        try:
+            assert not get_socket_path().is_tcp
+        finally:
+            scout_config.reset_all()
+
+    def test_tcp(self):
+        scout_config.set(core_agent_socket_path="tcp://127.0.0.1:1234")
         try:
-            assert get_socket_path() == "/tmp/mydir/my-agent/scout-agent.sock"
+            assert get_socket_path().is_tcp
         finally:
             scout_config.reset_all()