diff --git a/.changesets/detect-the-revision-from-the-platform.md b/.changesets/detect-the-revision-from-the-platform.md new file mode 100644 index 0000000..17840bd --- /dev/null +++ b/.changesets/detect-the-revision-from-the-platform.md @@ -0,0 +1,8 @@ +--- +bump: patch +type: add +--- + +Detect the revision that is being deployed from the environment variables set by Heroku, Render, Kamal and Scalingo: `HEROKU_SLUG_COMMIT`, `RENDER_GIT_COMMIT`, `KAMAL_VERSION` and `CONTAINER_VERSION`. Applications deployed on those platforms now report their revision without setting the `revision` configuration option. + +This affects collector mode, where deploys were reported as `unknown` when the revision was not configured. diff --git a/.changesets/keep-detected-values-when-an-option-is-none.md b/.changesets/keep-detected-values-when-an-option-is-none.md new file mode 100644 index 0000000..54bffcf --- /dev/null +++ b/.changesets/keep-detected-values-when-an-option-is-none.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: fix +--- + +An option set to `None` when initializing the `Appsignal` client no longer replaces a value that AppSignal detected itself. For example, `Appsignal(hostname=None)` now reports the detected hostname, instead of reporting no hostname at all. Options that AppSignal does not detect are unchanged: setting `request_headers` to `None`, for example, still turns off request header collection. diff --git a/.changesets/prefer-the-dyno-name-for-the-hostname.md b/.changesets/prefer-the-dyno-name-for-the-hostname.md new file mode 100644 index 0000000..3d10402 --- /dev/null +++ b/.changesets/prefer-the-dyno-name-for-the-hostname.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: change +--- + +On Heroku, report the name of the dyno as the hostname. Before, the hostname of the container that the dyno runs in was reported, so applications running on Heroku will see their data reported under a new host name. diff --git a/.changesets/report-host-name-when-set-to-none.md b/.changesets/report-host-name-when-set-to-none.md deleted file mode 100644 index ec808c7..0000000 --- a/.changesets/report-host-name-when-set-to-none.md +++ /dev/null @@ -1,8 +0,0 @@ ---- -bump: patch -type: fix ---- - -Report `unknown` as the host name in collector mode when the `hostname` -configuration option is set to `None`. Apps that set it that way reported no -host name at all. diff --git a/src/appsignal/config.py b/src/appsignal/config.py index 6170f96..555a5ca 100644 --- a/src/appsignal/config.py +++ b/src/appsignal/config.py @@ -49,6 +49,7 @@ class Options(TypedDict, total=False): nginx_port: str | int | None opentelemetry_port: str | int | None name: str | None + platform: str | None push_api_key: str | None revision: str | None request_headers: list[str] | None @@ -134,10 +135,11 @@ class Config: def __init__(self, options: Options | None = None) -> None: self.valid = False + system = Config.load_from_system() self.sources = Sources( default=self.DEFAULT_CONFIG, - system=Config.load_from_system(), - initial=options or Options(), + system=system, + initial=without_none_overrides(options or Options(), system), environment=Config.load_from_environment(), ) final_options = Options() @@ -171,13 +173,63 @@ def should_use_collector(self) -> bool: def should_use_external_collector(self) -> bool: return self.option("collector_endpoint") is not None + # Environment variables that deployment platforms set to the revision that + # is being deployed, in the order the agent reads them. The agent detects + # the revision this way as well, but only for the data it reports itself, + # so the package has to do it for collector mode. + PLATFORM_REVISION_ENVIRONMENT_VARIABLES: ClassVar[list[str]] = [ + "HEROKU_SLUG_COMMIT", + "RENDER_GIT_COMMIT", + "KAMAL_VERSION", + "CONTAINER_VERSION", # Scalingo + ] + @staticmethod def load_from_system() -> Options: - return Options( + options = Options( app_path=os.getcwd(), - hostname=os.environ.get("HOSTNAME") or socket.gethostname(), + # The Heroku dyno name comes first, the way the agent detects the + # hostname. Heroku sets the container hostname as well, and the + # dyno name is the more useful of the two. + hostname=os.environ.get("DYNO") + or os.environ.get("HOSTNAME") + or socket.gethostname(), ) + revision = Config.detect_revision() + if revision is not None: + options["revision"] = revision + + detected_platform = Config.detect_platform() + if detected_platform is not None: + options["platform"] = detected_platform + + return options + + @staticmethod + def detect_revision() -> str | None: + for variable in Config.PLATFORM_REVISION_ENVIRONMENT_VARIABLES: + revision = os.environ.get(variable) + if revision: + return revision + + return None + + # Detect the platform the application is deployed on, the way the agent + # does. The agent only detects it for the data it reports itself, so the + # package has to do it for collector mode. It is detected rather than + # configured: it has no environment variable of its own, and it is not + # documented as an option. + @staticmethod + def detect_platform() -> str | None: + if os.environ.get("DOKKU_ROOT"): + return "dokku" + + if os.environ.get("DYNO"): + return "heroku" + + return None + @staticmethod def load_from_environment() -> Options: options = Options( @@ -313,6 +365,7 @@ def set_private_environ(self) -> None: "_APPSIGNAL_LOGGING_ENDPOINT": options.get("logging_endpoint"), "_APPSIGNAL_NGINX_PORT": options.get("nginx_port"), "_APPSIGNAL_OPENTELEMETRY_PORT": options.get("opentelemetry_port"), + "_APPSIGNAL_PLATFORM": options.get("platform"), "_APPSIGNAL_PUSH_API_KEY": options.get("push_api_key"), "_APPSIGNAL_PUSH_API_ENDPOINT": options.get("endpoint"), "_APPSIGNAL_RUNNING_IN_CONTAINER": bool_to_env_str( @@ -501,6 +554,20 @@ def parse_bool(value: str | None) -> bool | None: return None +# An option passed to the client as None carries no value, so it must not erase +# one that was detected from the system. It does still override a default: that +# is how `request_headers=None` turns off request header collection. +def without_none_overrides(options: Options, system: Options) -> Options: + return cast( + Options, + { + key: value + for key, value in options.items() + if value is not None or key not in system + }, + ) + + def parse_list(value: str | None) -> list[str] | None: if value is None: return None diff --git a/src/appsignal/opentelemetry.py b/src/appsignal/opentelemetry.py index 1806332..4fda8e7 100644 --- a/src/appsignal/opentelemetry.py +++ b/src/appsignal/opentelemetry.py @@ -290,6 +290,7 @@ def _resource(config: Config) -> Resource: "appsignal.config.push_api_key": config.options.get("push_api_key"), "appsignal.config.revision": config.options.get("revision") or "unknown", "appsignal.config.app_path": config.options.get("app_path"), + "appsignal.config.platform": config.options.get("platform"), "appsignal.config.language_integration": "python", "service.name": config.options.get("service_name") or "app", "host.name": config.options.get("hostname") or "unknown", diff --git a/tests/test_config.py b/tests/test_config.py index 56ab391..702dcfd 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,11 +1,34 @@ from __future__ import annotations import os +import socket + +import pytest from appsignal.__about__ import __version__ from appsignal.config import Config, Options +# The system source detects configuration from these environment variables, so +# a value that happens to be set where the tests run changes what it detects. +DETECTED_ENVIRONMENT_VARIABLES = [ + "APP_REVISION", + "CONTAINER_VERSION", + "DOKKU_ROOT", + "DYNO", + "HEROKU_SLUG_COMMIT", + "HOSTNAME", + "KAMAL_VERSION", + "RENDER_GIT_COMMIT", +] + + +@pytest.fixture(autouse=True) +def clear_detected_environment_variables(): + for variable in DETECTED_ENVIRONMENT_VARIABLES: + os.environ.pop(variable, None) + + def test_option(): config = Config(Options(active=False, enable_host_metrics=True)) @@ -43,6 +66,115 @@ def test_system_source(): assert "hostname" in list(config.options.keys()) +def test_system_source_hostname(): + config = Config() + + assert config.option("hostname") == socket.gethostname() + + +def test_system_source_hostname_from_hostname_variable(): + os.environ["HOSTNAME"] = "from-hostname" + config = Config() + + assert config.option("hostname") == "from-hostname" + + +def test_system_source_hostname_prefers_the_dyno_name(): + os.environ["DYNO"] = "web.1" + os.environ["HOSTNAME"] = "from-hostname" + config = Config() + + assert config.option("hostname") == "web.1" + + +def test_initial_options_none_does_not_override_detected_values(): + os.environ["RENDER_GIT_COMMIT"] = "abc123" + config = Config(Options(hostname=None, revision=None)) + + assert config.option("hostname") == socket.gethostname() + assert config.option("revision") == "abc123" + + +def test_initial_options_none_still_overrides_defaults(): + config = Config(Options(request_headers=None)) + + assert config.option("request_headers") is None + + +def test_system_source_platform(): + config = Config() + + assert "platform" not in config.sources["system"] + + +def test_system_source_platform_dokku(): + os.environ["DOKKU_ROOT"] = "~dokku" + config = Config() + + assert config.option("platform") == "dokku" + + +def test_system_source_platform_heroku(): + os.environ["DYNO"] = "web.1" + config = Config() + + assert config.option("platform") == "heroku" + + +def test_system_source_platform_prefers_dokku(): + os.environ["DOKKU_ROOT"] = "~dokku" + os.environ["DYNO"] = "web.1" + config = Config() + + assert config.option("platform") == "dokku" + + +def test_system_source_revision(): + config = Config() + + assert "revision" not in config.sources["system"] + + +def test_system_source_revision_from_platform(): + for variable in [ + "HEROKU_SLUG_COMMIT", + "RENDER_GIT_COMMIT", + "KAMAL_VERSION", + "CONTAINER_VERSION", + ]: + os.environ[variable] = "abc123" + config = Config() + + assert config.sources["system"]["revision"] == "abc123" + assert config.option("revision") == "abc123" + + del os.environ[variable] + + +def test_system_source_revision_reads_variables_in_order(): + os.environ["RENDER_GIT_COMMIT"] = "from-render" + os.environ["KAMAL_VERSION"] = "from-kamal" + config = Config() + + assert config.option("revision") == "from-render" + + +def test_system_source_revision_ignores_empty_variables(): + os.environ["HEROKU_SLUG_COMMIT"] = "" + os.environ["RENDER_GIT_COMMIT"] = "from-render" + config = Config() + + assert config.option("revision") == "from-render" + + +def test_system_source_revision_is_overridden_by_app_revision(): + os.environ["RENDER_GIT_COMMIT"] = "from-render" + os.environ["APP_REVISION"] = "from-app-revision" + config = Config() + + assert config.option("revision") == "from-app-revision" + + def test_environ_source(): os.environ["APPSIGNAL_ACTIVE"] = "true" os.environ["APPSIGNAL_APP_ENV"] = "development" @@ -204,6 +336,7 @@ def test_set_private_environ(): nginx_port=8080, opentelemetry_port=9002, name="MyApp", + platform="heroku", push_api_key="some-api-key", revision="abc123", running_in_container=True, @@ -232,6 +365,7 @@ def test_set_private_environ(): assert os.environ["_APPSIGNAL_FILTER_PARAMETERS"] == "password,secret" assert os.environ["_APPSIGNAL_FILTER_SESSION_DATA"] == "key1,key2" assert os.environ["_APPSIGNAL_HOSTNAME"] == "Test hostname" + assert os.environ["_APPSIGNAL_PLATFORM"] == "heroku" assert os.environ["_APPSIGNAL_HOST_ROLE"] == "a role" assert os.environ["_APPSIGNAL_HTTP_PROXY"] == "http://proxy.local:9999" assert os.environ["_APPSIGNAL_IGNORE_ACTIONS"] == "action1,action2" @@ -267,6 +401,7 @@ def test_opentelemetry_resource(): push_api_key="test-key", revision="abc123", app_path="/path/to/app", + platform="heroku", service_name="test-service", hostname="test-host", filter_attributes=["password", "secret"], @@ -295,6 +430,7 @@ def test_opentelemetry_resource(): assert resource.attributes["appsignal.config.push_api_key"] == "test-key" assert resource.attributes["appsignal.config.revision"] == "abc123" assert resource.attributes["appsignal.config.app_path"] == "/path/to/app" + assert resource.attributes["appsignal.config.platform"] == "heroku" assert resource.attributes["appsignal.config.language_integration"] == "python" assert resource.attributes["service.name"] == "test-service" assert resource.attributes["host.name"] == "test-host" @@ -363,7 +499,7 @@ def test_opentelemetry_resource_with_none_values(): assert resource.attributes["appsignal.config.revision"] == "unknown" assert resource.attributes["service.name"] == "app" - assert resource.attributes["host.name"] == "unknown" + assert resource.attributes["host.name"] == socket.gethostname() def test_set_private_environ_valid_log_path():