feat(observability): add bounded residual runtime diagnostics - #122
feat(observability): add bounded residual runtime diagnostics#122Skywind5487 wants to merge 2 commits into
Conversation
૮ >ﻌ< ა ci reviewran on 5dbee48 — feat(observability): residual runtime fact vocabulary
|
e5b9f42 to
04eccfc
Compare
|
Review follow-up for the #114 lifetime finding:
Final branch shape after refresh: current fork |
f6e8476 to
cfb8045
Compare
Skywind5487
left a comment
There was a problem hiding this comment.
🔍 Summary
HOLD — one blocking startup-order race breaks the PR's core process-lifetime guarantee. The prior-art split, privacy boundary, and CI are otherwise in good shape.
Ponytail / over-engineering pass
hermes_cli/observability/runtime_observability.py—DOMAIN_OWNERSHIPis runtime-unused ownership metadata duplicated by the docs and an exact snapshot test. The behavioral Relay-ownership test already proves the important non-duplication invariant. yagni: delete the constant + exact dict assertion._RuntimeDiagnosticLogHandler.emit()/_emit_fact()— enabled state is checked twice for every mapped record, so the same config gate is read twice. shrink: keep one gate.
net: ~-18 lines possible (over-engineering pass only).
🔴 Critical
-
agent/monitoring/emitter.py: MonitoringEmitter.subscribe()— when a residual event is pre-buffered, the firstsubscribe()sees a non-empty queue and immediately starts_run(). But the real gateway startup registers the span streamer first (otlp_exporter.start_streaming(..., event_filter=_gateway_health_event)) and the diagnostic-log streamer second._gateway_health_event()accepts onlygateway_health/cron_execution, so the dispatcher can dequeue the earlygateway_diagnosticduring that gap and the first subscriber filters it out before the diagnostic subscriber exists. This directly contradicts the PR's stated multi-sink contract ("does not start dispatch from subscribe()") and can lose the exact startup diagnostic this revision is meant to preserve.Suggestion: do not start dispatch from
subscribe()just because the queue is non-empty. In the current gateway path, the ordinary initial snapshot emitted after both subscribers are attached already starts the dispatcher and drains the buffered residual through the complete fan-out. Add a deterministic regression for the actual order: buffered residual → span subscriber → diagnostic subscriber → initial snapshot → verify residual reaches diagnostic sink and snapshot reaches span sink.
⚠️ Warnings
tests/hermes_cli/test_runtime_observability.py:test_startup_fact_survives_until_first_monitoring_subscriber— this test uses only onedelivered.extendsubscriber, so it does not reproduce the multi-sink startup order claimed in the PR body and cannot catch the race above.AGENTS.mdexplicitly asks for real-path/E2E validation where mocks or partial wiring can hide integration bugs. Exercise the two real subscriber semantics (orstart_gateway_health_export()with a deterministic fake SDK/sink) instead of stopping at the emitter-only case.
💡 Suggestions
- Keep the exact-template source-anchor invariant; unlike a catalog snapshot, it checks a real relationship the adapter depends on: every configured exact template must still exist at its owned emit site.
✅ Looks Good
- Prior-art disposition checks out: upstream NousResearch#64536, NousResearch#73120, NousResearch#68882, NousResearch#73544 are merged; NousResearch#48871, NousResearch#51714, NousResearch#80359 remain open/unmerged; NousResearch#9596, NousResearch#22095, NousResearch#65077 are closed/unmerged. NousResearch#73120 is specifically the restored Relay landing after the earlier revert cycle.
- Upstream
mainhas advanced from the PR's checked5dd15872…tocb0fd836…, but the four intervening commits touch Desktop/package files only; none touch the relevant monitoring/stream/SQLite/delivery seams, so the preflight conclusion remains current. - Closed residual vocabulary, no raw rendered log export, frozen SQLite buckets, fail-open boundaries, and no duplicate lifecycle/tool plane all match #114's intended scope.
- CI and Docker workflow runs are green on
cfb8045; the blocker is an untested interleaving, not a currently failing check.
Standards
1 finding. The integration-sensitive startup lifetime contract is not validated through the actual two-subscriber assembly path; the focused test proves only single-subscriber buffering. Worst issue: real gateway fan-out ordering is untested and currently races.
Spec
1 blocking finding. #114 requires a tested, complete optional observability subsystem. The current subscribe() behavior can silently discard a pre-exporter residual diagnostic before its owning diagnostic sink attaches, so the startup-lifetime completion claim is not yet satisfied.
Verdict: HOLD / fix then re-review.
|
Review fix applied — addressing the HOLD on the startup-order race. New head:
Validation: hermetic runner 20/20 focused tests; Ruff + compileall clean. Requesting re-review. |
e7bdce0 to
8a69beb
Compare
Introduce the bounded-delivery seam between residual runtime producers and the OTLP sink set: - MonitoringEmitter gains emit_buffered(): an already-opted-in producer whose observation lifetime begins before its exporter/subscriber is constructed can enqueue one event into the existing bounded queue without turning on unrelated monitoring producers or creating a second queue/store. - subscribe() deliberately does NOT start dispatch on a non-empty queue. Gateway startup attaches a filtered span streamer before the diagnostic-log streamer; an early dispatcher would dequeue a buffered gateway_diagnostic into a partial fan-out and lose it before its owning sink exists. Dispatch starts on the first ordinary emit() after the full subscriber set attaches. - The dispatcher leaves buffered events queued when the subscriber set is empty rather than consuming them into an empty fan-out. - hermes_cli installs the dormant observation boundary for the process lifetime; installation itself performs no config/network/disk I/O and is fail-open so it can never decide whether Hermes starts. Regression coverage pins subscribe-never-starts-dispatch on buffered events and the ordinary post-subscribe emit that drains them.
Project the residual operator-diagnostic facts (stream, SQLite/session persistence, delivery) onto the existing monitoring diagnostic sink without duplicating upstream lifecycle/tool ownership (Relay shared metrics owns those). - STABLE_FACTS defines the deliberate closed fact subset; success/noise events are intentionally absent. Lifecycle and tool remain upstream-owned. - A dormant logging handler normalizes only exact, code-owned static log templates at the residual failure/terminal boundaries. It never fuzzy matches rendered text or stringifies interpolation arguments. - SQLite persistence maps only the exception through the existing bounded classifier and exports a frozen coarse bucket; everything else is content-free by contract. - Redaction is producer-side: no prompts, tool args/results, ids, paths, provider responses, or exception text can cross the event boundary. - _emit_fact keeps a single enablement gate and stays fail-open so telemetry can never change the observed runtime result. Docs record the ownership resolution, stable subset, static-log rationale, and redaction contract.
8a69beb to
5dbee48
Compare
|
Second review round — flush() residual + history cleanup applied. New head: flush() residual fixed
if timeout <= 0 or not self._subscribers or not self._started:
returnWith a subscriber attached but Regression added: Docs de-staledRemoved the sentence "the first subscriber starts normal dispatch and drains it" (now opposite to the code) and rewrote the lifetime paragraph to state: HistoryFolded the fix into the two behavior-intent commits via autosquash:
Cumulative diff vs fork |
Closes #114.
Completion claim
Reconstructs the Phase-1 runtime-observability capability as a bounded residual diagnostic extension on current fork
main, without replaying the historical telemetry stack.Intent-readable history
The branch no longer carries a single mega-squash. The final tree is split into two intent commits (code + tests + docs travel together per intent):
feat(observability): process-lifetime bounded delivery boundary(07581832b)MonitoringEmitter.emit_buffered()— an already-opted-in producer whose observation lifetime predates its exporter enqueues into the existing bounded queue without enabling unrelated producers or creating a second store.subscribe()never starts dispatch on a non-empty queue — the buffered residual survives the gateway's two-sink assembly (span streamer first, diagnostic-log streamer second) and is drained by the ordinary post-assembly snapshot emit.hermes_cliinstalls the dormant observation boundary for the process lifetime (fail-open, no config/network/disk I/O).feat(observability): residual runtime fact vocabulary(8a69bebad)STABLE_FACTSclosed residual subset (stream / SQLite-persistence / delivery); lifecycle and tool remain upstream-owned (Relay).Review fix (blocking, folded in)
PR #122 review found
subscribe()could start dispatch on the first (span-filtered) subscriber and lose a pre-bufferedgateway_diagnosticbefore the diagnostic-log streamer attached. Fixed: subscribe only enables the plane; the initial gateway-health snapshot starts dispatch after the full fan-out is assembled. Regressiontest_startup_fact_survives_multi_sink_subscribe_orderexercises the real order: buffered residual → span subscriber → diagnostic subscriber → initial snapshot, asserting the residual reaches only its owning sink. Over-engineering cleanup folded in: dropped runtime-unusedDOMAIN_OWNERSHIP+ its exact-snapshot test (YAGNI; Relay behavioral test already proves non-duplication) and the duplicateenabled()gate.Verification
tests/hermes_cli/test_runtime_observability.py— fact vocabulary, redaction, fail-open, template anchoring, multi-sink ordering.tests/monitoring/test_emitter.py— subscribe-never-starts-dispatch, hot path, unsubscribe.tests/monitoring/test_otlp_exporter.py— sink/filter compatibility.