diff --git a/swh/core/api/gunicorn_config.py b/swh/core/api/gunicorn_config.py --- a/swh/core/api/gunicorn_config.py +++ b/swh/core/api/gunicorn_config.py @@ -34,7 +34,7 @@ sentry_integrations.append(FlaskIntegration()) init_sentry( - default_sentry_dsn, + sentry_dsn=default_sentry_dsn, integrations=sentry_integrations, extra_kwargs=extra_sentry_kwargs, disable_logging_events=disable_logging_events, diff --git a/swh/core/cli/__init__.py b/swh/core/cli/__init__.py --- a/swh/core/cli/__init__.py +++ b/swh/core/cli/__init__.py @@ -132,7 +132,7 @@ signal.signal(signal.SIGTERM, clean_exit_on_signal) signal.signal(signal.SIGINT, clean_exit_on_signal) - init_sentry(sentry_dsn, debug=sentry_debug) + init_sentry(sentry_dsn=sentry_dsn, debug=sentry_debug) set_default_loglevel: Optional[str] = None diff --git a/swh/core/sentry.py b/swh/core/sentry.py --- a/swh/core/sentry.py +++ b/swh/core/sentry.py @@ -3,11 +3,14 @@ # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information +import logging import os from typing import Dict, List, Optional import pkg_resources +logger = logging.getLogger(__name__) + def get_sentry_release(): main_package = os.environ.get("SWH_MAIN_PACKAGE") @@ -18,30 +21,45 @@ return None -def envvar_is_positive(envvar: str) -> bool: - """Check whether a given environment variable looks like a positive boolean value""" - return os.environ.get(envvar, "false").lower() in ("t", "true", "y", "yes", "1") +def override_with_bool_envvar(envvar: str, default: bool) -> bool: + """Override the `default` with the environment variable `envvar` parsed as a boolean""" + envvalue = os.environ.get(envvar, "") + if envvalue.lower() in ("t", "true", "y", "yes", "1"): + return True + elif envvalue.lower() in ("f", "false", "n", "no", "0"): + return False + else: + if envvalue: + logger.warning( + "Could not interpret environment variable %s=%r as boolean, " + "using default value %s", + envvar, + envvalue, + default, + ) + return default def init_sentry( - sentry_dsn: str, + sentry_dsn: Optional[str] = None, *, - debug: Optional[bool] = None, - disable_logging_events: Optional[bool] = None, + environment: Optional[str] = None, + debug: bool = False, + disable_logging_events: bool = False, integrations: Optional[List] = None, extra_kwargs: Optional[Dict] = None, ): """Configure the sentry integration Args: - sentry_dsn: sentry DSN; where sentry report will be sent (if empty, pulled from - :envvar:`SWH_SENTRY_DSN`) - debug: turn on sentry SDK debug mode (if ``None``, pulled from - :envvar:`SWH_SENTRY_DEBUG`) + sentry_dsn: Sentry DSN; where sentry report will be sent. Overridden by + :envvar:`SWH_SENTRY_DSN` + environment: Sentry environment. Overridden by :envvar:`SWH_SENTRY_ENVIRONMENT` + debug: turn on Sentry SDK debug mode. Overridden by :envvar:`SWH_SENTRY_DEBUG` disable_logging_events: if set, disable the automatic reporting of error/exception - log entries as sentry events (if ``None``, pulled from - :envvar:`SWH_SENTRY_DISABLE_LOGGING_EVENTS`) - integrations: list of dedicated sentry integrations objects + log entries as Sentry events. Overridden by + :envvar:`SWH_SENTRY_DISABLE_LOGGING_EVENTS` + integrations: list of dedicated Sentry integrations to include extra_kwargs: dict of additional parameters passed to :func:`sentry_sdk.init` """ @@ -50,13 +68,13 @@ if extra_kwargs is None: extra_kwargs = {} - sentry_dsn = sentry_dsn or os.environ.get("SWH_SENTRY_DSN", "") - environment = os.environ.get("SWH_SENTRY_ENVIRONMENT") + sentry_dsn = os.environ.get("SWH_SENTRY_DSN", sentry_dsn) + environment = os.environ.get("SWH_SENTRY_ENVIRONMENT", environment) - if debug is None: - debug = envvar_is_positive("SWH_SENTRY_DEBUG") - if disable_logging_events is None: - disable_logging_events = envvar_is_positive("SWH_SENTRY_DISABLE_LOGGING_EVENTS") + debug = override_with_bool_envvar("SWH_SENTRY_DEBUG", debug) + disable_logging_events = override_with_bool_envvar( + "SWH_SENTRY_DISABLE_LOGGING_EVENTS", disable_logging_events + ) if sentry_dsn: import sentry_sdk diff --git a/swh/core/tests/test_cli.py b/swh/core/tests/test_cli.py --- a/swh/core/tests/test_cli.py +++ b/swh/core/tests/test_cli.py @@ -12,6 +12,7 @@ from click.testing import CliRunner import pkg_resources import pytest +import yaml help_msg_snippets = ( ( @@ -70,6 +71,18 @@ ) +@pytest.fixture(autouse=True) +def reset_root_loglevel(): + root_level = logging.root.level + yield + logging.root.setLevel(root_level) + + +@pytest.fixture +def patched_dictconfig(mocker): + yield mocker.patch("logging.config.dictConfig", autospec=True) + + def test_swh_help(swhmain): runner = CliRunner() result = runner.invoke(swhmain, ["-h"]) @@ -262,14 +275,11 @@ yield str(tmp_path / "log_config.yml") -def test_log_config(log_config_path, swhmain): +def test_log_config(log_config_path, swhmain, patched_dictconfig): @swhmain.command(name="test") @click.pass_context def swhtest(ctx): - logging.debug("Root log debug") - logging.info("Root log info") - logging.getLogger("dontshowdebug").debug("Not shown") - logging.getLogger("dontshowdebug").info("Shown") + click.echo("Hello SWH!") runner = CliRunner() result = runner.invoke( @@ -282,23 +292,20 @@ ) assert result.exit_code == 0 - assert result.output.strip() == "\n".join( - [ - "custom format:root:DEBUG:Root log debug", - "custom format:root:INFO:Root log info", - "custom format:dontshowdebug:INFO:Shown", - ] + assert result.output.strip() == "Hello SWH!" + + patched_dictconfig.assert_called_once_with( + yaml.safe_load(open(log_config_path, "r")) ) -def test_log_config_log_level_interaction(log_config_path, swhmain): +def test_log_config_log_level_interaction(log_config_path, swhmain, patched_dictconfig): @swhmain.command(name="test") @click.pass_context def swhtest(ctx): - logging.debug("Root log debug") - logging.info("Root log info") - logging.getLogger("dontshowdebug").debug("Not shown") - logging.getLogger("dontshowdebug").info("Shown") + print("Hello SWH!") + + assert logging.root.level != logging.DEBUG runner = CliRunner() result = runner.invoke( @@ -307,19 +314,20 @@ "--log-config", log_config_path, "--log-level", - "INFO", + "DEBUG", "test", ], ) assert result.exit_code == 0 - assert result.output.strip() == "\n".join( - [ - "custom format:root:INFO:Root log info", - "custom format:dontshowdebug:INFO:Shown", - ] + assert result.output.strip() == "Hello SWH!" + + patched_dictconfig.assert_called_once_with( + yaml.safe_load(open(log_config_path, "r")) ) + assert logging.root.level == logging.DEBUG + def test_multiple_log_level_behavior(swhmain): @swhmain.command(name="test") diff --git a/swh/core/tests/test_sentry.py b/swh/core/tests/test_sentry.py --- a/swh/core/tests/test_sentry.py +++ b/swh/core/tests/test_sentry.py @@ -5,9 +5,44 @@ import logging +import pytest from sentry_sdk import capture_message -from swh.core.sentry import init_sentry +from swh.core.sentry import init_sentry, override_with_bool_envvar + + +@pytest.mark.parametrize( + "envvalue,retval", + ( + ("y", True), + ("n", False), + ("0", False), + ("true", True), + ("FaLsE", False), + ("1", True), + ), +) +def test_override_with_bool_envvar(monkeypatch, envvalue: str, retval: bool): + """Test if the override_with_bool_envvar function returns appropriate results""" + envvar = "OVERRIDE_WITH_BOOL_ENVVAR" + monkeypatch.setenv(envvar, envvalue) + for default in (True, False): + assert override_with_bool_envvar(envvar, default) == retval + + +def test_override_with_bool_envvar_logging(monkeypatch, caplog): + envvar = "OVERRIDE_WITH_BOOL_ENVVAR" + monkeypatch.setenv(envvar, "not a boolean env value") + for default in (True, False): + caplog.clear() + assert override_with_bool_envvar(envvar, default) == default + assert len(caplog.records) == 1 + assert ( + "OVERRIDE_WITH_BOOL_ENVVAR='not a boolean env value'" + in caplog.records[0].getMessage() + ) + assert f"using default value {default}" in caplog.records[0].getMessage() + assert caplog.records[0].levelname == "WARNING" def test_sentry():