From 580bcda7fb9305574bf6eef8c382f5437012a842 Mon Sep 17 00:00:00 2001 From: mmercuri Date: Thu, 23 Apr 2026 07:53:09 -0700 Subject: [PATCH] sdk-telemetry: tighten cross-repo counter contract + auto atexit + scheme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the cross-PR panel audit of the metrics stack: F24 — Counter label-set divergence from atlas-app Go declaration atlas-app/apps/shared/observability/metrics.go declares atlas_sdk_events_total with EXACTLY two labels (surface, event). The SDK previously carried an "allowlist" that forwarded command + resource + outcome + status_code to the OTel counter, producing a 3-or-4-label Prometheus series server-side. Dashboards, cardinality budgets, and TestEmitSiteArity are all built against the 2-label declaration — the divergence meant some series didn't match the intended query shape. Fix: strip ALL attributes from the counter emission. Keep the ``attributes=`` parameter on the event() signature for forward compatibility, but drop it before counter.add(). Extra dimensions belong on OTel SPAN attributes, not the counter — and atlas-app's Go declaration stays the single source of truth for the label schema. Test renamed from test_attributes_allowlist to test_counter_labels_match_atlas_app_declaration and asserts the exact 2-key contract. F26 — OTLP endpoint scheme vs LAYERLENS_OTLP_INSECURE ambiguity Previously: LAYERLENS_OTLP_ENDPOINT=https://otel.layerlens.ai:4317 LAYERLENS_OTLP_INSECURE=true would pass both to OTLPMetricExporter, and the gRPC exporter's resolution was version-dependent — sometimes honouring the scheme, sometimes the flag. Customers couldn't predict which would win. Fix: detect scheme from the endpoint. https:// → insecure=False, http:// → insecure=True. The LAYERLENS_OTLP_INSECURE env var is now only consulted for scheme-less endpoints (the pure "host:port" form). Strip the scheme before handing the endpoint to the exporter since the gRPC exporter expects schemeless host:port. F27 — Non-CLI SDK consumers lost up to 30s of telemetry on exit PeriodicExportingMetricReader flushes every 30s and on meter_provider.shutdown(). CLI callers registered atexit.register(_telemetry.shutdown) explicitly; library consumers that just imported Stratix and opted in via env had nothing hooking their process exit. Up to 30s of init/cmd_run events were silently dropped on a clean exit. Fix: auto-register atexit.register(shutdown) on first successful _try_init(). _atexit_registered guard keeps it idempotent, so the CLI's explicit atexit.register is still a no-op second registration. Library consumers now flush automatically. Verification: PYTHONPATH=src python -m pytest tests/test_telemetry.py -v => 16 passed in 0.52s --- src/layerlens/_telemetry.py | 54 ++++++++++++++++++++++++++++++------- tests/test_telemetry.py | 36 ++++++++++++++++--------- 2 files changed, 68 insertions(+), 22 deletions(-) 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):