feat: add privacy-preserving runtime telemetry - #64472
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Privacy-preserving runtime telemetry adding model call and cron completion events. Uses stable_hash for provider/model identifiers rather than raw values. Well-structured with best-effort error handling (telemetry failures do not break model calls). 1116 additions, 3 deletions. The hermes_telemetry module provides the actual privacy guarantees.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the privacy-focused telemetry work. The feature is not already present on current main, but this version needs changes before it is safe to salvage.
Problems
cron/scheduler.py:620-682reintroduces per-job profile routing. Maintainer commit7d8d000b1explicitly removed this exact behavior, and currentcron/jobs.py:54-64treats profile isolation as a security boundary. Please remove this non-telemetry behavior from the PR.hermes_telemetry.py:79-80redacts every key containingtoken; this convertsgateway/run.py:216-217'sinput_tokens/output_tokensandagent/chat_completion_helpers.py:169'sestimated_context_tokensinto[REDACTED]in persisted JSONL. The gateway test only checks the pre-serialization payload (tests/test_gateway_telemetry.py:42-59).hermes_telemetry.py:126-128writes local JSONL unconditionally, but the PR contains no config or docs changes. This conflicts with the stated optional-storage behavior and leaveswebsite/docs/reference/faq.md:54inaccurate.
Suggested changes
- Keep cron instrumentation limited to the existing execution path; do not restore profile routing.
- Preserve safe numeric usage metrics and add serialized-event regression tests.
- Add a profile-safe user configuration gate and update the privacy documentation.
Automated hermes-sweeper review.
|
|
||
|
|
||
| @contextmanager | ||
| def _job_profile_context(job_id: str, profile: Optional[str]): |
There was a problem hiding this comment.
Please remove the per-job profile context from this telemetry PR. Commit 7d8d000b1 explicitly reverted this wrapper, and current cron/jobs.py:54-64 documents per-profile cron isolation as a security boundary; this reintroduces behavior unrelated to telemetry without restoring its public schema or CLI surface.
| for key, item in value.items(): | ||
| key_text = str(key) | ||
| key_lc = key_text.lower() | ||
| if any(fragment in key_lc for fragment in _SENSITIVE_KEY_FRAGMENTS): |
There was a problem hiding this comment.
This catches numeric metric names such as input_tokens, output_tokens, and estimated_context_tokens, so their persisted values become [REDACTED]. Keep secret-bearing values redacted, but allow these numeric usage counters and add a serialized JSONL assertion.
| """ | ||
| event = build_event(event_type, payload, status=status, source=source, ts=ts) | ||
| path = event_path(ts, hermes_home) | ||
| path.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
This creates the telemetry directory on every emission with no configuration gate. The PR describes local storage as optional, but it changes no config or docs files; please add a profile-safe opt-in/disable mechanism before creating persistent event files.
Summary
Privacy properties
Test plan
python -m pytest tests/test_hermes_telemetry.py tests/cron/test_cron_telemetry.py tests/test_gateway_telemetry.py tests/test_tool_call_telemetry.py tests/test_model_call_telemetry.py tests/hermes_cli/test_status.py -q -o 'addopts='ruff check hermes_telemetry.py cron/scheduler.py gateway/run.py model_tools.py agent/chat_completion_helpers.py hermes_cli/status.pyThis is the telemetry-only replacement for #50894; delegation and Telegram changes are submitted separately.