diff --git a/.changesets/add-helpers-for-each-kind-of-parameters.md b/.changesets/add-helpers-for-each-kind-of-parameters.md new file mode 100644 index 00000000..f399d052 --- /dev/null +++ b/.changesets/add-helpers-for-each-kind-of-parameters.md @@ -0,0 +1,16 @@ +--- +bump: minor +type: add +--- + +Add the `set_request_payload`, `set_request_query_parameters` and `set_function_parameters` helpers. Each one reports a kind of parameters that collector mode keeps apart, so the option that filters that kind, and the one that suppresses it, apply to what you report: + +```python +from appsignal import set_function_parameters + +set_function_parameters({"user_id": 123}) +``` + +In collector mode, the `set_params` helper is now deprecated, and it reports the request payload. It does not say which kind of parameters it is given, so use `set_request_payload`, `set_request_query_parameters` or `set_function_parameters` instead. AppSignal warns the first time it is used. + +In agent mode there is one place to report parameters, so all four helpers report to it, the last one called is the one that takes effect, and `set_params` is not deprecated. diff --git a/.changesets/deprecate-the-params-options-in-collector-mode.md b/.changesets/deprecate-the-params-options-in-collector-mode.md new file mode 100644 index 00000000..bdab3608 --- /dev/null +++ b/.changesets/deprecate-the-params-options-in-collector-mode.md @@ -0,0 +1,10 @@ +--- +bump: patch +type: change +--- + +The `filter_parameters` and `send_params` configuration options are deprecated in collector mode. Use `filter_request_payload`, `filter_function_parameters` and `filter_request_query_parameters` to filter different kinds of parameters, and `send_request_payload`, `send_request_query_parameters` and `send_function_parameters` to choose which kinds of parameters to report. + +AppSignal warns about the deprecated options at startup, and names the value to set for each option that replaces them. + +In agent mode, `filter_parameters` and `send_params` still apply to every kind of parameter, and the new options have no effect. diff --git a/.changesets/derive-the-collector-mode-parameter-options.md b/.changesets/derive-the-collector-mode-parameter-options.md new file mode 100644 index 00000000..8a9e4a1a --- /dev/null +++ b/.changesets/derive-the-collector-mode-parameter-options.md @@ -0,0 +1,8 @@ +--- +bump: patch +type: change +--- + +In collector mode, the `filter_request_payload`, `filter_function_parameters` and `filter_request_query_parameters` configuration options now fall back to the value of `filter_parameters`, and the `send_request_payload`, `send_request_query_parameters` and `send_function_parameters` options fall back to the value of `send_params`. An application that filtered parameters or turned parameter reporting off keeps doing so after it switches to a collector, without having to set the new options. + +Setting one of the new options still overrides the value that would be derived for it. diff --git a/.changesets/name-a-header-the-way-the-convention-does.md b/.changesets/name-a-header-the-way-the-convention-does.md new file mode 100644 index 00000000..3b36f2db --- /dev/null +++ b/.changesets/name-a-header-the-way-the-convention-does.md @@ -0,0 +1,8 @@ +--- +bump: patch +type: fix +--- + +Report a request or response header whose name is written with capital letters or underscores in the `request_headers` or `response_headers` configuration option, such as `Content-Type` or `content_type`. The collector matches these names against the ones it receives the headers under, which follow the OpenTelemetry semantic convention: the header's own name, lowercased, with its dashes kept. A name written any other way matched nothing, so the header was left out. + +The `set_header` helper names a header the same way when a collector is in use. diff --git a/.changesets/name-the-source-of-each-config-option.md b/.changesets/name-the-source-of-each-config-option.md new file mode 100644 index 00000000..52fd74a2 --- /dev/null +++ b/.changesets/name-the-source-of-each-config-option.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: change +--- + +The `appsignal diagnose` report now names where each configuration option's value came from. An option set from more than one source lists the value from each source. diff --git a/.changesets/report-each-kind-of-parameters-separately.md b/.changesets/report-each-kind-of-parameters-separately.md new file mode 100644 index 00000000..3fab4a16 --- /dev/null +++ b/.changesets/report-each-kind-of-parameters-separately.md @@ -0,0 +1,8 @@ +--- +bump: patch +type: change +--- + +In collector mode, the Django and Flask instrumentation now reports a request's query string as query parameters rather than as a request payload. So `filter_request_query_parameters` and `send_request_query_parameters` apply to a query string, and `filter_request_payload` and `send_request_payload` apply to a Django request's body. + +In agent mode, a Flask application's query parameters are now reported on their own, rather than nested under an `args` key. diff --git a/.changesets/report-the-configured-response-headers.md b/.changesets/report-the-configured-response-headers.md new file mode 100644 index 00000000..da826a18 --- /dev/null +++ b/.changesets/report-the-configured-response-headers.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: add +--- + +Report the response headers listed in the `response_headers` configuration option when a collector is used. diff --git a/conftest.py b/conftest.py index f1f4110e..1397f35a 100644 --- a/conftest.py +++ b/conftest.py @@ -25,6 +25,7 @@ from appsignal.heartbeat import _heartbeat_class_warning, _heartbeat_helper_warning from appsignal.internal_logger import _reset_logger from appsignal.opentelemetry import METRICS_PREFERRED_TEMPORALITY, _providers +from appsignal.tracing import _set_params_warning @pytest.fixture(scope="function", autouse=True) @@ -116,6 +117,11 @@ def reset_global_client() -> Any: _reset_client() +@pytest.fixture(scope="function", autouse=True) +def reset_set_params_warning() -> Any: + _set_params_warning.reset() + + @pytest.fixture(scope="function", autouse=True) def reset_checkins() -> Any: yield diff --git a/src/appsignal/__init__.py b/src/appsignal/__init__.py index 3f7e47ca..f23ccdca 100644 --- a/src/appsignal/__init__.py +++ b/src/appsignal/__init__.py @@ -14,10 +14,13 @@ set_category, set_custom_data, set_error, + set_function_parameters, set_header, set_name, set_namespace, set_params, + set_request_payload, + set_request_query_parameters, set_root_name, set_session_data, set_sql_body, @@ -38,11 +41,14 @@ "set_category", "set_custom_data", "set_error", + "set_function_parameters", "set_gauge", "set_header", "set_name", "set_namespace", "set_params", + "set_request_payload", + "set_request_query_parameters", "set_root_name", "set_session_data", "set_sql_body", diff --git a/src/appsignal/_headers.py b/src/appsignal/_headers.py new file mode 100644 index 00000000..ce78bb5b --- /dev/null +++ b/src/appsignal/_headers.py @@ -0,0 +1,14 @@ +from __future__ import annotations + + +# The OpenTelemetry SDK writes header attribute names with underscores, where +# the semantic conventions the collector compares against use dashes. +def normalize_header(name: str) -> str: + return name.lower().replace("_", "-") + + +def normalize_headers(names: list[str] | None) -> list[str] | None: + if names is None: + return None + + return [normalize_header(name) for name in names] diff --git a/src/appsignal/_once.py b/src/appsignal/_once.py new file mode 100644 index 00000000..faa3aec4 --- /dev/null +++ b/src/appsignal/_once.py @@ -0,0 +1,26 @@ +from __future__ import annotations + +from typing import Any, Callable + +from . import internal_logger as logger + + +class _Once: + def __init__(self, func: Callable[..., None], *args: Any, **kwargs: Any) -> None: + self.called = False + self.func = func + self.args = args + self.kwargs = kwargs + + def __call__(self) -> None: + if not self.called: + self.called = True + self.func(*self.args, **self.kwargs) + + def reset(self) -> None: + self.called = False + + +def _warn_logger_and_stdout(msg: str) -> None: + logger.warning(msg) + print(f"appsignal WARNING: {msg}") diff --git a/src/appsignal/cli/diagnose.py b/src/appsignal/cli/diagnose.py index 9b24f9ee..fa09e91e 100644 --- a/src/appsignal/cli/diagnose.py +++ b/src/appsignal/cli/diagnose.py @@ -8,11 +8,11 @@ from argparse import ArgumentParser from pathlib import Path from sys import stderr -from typing import Any +from typing import Any, cast from ..__about__ import __version__ from ..agent import Agent -from ..config import Config +from ..config import SOURCE_ORDER, Config from ..push_api_key_validator import PushApiKeyValidator from ..transmitter import transmit from .command import AppsignalCLICommand @@ -311,12 +311,31 @@ def _configuration_information(self) -> None: print("Configuration") for key in self.config.options: - print(f" {key}: {self.config.options[key]!r}") # type: ignore + value = self.config.options[key] # type: ignore + print(f" {key}: {value!r}{self._config_sources_label(key)}") print() print("Read more about how the diagnose config output is rendered") print("https://docs.appsignal.com/python/command-line/diagnose.html") + def _config_sources_label(self, option: str) -> str: + sources = cast(dict, self.config.sources) + names = [name for name in SOURCE_ORDER if option in sources[name]] + + if names == ["default"]: + return "" + + if len(names) == 1: + return f" (Loaded from: {names[0]})" + + width = max(len(name) for name in names) + 1 + lines = ["", " Sources:"] + for name in names: + label = f"{name}:".ljust(width) + lines.append(f" {label} {sources[name][option]!r}") + + return "\n".join(lines) + def _validation_information(self) -> None: validation_report: Any = self.report["validation"] print("Validation") diff --git a/src/appsignal/config.py b/src/appsignal/config.py index 5ffe2971..9718026f 100644 --- a/src/appsignal/config.py +++ b/src/appsignal/config.py @@ -68,11 +68,25 @@ class Options(TypedDict, total=False): class Sources(TypedDict): default: Options + derived: Options system: Options initial: Options environment: Options +SOURCE_ORDER: list[str] = [ + "default", + "derived", + "system", + "environment", + "initial", +] + +SOURCES_ABOVE_DERIVED: list[str] = SOURCE_ORDER[SOURCE_ORDER.index("derived") + 1 :] + +APPLICATION_SOURCES: list[str] = ["environment", "initial"] + + class Config: valid: bool sources: Sources @@ -84,6 +98,18 @@ class Config: DEFAULT_CONFIG = Options( ca_file_path=CA_FILE_PATH, diagnose_endpoint="https://appsignal.com/diag", + dns_servers=[], + filter_attributes=[], + filter_function_parameters=[], + filter_parameters=[], + filter_request_payload=[], + filter_request_query_parameters=[], + filter_session_data=[], + ignore_actions=[], + ignore_errors=[], + ignore_logs=[], + ignore_namespaces=[], + response_headers=[], enable_host_metrics=True, enable_minutely_probes=True, enable_nginx_metrics=False, @@ -96,7 +122,10 @@ class Config: logging_endpoint="https://appsignal-endpoint.net", opentelemetry_port=8099, send_environment_metadata=True, + send_function_parameters=True, send_params=True, + send_request_payload=True, + send_request_query_parameters=True, send_session_data=True, request_headers=[ "accept", @@ -133,22 +162,40 @@ class Config: List[DefaultInstrumentation], list(get_args(DefaultInstrumentation)) ) + DEPRECATED_COLLECTOR_OPTIONS: ClassVar[dict[str, list[str]]] = { + "filter_parameters": [ + "filter_request_payload", + "filter_function_parameters", + "filter_request_query_parameters", + ], + "send_params": [ + "send_request_payload", + "send_request_query_parameters", + "send_function_parameters", + ], + } + def __init__(self, options: Options | None = None) -> None: self.valid = False system = Config.load_from_system() self.sources = Sources( default=self.DEFAULT_CONFIG, + derived=Options(), system=system, initial=without_none_overrides(options or Options(), system), environment=Config.load_from_environment(), ) + self._merge_sources() + self.sources["derived"] = self._determine_derived() + self._merge_sources() + self._validate() + + def _merge_sources(self) -> None: + sources = cast(dict, self.sources) final_options = Options() - final_options.update(self.sources["default"]) - final_options.update(self.sources["system"]) - final_options.update(self.sources["environment"]) - final_options.update(self.sources["initial"]) + for source in SOURCE_ORDER: + final_options.update(sources[source]) self.options = final_options - self._validate() def is_active(self) -> bool: return self.valid and self.option("active") @@ -429,8 +476,34 @@ def _validate(self) -> None: if len(push_api_key.strip()) > 0: self.valid = True + def _determine_derived(self) -> Options: + derived: dict = {} + + for option, replacements in self.DEPRECATED_COLLECTOR_OPTIONS.items(): + if not self._filter_user_modified_options([option]): + continue + + for replacement in replacements: + if self._set_above_derived(replacement): + continue + + derived[replacement] = self.option(option) + + return cast(Options, derived) + + def _user_set(self, option: str) -> bool: + return self._set_by_any(APPLICATION_SOURCES, option) + + def _set_above_derived(self, option: str) -> bool: + return self._set_by_any(SOURCES_ABOVE_DERIVED, option) + + def _set_by_any(self, source_names: list[str], option: str) -> bool: + sources = cast(dict, self.sources) + return any(option in sources[name] for name in source_names) + def warn(self) -> None: if self.should_use_collector(): + self._warn_deprecated_collector_options() self._warn_agent_exclusive_options() else: self._warn_collector_exclusive_options() @@ -442,21 +515,10 @@ def warn(self) -> None: # nothing, because the collector receives that data instead. def _warn_agent_exclusive_options(self) -> None: exclusive_options = [ - "filter_parameters", "opentelemetry_port", - "send_params", ] option_specific_warnings = { - "filter_parameters": ( - "Use the 'filter_attributes', 'filter_function_parameters'," - " 'filter_request_payload' and 'filter_request_query_parameters'" - " configuration options instead." - ), - "send_params": ( - "Use the 'send_function_parameters', 'send_request_payload'" - " and 'send_request_query_parameters' configuration options instead." - ), "opentelemetry_port": ( "Set the collector's OpenTelemetry port as part of the" " 'collector_endpoint' configuration option." @@ -479,6 +541,35 @@ def _warn_agent_exclusive_options(self) -> None: " configuration option." ) + def _warn_deprecated_collector_options(self) -> None: + deprecated_options = self._filter_user_modified_options( + list(self.DEPRECATED_COLLECTOR_OPTIONS) + ) + + for option in deprecated_options: + logger.warning(self._deprecated_collector_option_message(option)) + + def _deprecated_collector_option_message(self, option: str) -> str: + replacements = self.DEPRECATED_COLLECTOR_OPTIONS[option] + message = ( + f"The collector is in use. The '{option}' configuration option is" + " deprecated in collector mode. It is replaced by" + f" {quoted_option_list(replacements)}." + ) + + derived = cast(dict, self.sources["derived"]) + values = [ + f"\n {name}: {derived[name]!r}" for name in replacements if name in derived + ] + + if not values: + return message + + return ( + f"{message} Set these options to keep reporting what this" + f" application reports now:{''.join(values)}" + ) + # Emit a warning if collector-exclusive configuration options are used. def _warn_collector_exclusive_options(self) -> None: exclusive_options = [ @@ -498,7 +589,6 @@ def _warn_collector_exclusive_options(self) -> None: send_warning = "Use the 'send_params' option instead." option_specific_warnings = { - "filter_attributes": filter_warning, "filter_function_parameters": filter_warning, "filter_request_payload": filter_warning, "filter_request_query_parameters": filter_warning, @@ -522,23 +612,24 @@ def _warn_collector_exclusive_options(self) -> None: "To use the collector, set the 'collector_endpoint' configuration option." ) - # Filter a list of options, returning a list of those options for which - # a value has been set by the user (through the initialiser or in the - # environment) which differs from that of the default configuration. def _filter_user_modified_options(self, options: list[str]) -> list[str]: return [ option for option in options - if ( - ( - option in self.sources["initial"] - or option in self.sources["environment"] - ) - and self.option(option) != self.sources["default"].get(option) - ) + if self._user_set(option) + and self.option(option) != self.sources["default"].get(option) ] +def quoted_option_list(names: list[str]) -> str: + quoted = [f"'{name}'" for name in names] + + if len(quoted) == 1: + return quoted[0] + + return f"{', '.join(quoted[:-1])} and {quoted[-1]}" + + def parse_bool(value: str | None) -> bool | None: if value is None: return None @@ -570,6 +661,9 @@ def parse_list(value: str | None) -> list[str] | None: if value is None: return None + if not value: + return [] + return value.split(",") diff --git a/src/appsignal/heartbeat.py b/src/appsignal/heartbeat.py index 1a967423..1561a99a 100644 --- a/src/appsignal/heartbeat.py +++ b/src/appsignal/heartbeat.py @@ -2,34 +2,13 @@ from typing import Any, Callable, TypeVar -from . import internal_logger as logger +from ._once import _Once, _warn_logger_and_stdout from .check_in import Cron, cron T = TypeVar("T") -class _Once: - def __init__(self, func: Callable[..., None], *args: Any, **kwargs: Any) -> None: - self.called = False - self.func = func - self.args = args - self.kwargs = kwargs - - def __call__(self) -> None: - if not self.called: - self.called = True - self.func(*self.args, **self.kwargs) - - def reset(self) -> None: - self.called = False - - -def _warn_logger_and_stdout(msg: str) -> None: - logger.warning(msg) - print(f"appsignal WARNING: {msg}") - - _heartbeat_helper_warning = _Once( _warn_logger_and_stdout, "The helper `heartbeat` has been deprecated. " diff --git a/src/appsignal/opentelemetry.py b/src/appsignal/opentelemetry.py index 2c16e240..89c3ded1 100644 --- a/src/appsignal/opentelemetry.py +++ b/src/appsignal/opentelemetry.py @@ -33,6 +33,7 @@ ) from . import internal_logger as logger +from ._headers import normalize_headers from .config import Config, list_to_env_str @@ -58,15 +59,19 @@ def add_celery_instrumentation(_config: Config) -> None: CeleryInstrumentor().instrument() -def add_django_instrumentation(_config: Config) -> None: +def add_django_instrumentation(config: Config) -> None: from django.http.request import HttpRequest from django.http.response import HttpResponse from opentelemetry.instrumentation.django import DjangoInstrumentor - from .tracing import set_params + from .tracing import set_params, set_request_payload, set_request_query_parameters def response_hook(span: Span, request: HttpRequest, response: HttpResponse) -> None: - set_params({"GET": request.GET, "POST": request.POST}, span) + if config.should_use_collector(): + set_request_query_parameters(request.GET, span) + set_request_payload(request.POST, span) + else: + set_params({"GET": request.GET, "POST": request.POST}, span) DjangoInstrumentor().instrument(response_hook=response_hook) @@ -76,12 +81,12 @@ def add_flask_instrumentation(_config: Config) -> None: from opentelemetry.instrumentation.flask import FlaskInstrumentor - from .tracing import set_params + from .tracing import set_request_query_parameters def request_hook(span: Span, environ: dict[str, str]) -> None: if span and span.is_recording(): query_params = parse_qs(environ.get("QUERY_STRING", "")) - set_params({"args": query_params}, span) + set_request_query_parameters(query_params, span) FlaskInstrumentor().instrument(request_hook=request_hook) @@ -211,13 +216,23 @@ def add_logging_instrumentation(config: Config) -> None: _providers: list[Provider] = [] +# The HTTP instrumentation reports a header only when it is named in one of +# these environment variables. +CAPTURE_HEADERS_ENVIRONMENT_VARIABLES: Mapping[str, str] = { + "request_headers": "OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST", + "response_headers": "OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE", +} + + +def _set_capture_headers(config: Config) -> None: + for option, variable in CAPTURE_HEADERS_ENVIRONMENT_VARIABLES.items(): + headers = list_to_env_str(normalize_headers(config.option(option))) + if headers: + os.environ[variable] = headers + + def start(config: Config) -> None: - # Configure OpenTelemetry request headers config - request_headers = list_to_env_str(config.option("request_headers")) - if request_headers: - os.environ["OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST"] = ( - request_headers - ) + _set_capture_headers(config) _start_tracer(config) _start_metrics(config) @@ -367,8 +382,12 @@ def _resource(config: Config) -> Resource: "appsignal.config.ignore_namespaces": config.options.get( "ignore_namespaces" ), - "appsignal.config.response_headers": config.options.get("response_headers"), - "appsignal.config.request_headers": config.options.get("request_headers"), + "appsignal.config.response_headers": normalize_headers( + config.options.get("response_headers") + ), + "appsignal.config.request_headers": normalize_headers( + config.options.get("request_headers") + ), "appsignal.config.send_function_parameters": config.options.get( "send_function_parameters" ), diff --git a/src/appsignal/tracing.py b/src/appsignal/tracing.py index 57aacab2..b0f1942d 100644 --- a/src/appsignal/tracing.py +++ b/src/appsignal/tracing.py @@ -9,6 +9,8 @@ from opentelemetry.trace import Status, StatusCode from . import internal_logger as logger +from ._headers import normalize_header +from ._once import _Once, _warn_logger_and_stdout if TYPE_CHECKING: @@ -62,17 +64,44 @@ def _use_collector() -> bool: return config is not None and config.should_use_collector() -def set_params(params: Any, span: Span | None = None) -> None: - # The collector and server recognize `appsignal.request.payload` for request - # body / merged parameters; the agent recognizes `appsignal.request.parameters`. +def _set_params(collector_attribute: str, params: Any, span: Span | None) -> None: attribute = ( - "appsignal.request.payload" - if _use_collector() - else "appsignal.request.parameters" + collector_attribute if _use_collector() else "appsignal.request.parameters" ) _set_serialised_attribute(attribute, params, span) +def set_request_payload(payload: Any, span: Span | None = None) -> None: + _set_params("appsignal.request.payload", payload, span) + + +def set_request_query_parameters( + query_parameters: Any, span: Span | None = None +) -> None: + _set_params("appsignal.request.query_parameters", query_parameters, span) + + +def set_function_parameters(parameters: Any, span: Span | None = None) -> None: + _set_params("appsignal.function.parameters", parameters, span) + + +_set_params_warning = _Once( + _warn_logger_and_stdout, + "The helper `set_params` is deprecated when a collector is used. It does " + "not say which kind of parameters it is given, so everything it reports " + "becomes the request payload. Use `set_request_payload`, " + "`set_request_query_parameters` or `set_function_parameters` instead, in " + "order to remove this message.", +) + + +def set_params(params: Any, span: Span | None = None) -> None: + if _use_collector(): + _set_params_warning() + + set_request_payload(params, span) + + def set_session_data(session_data: Any, span: Span | None = None) -> None: _set_serialised_attribute("appsignal.request.session_data", session_data, span) @@ -89,8 +118,12 @@ def set_header(header: str, value: Any, span: Span | None = None) -> None: # The collector and server read request headers from the OpenTelemetry # semantic-convention prefix `http.request.header`; the agent reads them # from `appsignal.request.headers`. - prefix = "http.request.header" if _use_collector() else "appsignal.request.headers" - _set_prefixed_attribute(prefix, header, value, span) + if _use_collector(): + _set_prefixed_attribute( + "http.request.header", normalize_header(header), value, span + ) + else: + _set_prefixed_attribute("appsignal.request.headers", header, value, span) def set_name(name: str, span: Span | None = None) -> None: diff --git a/tests/cli/test_diagnose.py b/tests/cli/test_diagnose.py index 1224ae17..9903a40a 100644 --- a/tests/cli/test_diagnose.py +++ b/tests/cli/test_diagnose.py @@ -116,3 +116,27 @@ def test_diagnose_with_missing_paths(mocker, capfd): out, err = capfd.readouterr() assert "Exists?: False" in out + + +def test_diagnose_names_where_a_value_came_from(mocker, capfd): + os.environ["APPSIGNAL_APP_ENV"] = "production" + os.environ["APPSIGNAL_HOST_ROLE"] = "worker" + + main(["diagnose", "--no-send-report"]) + + out, err = capfd.readouterr() + assert " host_role: 'worker' (Loaded from: environment)" in out + assert ( + " environment: 'production'\n" + " Sources:\n" + " default: 'development'\n" + " environment: 'production'\n" + ) in out + + +def test_diagnose_says_nothing_about_an_option_left_at_its_default(mocker, capfd): + main(["diagnose", "--no-send-report"]) + + out, err = capfd.readouterr() + assert " log: 'file'\n" in out + assert " log: 'file' (Loaded from" not in out diff --git a/tests/diagnose b/tests/diagnose index 30f1c121..5981d41e 160000 --- a/tests/diagnose +++ b/tests/diagnose @@ -1 +1 @@ -Subproject commit 30f1c121a4960999fccf02d18317e99edeb0d320 +Subproject commit 5981d41e6203ba65529550d9e3f982e9e84c6715 diff --git a/tests/test_config.py b/tests/test_config.py index 56a0abd8..698b3da0 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -2,6 +2,7 @@ import os import socket +from typing import cast import pytest @@ -252,6 +253,7 @@ def test_environ_source(): assert config.sources["environment"] == env_options final_options = Options() final_options.update(config.sources["default"]) + final_options.update(config.sources["derived"]) final_options.update(config.sources["system"]) final_options.update(env_options) assert config.options == final_options @@ -307,6 +309,96 @@ def test_environ_source_disable_default_instrumentations_bool(): assert config.options["disable_default_instrumentations"] is expected +def test_environment_source_reads_an_empty_variable_as_an_empty_list(): + os.environ["APPSIGNAL_RESPONSE_HEADERS"] = "" + + config = Config() + + assert config.option("response_headers") == [] + + +def test_a_replacement_defaults_to_what_its_option_derives_to(): + # Deriving never runs for an option left at its default, so a replacement + # has to default to what deriving from that default would give. Otherwise + # an application reports one thing while its own default says another. + defaults = cast(dict, Config.DEFAULT_CONFIG) + + for option, replacements in Config.DEPRECATED_COLLECTOR_OPTIONS.items(): + for replacement in replacements: + assert defaults[replacement] == defaults[option] + + +def test_derived_source_is_empty_when_nothing_is_configured(): + config = Config() + + assert config.sources["derived"] == {} + + +def test_derived_source_is_empty_when_an_option_is_set_to_its_default(): + config = Config(Options(send_params=True)) + + assert config.sources["derived"] == {} + + +def test_derives_the_filter_options_from_filter_parameters(): + config = Config(Options(filter_parameters=["password", "secret"])) + + assert config.sources["derived"] == Options( + filter_request_payload=["password", "secret"], + filter_function_parameters=["password", "secret"], + filter_request_query_parameters=["password", "secret"], + ) + assert config.option("filter_request_payload") == ["password", "secret"] + assert config.option("filter_function_parameters") == ["password", "secret"] + assert config.option("filter_request_query_parameters") == ["password", "secret"] + + +def test_derives_the_send_options_from_send_params(): + os.environ["APPSIGNAL_SEND_PARAMS"] = "false" + + config = Config() + + assert config.sources["derived"] == Options( + send_request_payload=False, + send_request_query_parameters=False, + send_function_parameters=False, + ) + assert config.option("send_request_payload") is False + assert config.option("send_request_query_parameters") is False + assert config.option("send_function_parameters") is False + + +def test_does_not_derive_an_option_the_initial_source_sets(): + config = Config( + Options( + filter_parameters=["password"], + filter_request_payload=["token"], + ) + ) + + assert "filter_request_payload" not in config.sources["derived"] + assert config.option("filter_request_payload") == ["token"] + assert config.option("filter_function_parameters") == ["password"] + + +def test_does_not_derive_an_option_the_environment_source_sets(): + os.environ["APPSIGNAL_FILTER_PARAMETERS"] = "password" + os.environ["APPSIGNAL_FILTER_FUNCTION_PARAMETERS"] = "token" + + config = Config() + + assert "filter_function_parameters" not in config.sources["derived"] + assert config.option("filter_function_parameters") == ["token"] + assert config.option("filter_request_payload") == ["password"] + + +def test_derives_an_option_set_to_an_empty_list(): + config = Config(Options(send_params=False, filter_request_payload=[])) + + assert config.option("filter_request_payload") == [] + assert config.option("send_request_payload") is False + + def test_set_private_environ(): cwdir = os.getcwd() config = Config( @@ -622,9 +714,7 @@ def config_builder() -> Config: return Config( Options( collector_endpoint="http://localhost:4318", - filter_parameters=["password"], opentelemetry_port="9999", - send_params=False, ) ) @@ -644,9 +734,7 @@ def config_builder() -> Config: warning_messages = [call.args[0] for call in mock_warning.call_args_list] agent_exclusive_options = [ - "filter_parameters", "opentelemetry_port", - "send_params", ] for option in agent_exclusive_options: @@ -676,9 +764,9 @@ def config_builder() -> Config: filter_request_query_parameters=["query1"], ignore_logs=["^log1"], response_headers=["x-response"], - send_function_parameters=True, - send_request_payload=True, - send_request_query_parameters=True, + send_function_parameters=False, + send_request_payload=False, + send_request_query_parameters=False, service_name="my-service", ) ) @@ -723,7 +811,7 @@ def config_builder() -> Config: ) -def test_warn_filter_parameters_emits_specific_advice(mocker): +def test_warn_filter_parameters_is_deprecated(mocker): mock_warning = mocker.patch("appsignal.internal_logger.warning") config = Config( @@ -737,15 +825,19 @@ def test_warn_filter_parameters_emits_specific_advice(mocker): warning_messages = [call.args[0] for call in mock_warning.call_args_list] - assert any( - "Use the 'filter_attributes', 'filter_function_parameters'," - " 'filter_request_payload' and 'filter_request_query_parameters'" - " configuration options instead." in msg - for msg in warning_messages - ), "Expected specific advice for 'filter_parameters' not found" + assert warning_messages == [ + "The collector is in use. The 'filter_parameters' configuration option" + " is deprecated in collector mode. It is replaced by" + " 'filter_request_payload', 'filter_function_parameters' and" + " 'filter_request_query_parameters'. Set these options to keep" + " reporting what this application reports now:" + "\n filter_request_payload: ['password']" + "\n filter_function_parameters: ['password']" + "\n filter_request_query_parameters: ['password']" + ] -def test_warn_send_params_emits_specific_advice(mocker): +def test_warn_send_params_is_deprecated(mocker): mock_warning = mocker.patch("appsignal.internal_logger.warning") config = Config( @@ -759,11 +851,41 @@ def test_warn_send_params_emits_specific_advice(mocker): warning_messages = [call.args[0] for call in mock_warning.call_args_list] - assert any( - "Use the 'send_function_parameters', 'send_request_payload'" - " and 'send_request_query_parameters' configuration options instead." in msg - for msg in warning_messages - ), "Expected specific advice for 'send_params' not found" + assert warning_messages == [ + "The collector is in use. The 'send_params' configuration option is" + " deprecated in collector mode. It is replaced by" + " 'send_request_payload', 'send_request_query_parameters' and" + " 'send_function_parameters'. Set these options to keep reporting what" + " this application reports now:" + "\n send_request_payload: False" + "\n send_request_query_parameters: False" + "\n send_function_parameters: False" + ] + + +def test_warn_deprecated_option_without_derived_values(mocker): + mock_warning = mocker.patch("appsignal.internal_logger.warning") + + config = Config( + Options( + collector_endpoint="http://localhost:4318", + send_params=False, + send_request_payload=True, + send_request_query_parameters=True, + send_function_parameters=True, + ) + ) + + config.warn() + + warning_messages = [call.args[0] for call in mock_warning.call_args_list] + + assert warning_messages == [ + "The collector is in use. The 'send_params' configuration option is" + " deprecated in collector mode. It is replaced by" + " 'send_request_payload', 'send_request_query_parameters' and" + " 'send_function_parameters'." + ] def test_warn_opentelemetry_port_emits_specific_advice(mocker): @@ -787,6 +909,15 @@ def test_warn_opentelemetry_port_emits_specific_advice(mocker): ), "Expected specific advice for 'opentelemetry_port' not found" +def test_warn_no_warnings_for_options_appsignal_derived(mocker): + mock_warning = mocker.patch("appsignal.internal_logger.warning") + + config = Config(Options(filter_parameters=["password"], send_params=False)) + config.warn() + + assert mock_warning.call_count == 0 + + def test_warn_collector_filter_options_emit_use_filter_parameters_advice(mocker): mock_warning = mocker.patch("appsignal.internal_logger.warning") @@ -803,7 +934,7 @@ def test_warn_collector_filter_options_emit_use_filter_parameters_advice(mocker) warning_messages = [call.args[0] for call in mock_warning.call_args_list] - assert warning_messages.count("Use the 'filter_parameters' option instead.") == 4 + assert warning_messages.count("Use the 'filter_parameters' option instead.") == 3 def test_warn_collector_send_options_emit_use_send_params_advice(mocker): @@ -811,9 +942,9 @@ def test_warn_collector_send_options_emit_use_send_params_advice(mocker): config = Config( Options( - send_function_parameters=True, - send_request_payload=True, - send_request_query_parameters=True, + send_function_parameters=False, + send_request_payload=False, + send_request_query_parameters=False, ) ) diff --git a/tests/test_opentelemetry.py b/tests/test_opentelemetry.py index 2c1774a0..e80e6936 100644 --- a/tests/test_opentelemetry.py +++ b/tests/test_opentelemetry.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os from typing import List, cast from unittest.mock import Mock @@ -7,6 +8,8 @@ from appsignal.opentelemetry import ( _exporter_session, _providers, + _resource, + _set_capture_headers, _start_logging, _start_metrics, _start_tracer, @@ -15,6 +18,72 @@ ) +REQUEST_HEADERS_VARIABLE = "OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST" +RESPONSE_HEADERS_VARIABLE = "OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE" + + +def test_set_capture_headers(): + config = Config( + Options( + request_headers=["accept", "x-request-id"], + response_headers=["content-type"], + ) + ) + + _set_capture_headers(config) + + assert os.environ[REQUEST_HEADERS_VARIABLE] == "accept,x-request-id" + assert os.environ[RESPONSE_HEADERS_VARIABLE] == "content-type" + + +def test_set_capture_headers_normalizes_the_names(): + config = Config( + Options( + request_headers=["Accept_Charset", "X-Custom-Header"], + response_headers=["Content_Type"], + ) + ) + + _set_capture_headers(config) + + assert os.environ[REQUEST_HEADERS_VARIABLE] == "accept-charset,x-custom-header" + assert os.environ[RESPONSE_HEADERS_VARIABLE] == "content-type" + + +def test_set_capture_headers_when_the_options_are_empty(): + config = Config(Options(request_headers=[], response_headers=[])) + + _set_capture_headers(config) + + assert REQUEST_HEADERS_VARIABLE not in os.environ + assert RESPONSE_HEADERS_VARIABLE not in os.environ + + +def test_set_capture_headers_when_the_options_are_unset(): + config = Config(Options(request_headers=None)) + + _set_capture_headers(config) + + assert REQUEST_HEADERS_VARIABLE not in os.environ + assert RESPONSE_HEADERS_VARIABLE not in os.environ + + +def test_resource_normalizes_the_header_options(): + config = Config( + Options( + name="MyApp", + push_api_key="0000-0000-0000-0000", + request_headers=["Accept_Charset"], + response_headers=["Content_Type"], + ) + ) + + attributes = _resource(config).attributes + + assert attributes["appsignal.config.request_headers"] == ("accept-charset",) + assert attributes["appsignal.config.response_headers"] == ("content-type",) + + def raise_module_not_found_error(_config: Config) -> None: raise ModuleNotFoundError diff --git a/tests/test_tracing.py b/tests/test_tracing.py index c15c891b..8a214f58 100644 --- a/tests/test_tracing.py +++ b/tests/test_tracing.py @@ -11,10 +11,13 @@ set_category, set_custom_data, set_error, + set_function_parameters, set_header, set_name, set_namespace, set_params, + set_request_payload, + set_request_query_parameters, set_root_name, set_session_data, set_sql_body, @@ -114,6 +117,80 @@ def test_set_params_collector_mode(spans): assert "appsignal.request.parameters" not in attributes +def test_set_each_kind_of_params_collector_mode(spans): + Client( + active=True, + name="MyApp", + push_api_key="0000-0000-0000-0000", + collector_endpoint="https://custom-endpoint.appsignal.com", + ) + + with tracer.start_as_current_span("span"): + set_request_payload({"id": 123}) + set_request_query_parameters({"page": 2}) + set_function_parameters({"job": "argument"}) + + attributes = dict(spans()[0].attributes) + assert attributes["appsignal.request.payload"] == '{"id": 123}' + assert attributes["appsignal.request.query_parameters"] == '{"page": 2}' + assert attributes["appsignal.function.parameters"] == '{"job": "argument"}' + assert "appsignal.request.parameters" not in attributes + + +def test_set_each_kind_of_params_agent_mode(spans): + with tracer.start_as_current_span("span"): + set_request_payload({"id": 123}) + + assert dict(spans()[0].attributes) == { + "appsignal.request.parameters": '{"id": 123}' + } + + with tracer.start_as_current_span("span"): + set_request_query_parameters({"page": 2}) + + assert dict(spans()[0].attributes) == { + "appsignal.request.parameters": '{"page": 2}' + } + + with tracer.start_as_current_span("span"): + set_function_parameters({"job": "argument"}) + + assert dict(spans()[0].attributes) == { + "appsignal.request.parameters": '{"job": "argument"}' + } + + +def test_set_params_warns_in_collector_mode(spans, mocker): + mock_warning = mocker.patch("appsignal.internal_logger.warning") + + Client( + active=True, + name="MyApp", + push_api_key="0000-0000-0000-0000", + collector_endpoint="https://custom-endpoint.appsignal.com", + ) + + with tracer.start_as_current_span("span"): + set_params({"id": 123}) + set_params({"id": 456}) + + warning_messages = [call.args[0] for call in mock_warning.call_args_list] + + assert len(warning_messages) == 1 + assert "`set_params` is deprecated when a collector is used" in ( + warning_messages[0] + ) + + +def test_set_params_does_not_warn_in_agent_mode(spans, mocker): + mock_warning = mocker.patch("appsignal.internal_logger.warning") + + with tracer.start_as_current_span("span"): + set_params({"id": 123}) + + assert mock_warning.call_count == 0 + + def test_set_header_collector_mode(spans): Client( active=True, @@ -130,6 +207,29 @@ def test_set_header_collector_mode(spans): assert "appsignal.request.headers.content-type" not in attributes +def test_set_header_normalizes_the_name_collector_mode(spans): + Client( + active=True, + name="MyApp", + push_api_key="0000-0000-0000-0000", + collector_endpoint="https://custom-endpoint.appsignal.com", + ) + + with tracer.start_as_current_span("span"): + set_header("Content_Type", "application/json") + + attributes = dict(spans()[0].attributes) + assert attributes["http.request.header.content-type"] == "application/json" + + +def test_set_header_agent_mode_keeps_the_name(spans): + with tracer.start_as_current_span("span"): + set_header("Content_Type", "application/json") + + attributes = dict(spans()[0].attributes) + assert attributes["appsignal.request.headers.Content_Type"] == "application/json" + + def test_set_sql_body_collector_mode(spans): Client( active=True,