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..4cdcd46 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" @@ -27,11 +26,16 @@ def start(self, config: Config) -> None: config.set_private_environ() if self.architecture_and_platform() == ["any"]: - print( - "AppSignal agent is not available for this platform. " - "The integration is now running in no-op mode therefore " - "no data will be sent to AppSignal." - ) + message = "AppSignal agent is not available for this platform." + # When a collector is used, the data still reaches it without the + # agent, so only what the agent reports itself is lost. The client + # names that, and saying nothing is sent would be wrong. + if not config.should_use_collector(): + message += ( + " The integration is now running in no-op mode therefore" + " no data will be sent to AppSignal." + ) + print(message) return p = subprocess.Popen( @@ -56,10 +60,18 @@ 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") + self._active = False + def diagnose(self, config: Config) -> bytes: config.set_private_environ() return subprocess.run( 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..5ffe297 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,14 +468,15 @@ 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]) if user_modified_options: logger.info( - "To use the agent, unset the 'collector_endpoint' configuration option." + "To use the agent for trace data, unset the 'collector_endpoint'" + " configuration option." ) # Emit a warning if collector-exclusive configuration options are used. diff --git a/tests/test_client.py b/tests/test_client.py index 9c6a452..57f74d4 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,73 @@ 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_agent_unavailable_message_in_collector_mode(mocker, capsys): + mocker.patch( + "appsignal.agent.Agent.architecture_and_platform", return_value=["any"] + ) + + client = Client( + active=True, + name="MyApp", + push_api_key="0000-0000-0000-0000", + collector_endpoint="https://custom-endpoint.appsignal.com", + ) + client.start() + + # Data still reaches the collector without the agent, so the message must + # not say that nothing is sent. + output = capsys.readouterr().out + assert "AppSignal agent is not available for this platform." in output + assert "no data will be sent to AppSignal" not in output + + +def test_client_agent_unavailable_message_in_agent_mode(mocker, capsys): + mocker.patch( + "appsignal.agent.Agent.architecture_and_platform", return_value=["any"] + ) + + client = Client(active=True, name="MyApp", push_api_key="0000-0000-0000-0000") + client.start() + + output = capsys.readouterr().out + assert "AppSignal agent is not available for this platform." in output + assert "no data will be sent to AppSignal" in output + + +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 +167,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 +225,59 @@ 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 + + +@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_only_stops_the_agent_once(mock_open, mock_kill, mock_sleep): + client = Client(active=True, name="MyApp", push_api_key="0000-0000-0000-0000") + client.start() + + client.stop() + client.stop() + + # The second stop has no agent left to signal, and the process it would + # signal may belong to something else by then. + assert mock_kill.call_count == 1 + + +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..56a0abd 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,33 +644,22 @@ 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" # Info log about using the agent should only be emitted once mock_info.assert_called_once_with( - "To use the agent, unset the 'collector_endpoint' configuration option." + "To use the agent for trace data, unset the 'collector_endpoint'" + " configuration option." )