feat: log compression attempt telemetry - #60444
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused, content-free telemetry work. One coverage gap needs resolving before this can substantiate its “each Hermes compression attempt” claim.
Problems
agent/conversation_compression.py:504initializes telemetry, but the Codex app-server branch atagent/conversation_compression.py:525immediately returns through_compress_context_via_codex_app_server. Current main performs native compaction atagent/conversation_compression.py:1173and records a successful boundary atagent/conversation_compression.py:1199; neither the success nor failure path reaches the proposed emitter.tests/agent/test_compression_attempt_telemetry.pyexercises only the ordinary compressor path, so this omission is untested.
Suggested changes
- Cover Codex-native success, interruption/error, and skipped outcomes with one metadata-only record per attempt, and add focused regression tests.
Automated hermes-sweeper review.
| @@ -465,6 +504,19 @@ def compress_context( | |||
| prompt — the session is NOT rotated. Callers should detect the | |||
There was a problem hiding this comment.
This telemetry setup is bypassed by the existing Codex app-server return at line 525. Native compaction therefore produces no record even though it is a real compression path; emit telemetry from _compress_context_via_codex_app_server for its success and failure outcomes too.
- aux summary call on main intentionally omits max_tokens; use .get() in the telemetry hook (and widen the param type) so the hook never breaks the call - update test expectation: aux_output_reservation is None on main - record no_progress failure_class in the no-progress boundary branch Follow-up for salvaged PR #60444.
- aux summary call on main intentionally omits max_tokens; use .get() in the telemetry hook (and widen the param type) so the hook never breaks the call - update test expectation: aux_output_reservation is None on main - record no_progress failure_class in the no-progress boundary branch Follow-up for salvaged PR #60444.
|
Merged via #69338 (commit cbc1054). Your commit was cherry-picked with authorship preserved (verified egress-free and content-free — the parking policy for outbound telemetry didn't apply to local logging). One adaptation: the aux call kwargs access was hardened against main's no-max-tokens summary contract. Thanks! |
- aux summary call on main intentionally omits max_tokens; use .get() in the telemetry hook (and widen the param type) so the hook never breaks the call - update test expectation: aux_output_reservation is None on main - record no_progress failure_class in the no-progress boundary branch Follow-up for salvaged PR NousResearch#60444.
- aux summary call on main intentionally omits max_tokens; use .get() in the telemetry hook (and widen the param type) so the hook never breaks the call - update test expectation: aux_output_reservation is None on main - record no_progress failure_class in the no-progress boundary branch Follow-up for salvaged PR NousResearch#60444.
Summary
Scope
Test Plan
........................................................................ [ 89%]
................. [100%]
161 passed in 9.66s