Start the agent when a collector is used

.changesets/report-host-metrics-when-using-a-collector.md

@@ -0,0 +1,6 @@
+---
+bump: minor
+type: add
+---
+
+Report host metrics, NGINX metrics, StatsD metrics and environment metadata when using a collector. These are sent by the AppSignal agent, which now runs alongside the collector instead of being replaced by it.

src/appsignal/agent.py

@@ -8,12 +8,11 @@
 from pathlib import Path
 
 from . import internal_logger as logger
-from .binary import Binary
 from .config import Config
 
 
 @dataclass
-class Agent(Binary):
+class Agent:
     package_path: Path = Path(__file__).parent
     agent_path: Path = package_path / "appsignal-agent"
     platform_path: Path = package_path / "_appsignal_platform"
@@ -56,7 +55,13 @@ def stop(self, config: Config) -> None:
                 line = file.readline()
                 pid = int(line.split(";")[2])
                 os.kill(pid, signal.SIGTERM)
-                time.sleep(2)
+                # Give the agent time to send what it holds before this
+                # process exits, which matters where the whole environment is
+                # frozen once it does. When a collector is used the agent only
+                # holds host, NGINX and StatsD metrics, so losing its last
+                # batch is worth a shutdown that is two seconds quicker.
+                if not config.should_use_collector():
+                    time.sleep(2)
         except FileNotFoundError:
             logger.info("Agent lock file not found; not stopping the agent")
 

src/appsignal/client.py

@@ -3,7 +3,7 @@
 from typing import TYPE_CHECKING
 
 from . import internal_logger as logger
-from .binary import NoopBinary
+from .agent import Agent
 from .config import Config, Options
 from .opentelemetry import start as start_opentelemetry
 from .opentelemetry import stop as stop_opentelemetry
@@ -14,8 +14,6 @@
 if TYPE_CHECKING:
     from typing_extensions import Unpack
 
-    from .binary import Binary
-
 
 _client: Client | None = None
 
@@ -27,13 +25,13 @@ def _reset_client() -> None:
 
 class Client:
     _config: Config
-    _binary: Binary
+    _agent: Agent
 
     def __init__(self, **options: Unpack[Options]) -> None:
         global _client
 
         self._config = Config(options)
-        self._binary = NoopBinary()
+        self._agent = Agent()
         _client = self
 
     @classmethod
@@ -44,14 +42,20 @@ def config(cls) -> Config | None:
         return _client._config
 
     def start(self) -> None:
-        self._set_binary()
-
         if self._config.is_active():
             logger.info("Starting AppSignal")
             self._config.warn()
-            self._binary.start(self._config)
-            if not self._binary.active:
-                return
+            self._agent.start(self._config)
+            if not self._agent.active:
+                # Without the agent there is nothing to send trace data to,
+                # unless a collector receives it instead.
+                if not self._config.should_use_collector():
+                    return
+                logger.warning(
+                    "The AppSignal agent did not start. Host metrics, NGINX "
+                    "metrics, StatsD metrics and environment metadata will "
+                    "not be reported."
+                )
             start_opentelemetry(self._config)
             self._start_probes()
         else:
@@ -70,23 +74,9 @@ def stop(self) -> None:
         # agent is used, it is the endpoint that data is sent to, so it must
         # still be running to receive it.
         stop_opentelemetry()
-        self._binary.stop(self._config)
+        if self._agent.active:
+            self._agent.stop(self._config)
 
     def _start_probes(self) -> None:
         if self._config.option("enable_minutely_probes"):
             start_probes()
-
-    def _set_binary(self) -> None:
-        if self._config.should_use_external_collector():
-            # When a custom collector endpoint is set, use a `NoopBinary`
-            # set to active, so that OpenTelemetry and probes are started,
-            # but the agent is not started.
-            logger.info(
-                "Not starting the AppSignal agent: using collector endpoint instead"
-            )
-            self._binary = NoopBinary(active=True)
-        else:
-            # Use the agent when a custom collector endpoint is not set.
-            from .agent import Agent
-
-            self._binary = Agent()

src/appsignal/config.py

@@ -320,7 +320,6 @@ def load_from_environment() -> Options:
 
     CONSTANT_PRIVATE_ENVIRON: ClassVar[dict[str, str]] = {
         "_APPSIGNAL_LANGUAGE_INTEGRATION_VERSION": f"python-{__version__}",
-        "_APPSIGNAL_ENABLE_OPENTELEMETRY_HTTP": "true",
     }
 
     def set_private_environ(self) -> None:
