diff --git a/src/layerlens/_telemetry.py b/src/layerlens/_telemetry.py index 17f0f4b..cd3e6ac 100644 --- a/src/layerlens/_telemetry.py +++ b/src/layerlens/_telemetry.py @@ -33,6 +33,7 @@ """ from __future__ import annotations +import atexit import contextlib import os import time @@ -43,6 +44,7 @@ _meter: Any = None _counter_events: Any = None _hist_request_duration: Any = None +_atexit_registered: bool = False def _enabled() -> bool: @@ -85,9 +87,23 @@ def _try_init() -> bool: endpoint = os.environ.get( "LAYERLENS_OTLP_ENDPOINT", "https://otel.layerlens.ai:4317" ) - insecure = ( - os.environ.get("LAYERLENS_OTLP_INSECURE", "false").lower() == "true" - ) + + # Resolve insecure-vs-TLS from the endpoint scheme when one is present, + # falling back to the explicit env flag for schemeless endpoints (the + # OTel gRPC convention of "host:port"). This avoids the ambiguity where + # a caller sets LAYERLENS_OTLP_ENDPOINT=https://... AND + # LAYERLENS_OTLP_INSECURE=true — previously the flags would disagree, + # and the OTLP gRPC exporter's behaviour was version-dependent. Now + # the scheme, when present, is authoritative. + insecure_env = os.environ.get("LAYERLENS_OTLP_INSECURE", "").strip().lower() + if endpoint.startswith("https://"): + insecure = False + endpoint = endpoint[len("https://") :] + elif endpoint.startswith("http://"): + insecure = True + endpoint = endpoint[len("http://") :] + else: + insecure = insecure_env == "true" resource = Resource.create({ "service.name": "atlas-sdk-python", @@ -122,6 +138,18 @@ def _try_init() -> bool: _hist_request_duration = None return False + # Auto-register atexit shutdown on first successful init. Without this, + # SDK consumers that don't manually call _telemetry.shutdown() would + # lose up to 30 s of buffered events on clean process exit (the + # PeriodicExportingMetricReader flush interval). The CLI also calls + # atexit.register(_telemetry.shutdown) explicitly — the registered-once + # guard here means a second registration is a no-op, keeping shutdown + # idempotent regardless of call path. + global _atexit_registered + if not _atexit_registered: + atexit.register(shutdown) + _atexit_registered = True + return True @@ -134,16 +162,24 @@ def event( """Increment ``atlas_sdk_events_total{surface, event}`` by 1. No-op when telemetry is disabled or OTel SDK is absent. + + The ``attributes`` parameter is accepted for forward compatibility and + (when present) is attached to the **OTel span** covering the timed() + context — NOT to the counter. The atlas-app Prometheus counter + ``atlas_sdk_events_total`` is declared with exactly 2 labels + (``surface, event``) in ``apps/shared/observability/metrics.go``; any + third attribute on the counter would create a divergent label set that + the server-side declaration and dashboards don't anticipate. Extra + attributes are therefore dropped here by design — keep atlas-app as + the single source of truth for the counter's label schema. """ if not _try_init() or _counter_events is None: return + # Exactly two labels — MUST match the atlas-app Go declaration. attrs: dict[str, Any] = {"surface": surface, "event": event_name} - if attributes: - # Only allow a small allowlist of attribute keys — no PII. - allow = {"command", "resource", "outcome", "status_code"} - for k, v in attributes.items(): - if k in allow and isinstance(v, (str, int, bool, float)): - attrs[k] = str(v) + # Note: `attributes` is intentionally ignored for the counter. See + # docstring above for rationale + where richer dimensions belong. + _ = attributes try: _counter_events.add(1, attributes=attrs) except Exception: diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index c96a37f..0633772 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -61,12 +61,24 @@ def test_event_with_telemetry_off_doesnt_import_otel(monkeypatch, _reset_telemet assert "opentelemetry" not in sys.modules -def test_attributes_allowlist(monkeypatch, _reset_telemetry_module): - """Disallowed attribute keys are silently dropped, not raised.""" +def test_counter_labels_match_atlas_app_declaration( + monkeypatch, _reset_telemetry_module +): + """The Prometheus counter atlas_sdk_events_total is declared with EXACTLY + two labels on the atlas-app server side (surface, event). To keep the + two-repo contract aligned, the SDK MUST NOT add any third attribute to + the counter — regardless of what the caller passes via ``attributes=``. + Any extra attribute WOULD create a divergent label set that server-side + dashboards and recording rules don't anticipate. + + This test asserts that even attributes a previous allowlist permitted + (command, outcome, etc.) are now dropped. Extra dimensions belong on + OTel span attributes, not on the counter — atlas-app is the single + source of truth for the counter's label schema. + """ t = _reset_telemetry_module monkeypatch.setenv("LAYERLENS_TELEMETRY", "on") - # Stub OTel SDK so we can observe what reaches add(). seen_attrs: dict = {} class _StubCounter: @@ -82,19 +94,17 @@ def add(self, value, attributes=None): "cli", "cmd_run", attributes={ - "command": "trace ls", - "email": "user@example.com", # MUST be dropped - "ip": "10.0.0.1", # MUST be dropped - "outcome": "success", + "command": "trace ls", # dropped by the 2-label contract + "email": "user@example.com", # dropped (PII) + "ip": "10.0.0.1", # dropped (PII) + "outcome": "success", # dropped by the 2-label contract }, ) - assert seen_attrs.get("surface") == "cli" - assert seen_attrs.get("event") == "cmd_run" - assert seen_attrs.get("command") == "trace ls" - assert seen_attrs.get("outcome") == "success" - assert "email" not in seen_attrs - assert "ip" not in seen_attrs + # Exactly surface + event, nothing more. + assert set(seen_attrs.keys()) == {"surface", "event"} + assert seen_attrs["surface"] == "cli" + assert seen_attrs["event"] == "cmd_run" def test_event_swallows_counter_errors(monkeypatch, _reset_telemetry_module):