Repository navigation
fix(telemetry): bucket gateway task sources to a known surface, not "unknown" - #2727
Conversation
…unknown" #2432 added task_processed{source} to the gateway (_write_task), tagging the task's free-form `source: or PROVIDER`. Any value outside _KNOWN_SOURCES collapses to "unknown" (_coarse_source, by design), so real gateway activity — custom REMOTE_TASK_PROVIDER labels and any non-allowlisted per-task source — was recorded as "unknown" instead of its surface. It deployed Aug 6 and by Aug 7 "unknown" was the single largest task_processed bucket fleet-wide (5040 events / 156 installs), burying the activation signal #2432 set out to add. Fix: add telemetry.bucket_source(source, default) — resolve to a known bucket, falling back to a caller-supplied *validated* default instead of the generic "unknown". The gateway is the "remote" surface, so _write_task now emits bucket_source(..., "remote"): allowlisted sources (ag2space, events-promotion, remote) are unchanged; everything else counts as "remote" rather than vanishing into "unknown". A bad default can't smuggle cardinality (it's coarsened too), so the leak/cardinality posture the allowlist exists for is preserved. Evidence (real telemetry module, sink stubbed): gateway-stamped source BEFORE AFTER acme-cloud-deployment unknown remote matrix unknown remote ag2space ag2space ag2space (allowlisted, unchanged) events-promotion events-promotion events-promotion Tests: tests/gateway-task-telemetry.test.py extended — non-allowlisted source buckets to remote (7e), secret-ish source still never leaks the raw value (7c), bucket_source unit (known kept / unknown→default / bad-default→unknown) (7f). Full suite green; src/telemetry.py suite unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 77%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Cloned fresh, checked out branch. The actual fix is correct, necessary, and well-evidenced — verified independently, not just read:
- Root cause confirmed real: PR #2432 (merged 2026-08-06, "emit task_processed for gateway... — the #2274 gap") added the un-bucketed
task_processed(_one_line(task.get("source") or PROVIDER))call — confirmed on main's currentremote_gateway_bridge.py. This PR's fix wraps it in the newbucket_source(). - Every row of the before/after table verified by direct execution, not read-and-trust:
bucket_source('acme-cloud-deployment', 'remote') -> 'remote' (OK) bucket_source('matrix', 'remote') -> 'remote' (OK) bucket_source('gateway', 'remote') -> 'remote' (OK) bucket_source('ag2space', 'remote') -> 'ag2space' (OK, allowlisted unchanged) bucket_source('events-promotion', 'remote') -> 'events-promotion' (OK, unchanged) bucket_source('remote', 'remote') -> 'remote' (OK) bucket_source('slack', 'remote') -> 'slack' (OK) - Cardinality-leak-guard claim verified:
bucket_source('some-secret-value', 'attacker-controlled-default-xyz')returns'unknown'— a bad/arbitrary default gets coarsened too, exactly as claimed ("a bad default is itself coarsened, so it can't smuggle cardinality"). - CI: 18/18 green.
One real but non-blocking finding, worth a fast follow-up — not a comment I'd trust without tracing it end to end:
tests/gateway-task-telemetry.test.py's new case 7f does import telemetry as _tel # the REAL module (src/ on sys.path from the header) — but the file's header only ever adds packages/ag2-sparrow to sys.path (line 45), never src/. Running the file standalone (python3 tests/gateway-task-telemetry.test.py) from a fresh clone on my host: ModuleNotFoundError: No module named 'telemetry' at that exact line. I traced why it nonetheless passes in your CI run (confirmed via the run's own log — genuinely green, not a stale/cached result): my host has a real credential file at ~/Library/.../channels/ag2space/.env that lets remote_gateway_bridge.py's token resolution (line ~424) succeed early. On a clean CI runner (or any host without that file), token resolution falls through to a different, unrelated helper (lines 476-499, walking up the directory tree for src/channel_token.py for a vault-token fallback) whose side effect — not its purpose — is sys.path.insert(0, src). Once any earlier _write_task() call in the same process triggers that fallback, src/ stays on sys.path for the rest of the run, which is the only reason the later bare import telemetry succeeds. The comment's claimed mechanism ("from the header") doesn't exist; the real mechanism is an accidental, host/environment-dependent side effect of unrelated code.
This isn't a regression from this PR's actual fix, and CI genuinely passes today — so not blocking a live fleet-wide mislabeling fix over it. But it's fragile: it would silently break the moment a real token becomes resolvable earlier in that same test run (e.g., CI ever gets a real AG2_REMOTE_TOKEN/.env), and the comment actively misleads about why it currently works. Suggest a fast follow-up: add an explicit sys.path.insert(0, str(REPO / "src")) near the file's existing sys.path.insert(0, str(REPO / "packages" / "ag2-sparrow")) (the pattern nearly every other test file in this repo already uses) and correct the comment.
Approving — the fix itself is solid and this is an active production issue.
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head fdc97c1beebd68e4dbaf79ecc6b6a48d9a782f1c.
The telemetry behavior is correct: reviewer-run gateway telemetry checks and the 24-check core telemetry suite pass; hostile source values collapse to remote without leaking raw data; hosted CI/coverage/CLA and diff/path checks are green. I am not repeating the already-reported environment-dependent sys.path test fragility.
[P2 blocker] The new comments/docstrings violate the current two-line/no-history code-comment policy. src/telemetry.py:303-309 is a six-line explanatory docstring; packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1584-1586 is a three-line historical #2274 block; and tests/gateway-task-telemetry.test.py:310-313 plus :355-360 carry long incident/history narratives. Keep each block to at most two lines containing only the non-obvious constraint; the full rationale already belongs in the PR body.
Not merge-ready: functionality is green, but the repository’s comment-maintainability gate is not yet satisfied.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head fdc97c1beebd68e4dbaf79ecc6b6a48d9a782f1c.
No blocking findings. bucket_source() preserves the fixed telemetry allowlist, validates the fallback bucket, and the gateway now reports non-allowlisted gateway/provider labels as the remote surface without leaking the raw source string. Idempotent early returns still avoid double-counting.
Focused checks passed:
python3 tests/gateway-task-telemetry.test.pypython3 tests/telemetry.test.py(rerun outside the sandbox because the localhost HTTP server bind is blocked there)python3 -m py_compile src/telemetry.py packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py tests/gateway-task-telemetry.test.pygit diff --check origin/main...HEAD
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
|
@cla-assistant check |
Problem
task_processed{source="unknown"}spiked to the single largest task bucket fleet-wide starting Aug 7 — 5040 events across 156 installs (≈32/install), where it had been ~0 the prior days. Root cause is a regression from #2432 (merged Aug 6, deployed → fleet Aug 7), not a test leak (spread across many distinct cloud IPs) and not new volume (it's mislabeled existing gateway activity).#2432 added
task_processedto the gateway task-accept point (ag2_sparrow/remote_gateway_bridge.py:_write_task), tagging the task's free-form source:_coarse_sourcecollapses anything outside_KNOWN_SOURCESto"unknown"(by design — cardinality/leak guard). The gateway's allowlisted sources (ag2space,remote,events-promotion) bucket fine, but a customREMOTE_TASK_PROVIDERor any other per-task source label collapses to"unknown"— so real gateway/remote activity was recorded as "unknown" rather than under its surface, burying the exact activation signal #2432 set out to add.Fix
Add
telemetry.bucket_source(source, default)— resolve to a known bucket, falling back to a caller-supplied validated default instead of the generic"unknown". The gateway is the"remote"surface, so_write_tasknow emitsbucket_source(..., "remote"). A bad default is itself coarsened, so it can't smuggle cardinality — the leak posture the allowlist exists for is unchanged.Before / after (real telemetry module, network sink stubbed)
Tests
tests/gateway-task-telemetry.test.pyextended (all 27 checks pass):remote, notunknown(the regression pin)remote, raw string absent from the payload)bucket_sourceunit: known kept / unknown→default / bad-default→unknownsrc/telemetry.pysuite unaffected (24 checks pass). The_FakeTelemetryharness now delegates to the realbucket_sourceso fake cases exercise the same bucketing as production.Scope note
This attributes non-allowlisted gateway sources to the
remotesurface, which is correct and value-agnostic. If specific gateway sub-surfaces beyondag2space/events-promotiondeserve their own distinct bucket, that's an additive follow-up (allowlist the specific value) — orthogonal to stopping the "unknown" bleed.