Instrument skill handlers and dialog rendering - #509
goldyfruit wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds process-local latency histograms for skill-handler execution and dialog rendering. Instruments eligible handlers and rendering paths, registers the metrics provider, adds runtime tests, and documents histogram semantics and exposure. ChangesRuntime performance metrics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EventSystem
participant OVOSSkill
participant SKILL_HANDLER
participant DIALOG_RENDER
EventSystem->>OVOSSkill: dispatch handler with handler_info
OVOSSkill->>SKILL_HANDLER: measure handler execution
SKILL_HANDLER-->>OVOSSkill: record duration
OVOSSkill->>DIALOG_RENDER: measure dialog rendering
DIALOG_RENDER-->>OVOSSkill: record duration
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds fixed-cardinality timing metrics for skill handlers and dialog rendering without a remaining concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
| ) | ||
|
|
||
|
|
||
| class LatencyHistogram: |
There was a problem hiding this comment.
I have some doubts on this one, maybe we need metrics helpers in ovos-utils to use across all other packages? we have the StopWatch class in there already.
There was a problem hiding this comment.
Agreed—the exporter is already provider/entry-point driven, but the low-level monotonic histogram storage should be shared instead of copied. I extracted that dependency-free primitive to OpenVoiceOS/ovos-utils#416. Workshop cannot import Core without reversing the dependency direction, and I do not want to hide the coordination behind a fallback/getattr. The intended order is: review and release #416, then replace this PR’s local helper and bump the explicit ovos-utils minimum. Metric ownership stays in Workshop; scrape/export policy stays in the host runtime. I am leaving this thread unresolved until that prerequisite and consumer rebase are complete.
Adds two opt-in latency histograms to the skill runtime: SKILL_HANDLER around every registered event handler that carries handler_info, and DIALOG_RENDER around each dialog-renderer call (speak_dialog and both get_response fallbacks). The histograms are exported through the `ovos.performance.metrics` entry point so a collector can aggregate them alongside core's without importing workshop internals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9efd827 to
f10d342
Compare
|
@coderabbitai review |
|
Closing this: the instrumentation it adds turns out not to need a change in this repository at all. A stock That has been verified rather than assumed: a real unmodified skill drives the events, a separate bus client correlates them, and an HTTP scrape of the real exporter returns the metric family with a non-zero count. The error path emits The one metric that genuinely cannot be observed from outside is the dialog render timer, which measures a template expansion nested inside a handler that is already timed. That is not worth carrying a fork of this package for, so it is dropped rather than replaced. The |
2 participants
What changed
speak_dialogandget_responseretry pathsovos.performance.metricsArchitecture and scope
Workshop owns these skill-level observations;
ovos-coreremains the sole HTTP exporter owner. No session, skill, utterance, request, or other unbounded value is exported as a label.The earlier reply-tracing experiment was removed from the final diff. Direct-emitting skills can bypass a skill-mixin hook, and generic request IDs are not an OVOS message contract. This PR is metrics-only.
Jarbas's shared-helper review is tracked explicitly: OpenVoiceOS/ovos-utils#416 adds the dependency-free monotonic histogram primitive. The intended release order is to review and release #416, then replace this PR's local helper and bump the minimum
ovos-utilsversion. Workshop will not import Core and create a dependency cycle, and no fallback orgetattrwill hide package skew.Why
The HiveMind bridge metrics stop before OVOS skill execution. Handler and dialog boundaries expose the remaining user-visible work while preserving message, handler, and rendering semantics.
Validation
Current head
9efd827ee6b3f29d31407f950f74019a46ca66a8:devin the same reused environmentgit diff --check: passedSummary by CodeRabbit
New Features
Documentation