@@ -341,6 +340,13 @@ def set_private_environ(self) -> None:
             "_APPSIGNAL_ENABLE_NGINX_METRICS": bool_to_env_str(
                 options.get("enable_nginx_metrics")
             ),
+            # The agent receives OpenTelemetry data over HTTP when it is the
+            # one sending it to AppSignal. When a collector is used, the data
+            # goes there instead, and the agent's port would clash with the
+            # collector's, which defaults to the same number.
+            "_APPSIGNAL_ENABLE_OPENTELEMETRY_HTTP": bool_to_env_str(
+                not self.should_use_collector()
+            ),
             "_APPSIGNAL_ENABLE_STATSD": bool_to_env_str(options.get("enable_statsd")),
             "_APPSIGNAL_FILES_WORLD_ACCESSIBLE": bool_to_env_str(
                 options.get("files_world_accessible")
@@ -430,24 +436,15 @@ def warn(self) -> None:
             self._warn_collector_exclusive_options()
 
     # Emit a warning if agent-exclusive configuration options are used.
+    #
+    # The agent runs when a collector is used as well, so most of its options
+    # still apply. Only those that configure how it handles trace data do
+    # nothing, because the collector receives that data instead.
     def _warn_agent_exclusive_options(self) -> None:
         exclusive_options = [
-            "bind_address",
-            "cpu_count",
-            "dns_servers",
-            "enable_host_metrics",
-            "enable_nginx_metrics",
-            "enable_statsd",
-            "files_world_accessible",
             "filter_parameters",
-            "host_role",
-            "nginx_port",
             "opentelemetry_port",
-            "running_in_container",
-            "send_environment_metadata",
             "send_params",
-            "working_directory_path",
-            "statsd_port",
         ]
 
         option_specific_warnings = {
@@ -471,7 +468,7 @@ def _warn_agent_exclusive_options(self) -> None:
         for option in user_modified_options:
             logger.warning(
                 f"The collector is in use. The '{option}' configuration option"
-                " is only used by the agent and will be ignored."
+                " is only used by the agent for trace data and will be ignored."
             )
             if option in option_specific_warnings:
                 logger.warning(option_specific_warnings[option])

tests/test_client.py

@@ -6,7 +6,6 @@
 
 from appsignal import probes
 from appsignal.agent import Agent
-from appsignal.binary import NoopBinary
 from appsignal.client import Client
 
 
@@ -25,8 +24,8 @@ def test_client_agent_inactive():
     client.start()
 
     assert os.environ.get("_APPSIGNAL_ACTIVE") is None
-    assert type(client._binary) is Agent
-    assert client._binary.active is False
+    assert type(client._agent) is Agent
+    assert client._agent.active is False
 
 
 def test_client_agent_active():
@@ -36,8 +35,8 @@ def test_client_agent_active():
     client.start()
 
     assert os.environ.get("_APPSIGNAL_ACTIVE") == "true"
-    assert type(client._binary) is Agent
-    assert client._binary.active is True
+    assert type(client._agent) is Agent
+    assert client._agent.active is True
 
 
 def test_client_agent_active_invalid():
@@ -47,11 +46,11 @@ def test_client_agent_active_invalid():
     client.start()
 
     assert os.environ.get("_APPSIGNAL_ACTIVE") is None
-    assert type(client._binary) is Agent
-    assert client._binary.active is False
+    assert type(client._agent) is Agent
+    assert client._agent.active is False
 
 
-def test_client_active_noopbinary_when_collector_endpoint_set():
+def test_client_active_when_collector_endpoint_set():
     client = Client(
         active=True,
         name="MyApp",
@@ -62,13 +61,18 @@ def test_client_active_noopbinary_when_collector_endpoint_set():
 
     client.start()
 
-    assert type(client._binary) is NoopBinary
-    assert client._binary.active
+    # Starts the agent, which reports what the collector does not
+    assert type(client._agent) is Agent
+    assert client._agent.active
 
-    # Does not set the private config environment variables
-    assert os.environ.get("_APPSIGNAL_ACTIVE") is None
-    assert os.environ.get("_APPSIGNAL_APP_NAME") is None
-    assert os.environ.get("_APPSIGNAL_PUSH_API_KEY") is None
+    # Sets the private config environment variables
+    assert os.environ.get("_APPSIGNAL_ACTIVE") == "true"
+    assert os.environ.get("_APPSIGNAL_APP_NAME") == "MyApp"
+    assert os.environ.get("_APPSIGNAL_PUSH_API_KEY") == "0000-0000-0000-0000"
+
+    # Does not let the agent listen for OpenTelemetry data, because it is sent
+    # to the collector instead, on a port that defaults to the same number
+    assert os.environ.get("_APPSIGNAL_ENABLE_OPENTELEMETRY_HTTP") == "false"
 
     # Sets the OpenTelemetry config environment variables
     assert (
@@ -77,6 +81,40 @@ def test_client_active_noopbinary_when_collector_endpoint_set():
     )
 
 
+def test_client_starts_opentelemetry_in_collector_mode_without_the_agent(mocker):
+    mocker.patch("appsignal.agent.Agent.start")
+    start_opentelemetry = mocker.patch("appsignal.client.start_opentelemetry")
+    warning = mocker.patch("appsignal.internal_logger.warning")
+
+    client = Client(
+        active=True,
+        name="MyApp",
+        push_api_key="0000-0000-0000-0000",
+        collector_endpoint="https://custom-endpoint.appsignal.com",
+    )
+
+    client.start()
+
+    assert client._agent.active is False
+    start_opentelemetry.assert_called_once()
+    assert any(
+        "The AppSignal agent did not start" in call.args[0]
+        for call in warning.call_args_list
+    )
+
+
+def test_client_does_not_start_opentelemetry_without_the_agent(mocker):
+    mocker.patch("appsignal.agent.Agent.start")
+    start_opentelemetry = mocker.patch("appsignal.client.start_opentelemetry")
+
+    client = Client(active=True, name="MyApp", push_api_key="0000-0000-0000-0000")
+
+    client.start()
+
+    assert client._agent.active is False
+    start_opentelemetry.assert_not_called()
+
+
 def test_client_active():
     client = Client(
         active=True,
@@ -96,14 +134,17 @@ def test_client_active():
     assert os.environ.get("_APPSIGNAL_APP_NAME") == "MyApp"
     assert os.environ.get("_APPSIGNAL_PUSH_API_KEY") == "0000-0000-0000-0000"
 
+    # Lets the agent listen for OpenTelemetry data, because it is sent there
+    assert os.environ.get("_APPSIGNAL_ENABLE_OPENTELEMETRY_HTTP") == "true"
+
     # Sets the OpenTelemetry config environment variables
     assert (
         os.environ.get("OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST")
         == "accept,x-custom-header"
     )
 
-    assert type(client._binary) is Agent
-    assert client._binary.active
+    assert type(client._agent) is Agent
+    assert client._agent.active
 
 
 def test_client_active_without_request_headers():
@@ -151,6 +192,44 @@ def test_client_stop_kills_agent(mock_open, mock_kill, mock_sleep):
             call(123, signal.SIGTERM),
         ]
     )
+    # Waits for the agent to send the trace data it still holds
+    assert call(2) in mock_sleep.call_args_list
+
+
+@patch("time.sleep", return_value=None)
+@patch("os.kill", return_value=None)
+@patch("builtins.open", new_callable=mock_open, read_data="123456;running;123\n")
+def test_client_stop_does_not_wait_for_the_agent_in_collector_mode(
+    mock_open, mock_kill, mock_sleep
+):
+    client = Client(
+        active=True,
+        name="MyApp",
+        push_api_key="0000-0000-0000-0000",
+        collector_endpoint="https://custom-endpoint.appsignal.com",
+    )
+    client.start()
+
+    client.stop()
+
+    mock_kill.assert_has_calls(
+        [
+            call(123, signal.SIGTERM),
+        ]
+    )
+    # Does not wait for the agent, which only holds host, NGINX and StatsD
+    # metrics when a collector is used
+    assert call(2) not in mock_sleep.call_args_list
+
+
+def test_client_stop_does_not_kill_an_agent_it_did_not_start(mocker):
+    kill = mocker.patch("os.kill")
+
+    client = Client(active=True, name="MyApp", push_api_key="0000-0000-0000-0000")
+
+    client.stop()
+
+    kill.assert_not_called()
 
 
 def test_client_stop_stops_probes(mocker):