mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(telemetry): honour HEADROOM_TELEMETRY=off in /v1/telemetry collector (#390)
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).
This commit is contained in:
parent
4745219901
commit
9ddbdf2313
2 changed files with 49 additions and 4 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue