From 9ddbdf2313eb4a77ad6cff3a0ae5eb0264cf7c57 Mon Sep 17 00:00:00 2001 From: chopratejas Date: Tue, 5 May 2026 11:30:10 -0700 Subject: [PATCH] fix(telemetry): honour HEADROOM_TELEMETRY=off in /v1/telemetry collector (#390) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two telemetry env vars existed in the codebase, only one was wired to the /v1/telemetry endpoint: - HEADROOM_TELEMETRY (documented opt-out, used by Supabase beacon and the Telemetry-Warning notice). Honoured: off / false / 0 / no / disable / disabled. - HEADROOM_TELEMETRY_DISABLED (undocumented). Was the ONLY one the collector singleton consulted. Accepted only "1" or "true". A user setting HEADROOM_TELEMETRY=off (the value from the docs) saw /v1/telemetry continue to report enabled=true. Fix: collector calls is_telemetry_enabled() (the same predicate the beacon uses), so both env vars take effect. HEADROOM_TELEMETRY_DISABLED remains accepted for back-compat. Tests: parametrized regression covering all six documented OFF values and a positive-path test for explicit ON / unset. Drive-by: convert two pre-existing isinstance(v, (int, float)) to isinstance(v, int | float) so the pre-commit ruff hook (which scans the whole file) stops flagging UP038 on every commit that touches this file. Addresses #390 (do not auto-close — needs user confirmation in their own environment after the next release). --- headroom/telemetry/collector.py | 19 ++++++++++++++---- tests/test_telemetry.py | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/headroom/telemetry/collector.py b/headroom/telemetry/collector.py index 1439c1a26..34682c38d 100644 --- a/headroom/telemetry/collector.py +++ b/headroom/telemetry/collector.py @@ -500,7 +500,7 @@ class TelemetryCollector: type_counts["string"] = type_counts.get("string", 0) + 1 elif isinstance(v, bool): type_counts["boolean"] = type_counts.get("boolean", 0) + 1 - elif isinstance(v, (int, float)): + elif isinstance(v, int | float): type_counts["numeric"] = type_counts.get("numeric", 0) + 1 elif isinstance(v, list): type_counts["array"] = type_counts.get("array", 0) + 1 @@ -532,7 +532,7 @@ class TelemetryCollector: dist.looks_like_id = dist.unique_ratio > 0.9 and dist.avg_length > 5 elif field_type == "numeric": - num_values = [v for v in values if isinstance(v, (int, float))] + num_values = [v for v in values if isinstance(v, int | float)] # Filter out infinity and NaN which can cause issues num_values = [ v @@ -744,8 +744,19 @@ def get_telemetry_collector( if _telemetry_collector is None: with _collector_lock: if _telemetry_collector is None: - # Check environment for opt-out - if os.environ.get("HEADROOM_TELEMETRY_DISABLED", "").lower() in ("1", "true"): + # Honour HEADROOM_TELEMETRY (the documented opt-out var, + # also used by the Supabase beacon at telemetry/beacon.py). + # Pre-#390 this only checked HEADROOM_TELEMETRY_DISABLED, + # so users who set HEADROOM_TELEMETRY=off (the value in + # the docs) still saw /v1/telemetry report enabled=true. + # HEADROOM_TELEMETRY_DISABLED stays accepted for back-compat. + from headroom.telemetry.beacon import is_telemetry_enabled + + disabled_legacy = os.environ.get("HEADROOM_TELEMETRY_DISABLED", "").lower() in ( + "1", + "true", + ) + if disabled_legacy or not is_telemetry_enabled(): config = config or TelemetryConfig() config.enabled = False diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 4ad51cf3c..b00c52d3f 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -576,6 +576,40 @@ class TestGlobalTelemetryCollector: assert collector._config.enabled is False + @pytest.mark.parametrize("off_value", ["off", "false", "0", "no", "disable", "disabled"]) + def test_headroom_telemetry_off_disables_collector(self, monkeypatch, off_value): + """HEADROOM_TELEMETRY=off (and other documented opt-out values) disables + the collector — closes #390. + + Pre-#390 the collector only honoured HEADROOM_TELEMETRY_DISABLED, which + is undocumented. Users following the docs set HEADROOM_TELEMETRY=off and + watched /v1/telemetry continue to report enabled=true. The collector now + consults `is_telemetry_enabled()` (the same predicate the Supabase beacon + uses), so both env vars take effect. + """ + reset_telemetry_collector() + monkeypatch.delenv("HEADROOM_TELEMETRY_DISABLED", raising=False) + monkeypatch.setenv("HEADROOM_TELEMETRY", off_value) + + collector = get_telemetry_collector() + + assert collector._config.enabled is False, ( + f"HEADROOM_TELEMETRY={off_value!r} must disable the collector — " + "this is the documented opt-out path. If this assertion fails the " + "collector is silently ignoring the user's opt-out and /v1/telemetry " + "will report enabled=true even when telemetry is supposed to be off." + ) + + def test_headroom_telemetry_on_keeps_collector_enabled(self, monkeypatch): + """Sanity check: the explicit on/unset path leaves the collector enabled.""" + reset_telemetry_collector() + monkeypatch.delenv("HEADROOM_TELEMETRY_DISABLED", raising=False) + monkeypatch.setenv("HEADROOM_TELEMETRY", "on") + + collector = get_telemetry_collector() + + assert collector._config.enabled is True + class TestRetrievalStatsModel: """Test RetrievalStats data model."""