From 187b396fb12cc452912d8541b0b65bc335cc552b Mon Sep 17 00:00:00 2001 From: Bartosz Blizniak Date: Mon, 26 Oct 2026 11:48:26 +0000 Subject: [PATCH] refactor(ENG-15367): share telemetry opt-outs and the exit wait budget Move the opt-out checks, build environment detection and the Cloudsmith host check out of the Sentry module into core/telemetry_common.py, so error reporting and usage telemetry follow the same rules. The update check now draws its end-of-run wait from a shared 1 second budget, so all background work together never delays a command's exit by more than 1 second. Co-Authored-By: Claude Opus 5.5 --- cloudsmith_cli/conftest.py | 9 ++ cloudsmith_cli/core/telemetry.py | 79 ++-------- cloudsmith_cli/core/telemetry_common.py | 146 ++++++++++++++++++ .../core/tests/test_telemetry_common.py | 68 ++++++++ .../core/tests/test_update_check.py | 26 ++++ cloudsmith_cli/core/update_check.py | 7 +- 6 files changed, 269 insertions(+), 66 deletions(-) create mode 100644 cloudsmith_cli/core/telemetry_common.py create mode 100644 cloudsmith_cli/core/tests/test_telemetry_common.py diff --git a/cloudsmith_cli/conftest.py b/cloudsmith_cli/conftest.py index 5a7075828..610db8459 100644 --- a/cloudsmith_cli/conftest.py +++ b/cloudsmith_cli/conftest.py @@ -46,6 +46,7 @@ def telemetry_config_dir(monkeypatch, tmp_path_factory): monkeypatch.setattr(reader, "config_files", [name]) monkeypatch.setattr(reader, "config_searchpath", [str(path)]) monkeypatch.delenv(telemetry.PROFILE_ENV, raising=False) + monkeypatch.delenv("CLOUDSMITH_CONFIG_FILE", raising=False) return path @@ -88,3 +89,11 @@ def restore_default_proxy(): """Clear the process-wide proxy that ``initialise_session`` sets.""" yield session.set_default_proxy(None) + + +@pytest.fixture(autouse=True) +def reset_wait_budget(): + """Give every test a full shared exit-wait budget.""" + from cloudsmith_cli.core import telemetry_common + + telemetry_common.WaitBudget.shared().reset(1.0) diff --git a/cloudsmith_cli/core/telemetry.py b/cloudsmith_cli/core/telemetry.py index 50adac798..b43447b28 100644 --- a/cloudsmith_cli/core/telemetry.py +++ b/cloudsmith_cli/core/telemetry.py @@ -30,6 +30,16 @@ import click from .certificate_errors import find_certificate_error +from .telemetry_common import CLOUDSMITH_HOST_SUFFIXES as _ALLOWED_HOST_SUFFIXES +from .telemetry_common import ( # noqa: F401 (re-exported for callers and tests) + DO_NOT_TRACK_ENV, + NO_TELEMETRY_ENV, + OPT_OUT_ENVS, + PROFILE_ENV, + config_disabled, + get_environment, + telemetry_disabled, +) #: Production ingest DSN for the dedicated ``cloudsmith-cli`` Sentry project. #: Not a secret: a DSN is a public, write-only ingest key, and every client @@ -41,18 +51,14 @@ #: Set it empty to disable sending. DSN_ENV = "CLOUDSMITH_TELEMETRY_DSN" -NO_TELEMETRY_ENV = "CLOUDSMITH_NO_TELEMETRY" -DO_NOT_TRACK_ENV = "DO_NOT_TRACK" -OPT_OUT_ENVS = (DO_NOT_TRACK_ENV, NO_TELEMETRY_ENV) -_TRUTHY_ENV_VALUES = ("1", "true", "yes") - #: Upper bound on how long a failing command waits for the report to send. FLUSH_TIMEOUT_SECONDS = 2.0 MAX_MESSAGE_LENGTH = 1024 -#: Hosts that may appear in reports; any other host (custom/self-hosted API, -#: proxies, internal registries) is replaced. -_ALLOWED_HOST_SUFFIXES = ("cloudsmith.io", "cloudsmith.com") +# Hosts that may appear in reports are ``_ALLOWED_HOST_SUFFIXES`` (imported +# above); any other host (custom/self-hosted API, proxies, internal +# registries) is replaced. + #: Env vars whose values are treated as secrets wherever they appear. _SECRET_ENV_NAME = re.compile(r"KEY|TOKEN|SECRET|PASSW|CREDENTIAL|AUTH", re.IGNORECASE) _MIN_SECRET_LENGTH = 8 @@ -87,48 +93,6 @@ def format_exception_summary(exc: BaseException) -> str: return f"{type(exc).__name__}: {exc}" -def telemetry_disabled(env: "os._Environ[str] | dict[str, str] | None" = None) -> bool: - """Tell whether the user has opted out of error reporting. - - Either opt-out variable set to ``1``/``true``/``yes`` (case-insensitive) - disables reporting. Any other value, or unset, leaves it on. - """ - env = os.environ if env is None else env - return any( - env.get(name, "").strip().lower() in _TRUTHY_ENV_VALUES for name in OPT_OUT_ENVS - ) - - -PROFILE_ENV = "CLOUDSMITH_PROFILE" - - -def config_disabled() -> bool: - """Tell whether ``telemetry = false`` is set in config.ini. - - Checked two ways, and either one saying ``false`` wins: - - * the options the command loaded, which honour ``--config-file`` and - ``--profile`` - but only exist if the command loads config at all; - * a fresh read of config.ini (``[default]`` plus ``$CLOUDSMITH_PROFILE``), - which covers commands that never load config or failed before doing so. - - Fails closed: if the config cannot be read we cannot confirm the user has - not opted out, so we do not report. - """ - try: - from ..cli import config - - opts = getattr(config.OPTIONS, "value", None) - if opts is not None and opts.telemetry is False: - return True - - probe = config.Options() - config.ConfigReader.load_config(probe, profile=os.environ.get(PROFILE_ENV)) - return probe.telemetry is False - except Exception: # pylint: disable=broad-exception-caught - return True - - def get_dsn(env: "os._Environ[str] | dict[str, str] | None" = None) -> str: """Return the DSN to report to; empty means do not report.""" env = os.environ if env is None else env @@ -235,21 +199,6 @@ def _send(exc: BaseException, dsn: str) -> bool: return event_id is not None -def get_environment(package_file=None, frozen=None): - """Return ``development`` for a source checkout, else ``production``. - - Frozen builds and wheels installed into site-/dist-packages are releases; - anything imported from elsewhere is an editable install or a checkout, so - developers' errors stay out of production triage. - """ - frozen = getattr(sys, "frozen", False) if frozen is None else frozen - if frozen: - return "production" - path = (package_file or os.path.abspath(__file__)).replace("\\", "/") - installed = "/site-packages/" in path or "/dist-packages/" in path - return "production" if installed else "development" - - def _safe(func, default): try: return func() diff --git a/cloudsmith_cli/core/telemetry_common.py b/cloudsmith_cli/core/telemetry_common.py new file mode 100644 index 000000000..98f279130 --- /dev/null +++ b/cloudsmith_cli/core/telemetry_common.py @@ -0,0 +1,146 @@ +# Copyright 2026 Cloudsmith Ltd +"""Helpers shared by error reporting, update checks and usage telemetry. + +Keep this module's imports light (no ``requests``, ``ssl`` or ``urllib3``): +it is imported on every command. +""" + +import os +import sys +from urllib.parse import urlsplit + +NO_TELEMETRY_ENV = "CLOUDSMITH_NO_TELEMETRY" +DO_NOT_TRACK_ENV = "DO_NOT_TRACK" +OPT_OUT_ENVS = (DO_NOT_TRACK_ENV, NO_TELEMETRY_ENV) +_TRUTHY_ENV_VALUES = ("1", "true", "yes") + +PROFILE_ENV = "CLOUDSMITH_PROFILE" +CONFIG_FILE_ENV = "CLOUDSMITH_CONFIG_FILE" + +#: Hosts whose API traffic may be measured; any other host (self-hosted, +#: proxies, internal registries) never sends usage telemetry. +CLOUDSMITH_HOST_SUFFIXES = ("cloudsmith.io", "cloudsmith.com") + + +def telemetry_disabled(env: "os._Environ[str] | dict[str, str] | None" = None) -> bool: + """Tell whether the user has opted out of telemetry (error reporting and usage). + + Either opt-out variable set to ``1``/``true``/``yes`` (case-insensitive) + disables both. Any other value, or unset, leaves them on. + """ + env = os.environ if env is None else env + return any( + env.get(name, "").strip().lower() in _TRUTHY_ENV_VALUES for name in OPT_OUT_ENVS + ) + + +def read_config_file(): + """Read config.ini afresh (``[default]`` plus ``$CLOUDSMITH_PROFILE``). + + The file named by ``$CLOUDSMITH_CONFIG_FILE`` is read first, as the CLI + reads it. Returns new options holding what the files set. Covers commands + that never load config or failed before doing so. Raises if a file cannot + be read; callers fail closed. + """ + from ..cli import config + + # load_config inserts the path into the reader's class-level lists, so + # read through a subclass with copies to leave the CLI's reader as it was. + reader = type( + "ProbeConfigReader", + (config.ConfigReader,), + { + "config_files": list(config.ConfigReader.config_files), + "config_searchpath": list(config.ConfigReader.config_searchpath), + }, + ) + probe = config.Options() + reader.load_config( + probe, + path=os.environ.get(CONFIG_FILE_ENV) or None, + profile=os.environ.get(PROFILE_ENV), + ) + return probe + + +def config_disabled(read_config=read_config_file) -> bool: + """Tell whether ``telemetry = false`` is set in config.ini. + + It turns off telemetry (error reporting and usage). + + Checked two ways, and either one saying ``false`` wins: + + * the options the command loaded, which honour ``--config-file`` and + ``--profile`` - but only exist if the command loads config at all; + * a fresh read of config.ini by ``read_config``, which a caller can pass + to share one read with its own config.ini checks. + + Fails closed: if the config cannot be read we cannot confirm the user has + not opted out, so we send nothing. + """ + try: + from ..cli import config + + opts = getattr(config.OPTIONS, "value", None) + if opts is not None and opts.telemetry is False: + return True + + return read_config().telemetry is False + except Exception: # pylint: disable=broad-exception-caught + return True + + +def get_environment(package_file=None, frozen=None): + """Return ``development`` for a source checkout, else ``production``. + + Frozen builds and wheels installed into site-/dist-packages are releases; + anything imported from elsewhere is an editable install or a checkout, so + developers' errors stay out of production triage. + """ + frozen = getattr(sys, "frozen", False) if frozen is None else frozen + if frozen: + return "production" + path = (package_file or os.path.abspath(__file__)).replace("\\", "/") + installed = "/site-packages/" in path or "/dist-packages/" in path + return "production" if installed else "development" + + +def is_cloudsmith_host(url_or_host): + """Tell whether ``url_or_host`` is a Cloudsmith host (empty means default).""" + if not url_or_host: + return True + text = url_or_host.strip() + host = urlsplit(text if "://" in text else f"//{text}").hostname or "" + host = host.lower().rstrip(".") + return any( + host == suffix or host.endswith("." + suffix) + for suffix in CLOUDSMITH_HOST_SUFFIXES + ) + + +class WaitBudget: + """One wait budget per process for background work at command exit. + + The update check and usage telemetry both wait for a background thread + when the command ends; together they must not exceed ``total`` seconds. + """ + + _shared = None + + def __init__(self, total=1.0): + self._remaining = total + + @classmethod + def shared(cls): + if cls._shared is None: + cls._shared = cls() + return cls._shared + + def remaining(self): + return max(0.0, self._remaining) + + def spend(self, seconds): + self._remaining = max(0.0, self._remaining - max(0.0, seconds)) + + def reset(self, total=1.0): + self._remaining = total diff --git a/cloudsmith_cli/core/tests/test_telemetry_common.py b/cloudsmith_cli/core/tests/test_telemetry_common.py new file mode 100644 index 000000000..da0dce56c --- /dev/null +++ b/cloudsmith_cli/core/tests/test_telemetry_common.py @@ -0,0 +1,68 @@ +"""Tests for the helpers shared by error reporting, update checks and usage.""" + +import pytest + +from cloudsmith_cli.core import telemetry, telemetry_common + + +@pytest.mark.parametrize( + "value,expected", + [ + ("https://api.cloudsmith.io", True), + ("https://api.cloudsmith.com/v1/", True), + ("api.cloudsmith.io", True), + ("https://cloudsmith.io", True), + ("https://evilcloudsmith.io", False), + ("https://cloudsmith.io.evil.com", False), + ("https://artifacts.example.com", False), + ("", True), + (None, True), + ], +) +def test_is_cloudsmith_host(value, expected): + # Empty or missing means the default host, which is Cloudsmith's. + assert telemetry_common.is_cloudsmith_host(value) is expected + + +def test_sentry_module_reexports_shared_names(): + assert telemetry.telemetry_disabled is telemetry_common.telemetry_disabled + assert telemetry.config_disabled is telemetry_common.config_disabled + assert telemetry.get_environment is telemetry_common.get_environment + assert telemetry.OPT_OUT_ENVS == telemetry_common.OPT_OUT_ENVS + + +def test_wait_budget_is_shared_and_never_negative(): + budget = telemetry_common.WaitBudget.shared() + budget.reset(1.0) + budget.spend(0.4) + assert telemetry_common.WaitBudget.shared().remaining() == pytest.approx(0.6) + budget.spend(5.0) + assert budget.remaining() == 0.0 + + +def test_read_config_file_honours_the_config_file_env( + telemetry_config_dir, tmp_path, monkeypatch +): + from cloudsmith_cli.cli import config + + (telemetry_config_dir / "config.ini").write_text("[default]\ntelemetry = true\n") + path = tmp_path / "optout.ini" + path.write_text("[default]\ntelemetry = false\n") + files = list(config.ConfigReader.config_files) + searchpath = list(config.ConfigReader.config_searchpath) + + monkeypatch.setenv(telemetry_common.CONFIG_FILE_ENV, str(path)) + assert telemetry_common.read_config_file().telemetry is False + # The CLI's own reader is left as it was for the rest of the run. + assert config.ConfigReader.config_files == files + assert config.ConfigReader.config_searchpath == searchpath + + monkeypatch.delenv(telemetry_common.CONFIG_FILE_ENV) + assert telemetry_common.read_config_file().telemetry is True + + +def test_config_file_env_matches_the_option_declaration(): + from cloudsmith_cli.cli.commands import main + + envvars = {p.name: p.envvar for p in main.main.params} + assert envvars["config_file"] == telemetry_common.CONFIG_FILE_ENV diff --git a/cloudsmith_cli/core/tests/test_update_check.py b/cloudsmith_cli/core/tests/test_update_check.py index 1620f9d92..cd9d54b57 100644 --- a/cloudsmith_cli/core/tests/test_update_check.py +++ b/cloudsmith_cli/core/tests/test_update_check.py @@ -705,3 +705,29 @@ def writer(): thread.start() update_check._finish_and_notify(thread, self._Opts(), "list", now=NOW + 1) assert NEWER_VERSION in capsys.readouterr().err + + +def test_finish_spends_shared_wait_budget(monkeypatch): + import threading + import time + + from cloudsmith_cli.core import telemetry_common, update_check + + budget = telemetry_common.WaitBudget.shared() + budget.reset(1.0) + gate = threading.Event() + thread = threading.Thread(target=gate.wait, args=(5,), daemon=True) + thread.start() + monkeypatch.setattr(update_check, "notice_suppressed", lambda *a: True) + + started = time.monotonic() + update_check._finish_and_notify( + thread, + opts=object(), # ty: ignore[invalid-argument-type] + invoked=None, + ) + elapsed = time.monotonic() - started + gate.set() + + assert 0.9 <= elapsed < 1.5 + assert budget.remaining() < 0.1 diff --git a/cloudsmith_cli/core/update_check.py b/cloudsmith_cli/core/update_check.py index b180d8d82..a0e7838de 100644 --- a/cloudsmith_cli/core/update_check.py +++ b/cloudsmith_cli/core/update_check.py @@ -434,7 +434,12 @@ def _finish_and_notify( interactive run can still speak. """ if thread is not None: - thread.join(BACKGROUND_JOIN_TIMEOUT_SECONDS) + from .telemetry_common import WaitBudget + + budget = WaitBudget.shared() + started = time.monotonic() + thread.join(min(BACKGROUND_JOIN_TIMEOUT_SECONDS, budget.remaining())) + budget.spend(time.monotonic() - started) if notice_suppressed(getattr(opts, "output", None), invoked): return state = read_cached_state()