From c9b9c61bda3b45584c0804b406a595fc317218b8 Mon Sep 17 00:00:00 2001 From: Noemi Lapresta Date: Thu, 27 Aug 2026 17:52:19 +0200 Subject: [PATCH] Start the agent when a collector is used Setting `collector_endpoint` replaces the agent, so host metrics, NGINX metrics, StatsD metrics and environment metadata are not reported at all. The collector implements none of them for the application's host: it can report host metrics, but for the host it runs on, which is another machine. The agent now runs alongside the collector, and only the trace, metric and log data moves to the collector. Its OpenTelemetry listener is turned off there, because that data goes to the collector instead and the two default to the same port. The agent no longer decides whether AppSignal starts, so collector mode keeps working where the agent is unavailable, and stopping it no longer waits for it to flush, because what it holds is a minute of host metrics. Most agent options apply again, leaving only those that configure how it handles trace data to warn that they are ignored. --- ...ort-host-metrics-when-using-a-collector.md | 6 + src/appsignal/agent.py | 11 +- src/appsignal/binary.py | 32 ----- src/appsignal/client.py | 42 +++---- src/appsignal/config.py | 27 ++--- tests/test_client.py | 111 +++++++++++++++--- tests/test_config.py | 57 ++++----- 7 files changed, 167 insertions(+), 119 deletions(-) create mode 100644 .changesets/report-host-metrics-when-using-a-collector.md delete mode 100644 src/appsignal/binary.py diff --git a/.changesets/report-host-metrics-when-using-a-collector.md b/.changesets/report-host-metrics-when-using-a-collector.md new file mode 100644 index 0000000..327b2cf --- /dev/null +++ b/.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. diff --git a/src/appsignal/agent.py b/src/appsignal/agent.py index 1d3c41e..9ace68d 100644 --- a/src/appsignal/agent.py +++ b/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") diff --git a/src/appsignal/binary.py b/src/appsignal/binary.py deleted file mode 100644 index c2f831e..0000000 --- a/src/appsignal/binary.py +++ /dev/null @@ -1,32 +0,0 @@ -from __future__ import annotations - -from abc import ABC, abstractmethod - -from .config import Config - - -class Binary(ABC): - @property - @abstractmethod - def active(self) -> bool: ... - - @abstractmethod - def start(self, config: Config) -> None: ... - - @abstractmethod - def stop(self, config: Config) -> None: ... - - -class NoopBinary(Binary): - def __init__(self, active: bool = False) -> None: - self._active = active - - @property - def active(self) -> bool: - return self._active - - def start(self, config: Config) -> None: - pass - - def stop(self, config: Config) -> None: - pass diff --git a/src/appsignal/client.py b/src/appsignal/client.py index ecbdeb5..d2dc0a3 100644 --- a/src/appsignal/client.py +++ b/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() diff --git a/src/appsignal/config.py b/src/appsignal/config.py index 555a5ca..9f0fb27 100644 --- a/src/appsignal/config.py +++ b/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]) diff --git a/tests/test_client.py b/tests/test_client.py index 9c6a452..64faef6 100644 --- a/tests/test_client.py +++ b/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): diff --git a/tests/test_config.py b/tests/test_config.py index 702dcfd..7457398 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -586,6 +586,34 @@ def test_warn_no_warnings_when_using_default_values(mocker): assert mock_warning.call_count == 0 +def test_warn_no_warnings_for_options_the_agent_still_uses(mocker): + mock_warning = mocker.patch("appsignal.internal_logger.warning") + + # The agent runs when a collector is used too, so the options that + # configure the rest of what it does still apply. + config = Config( + Options( + collector_endpoint="http://localhost:4318", + bind_address="0.0.0.0", + cpu_count=2.0, + dns_servers=["8.8.8.8"], + enable_host_metrics=False, + enable_nginx_metrics=True, + enable_statsd=True, + files_world_accessible=False, + host_role="web", + nginx_port="8080", + running_in_container=True, + send_environment_metadata=False, + working_directory_path="/app", + statsd_port="8125", + ) + ) + config.warn() + + assert mock_warning.call_count == 0 + + def test_warn_all_agent_exclusive_options(mocker): mock_warning = mocker.patch("appsignal.internal_logger.warning") mock_info = mocker.patch("appsignal.internal_logger.info") @@ -594,22 +622,9 @@ def config_builder() -> Config: return Config( Options( collector_endpoint="http://localhost:4318", - bind_address="0.0.0.0", - cpu_count=2.0, - dns_servers=["8.8.8.8"], - enable_host_metrics=False, - enable_nginx_metrics=True, - enable_statsd=True, - files_world_accessible=False, filter_parameters=["password"], - host_role="web", - nginx_port="8080", opentelemetry_port="9999", - running_in_container=True, - send_environment_metadata=False, send_params=False, - working_directory_path="/app", - statsd_port="8125", ) ) @@ -629,27 +644,15 @@ def config_builder() -> Config: warning_messages = [call.args[0] for call in mock_warning.call_args_list] agent_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", ] for option in agent_exclusive_options: assert any( - f"'{option}' configuration option is only used by the agent" in msg + f"'{option}' configuration option is only used by the agent" + " for trace data" in msg for msg in warning_messages ), f"Expected warning for '{option}' not found"