Expose scrapeable OVOS runtime stage histograms - #844
Open
goldyfruit wants to merge 14 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds process-local latency histograms, opt-in request tracing, Prometheus collection and rendering, an opt-in ChangesRuntime performance metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Service
participant IntentService
participant LatencyHistogram
participant start_metrics_server
participant MetricsRequestHandler
participant collect_histograms
Service->>start_metrics_server: start when metrics are enabled
Service->>IntentService: process an utterance
IntentService->>LatencyHistogram: record stage timings
MetricsRequestHandler->>collect_histograms: collect snapshots for /metrics
collect_histograms-->>MetricsRequestHandler: return validated metrics
MetricsRequestHandler-->>Service: return the Prometheus response
Service->>start_metrics_server: stop during cleanup
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
ovos_core/_metrics.py (1)
80-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
strict=Trueto thezip()call.Ruff reports B905 on this line.
_boundsand_bucketsalways have the same length, sostrict=Truepreserves behavior and clears the lint finding.♻️ Proposed change
buckets = { f"le_{bound:g}": count - for bound, count in zip(self._bounds, self._buckets) + for bound, count in zip(self._bounds, self._buckets, strict=True) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ovos_core/_metrics.py` around lines 80 - 83, Add strict=True to the zip call within the bucket construction in the relevant metrics method, preserving the existing pairing of _bounds and _buckets while satisfying Ruff B905.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/performance-metrics.md`:
- Around line 39-40: Update the Prometheus guidance near the “32 partitions”
text to remove the hardcoded partition count and describe aggregating buckets
across all deployed runtime processes before calculating a percentile. Keep the
requirement that Prometheus scrape every runtime process.
- Around line 29-35: Update the entry-point documentation to remove the specific
ovos-workshop and skill-handler/dialog-rendering collector claims. Keep the
general statement that installed packages can contribute metrics through
ovos.performance.metrics, and retain the collector contract and loading behavior
without asserting metrics this repository does not publish.
- Around line 7-14: Update the metrics configuration example to use 127.0.0.1
for OVOS_METRICS_HOST instead of 0.0.0.0. Expand the guidance around the GET
/metrics endpoint to state that it has no authentication and require an
authenticating reverse proxy or network policy for remote scraping; explicitly
discourage binding 0.0.0.0 on untrusted networks.
In `@ovos_core/_prometheus.py`:
- Around line 86-94: Update render_prometheus to validate metric-name uniqueness
after applying _metric_name, so names such as stage_ms and stage_seconds cannot
emit duplicate exported metrics. Keep collect_histograms’ raw-name validation,
but add exported-name collision detection before rendering and reject duplicates
consistently.
- Line 148: Update the `_sum` counter formatting in the Prometheus exposition
code around the `exported` and `sum_seconds` symbols to use a full-precision
numeric format instead of `:g`. Leave the fixed-bound `le` label formatting
unchanged.
---
Nitpick comments:
In `@ovos_core/_metrics.py`:
- Around line 80-83: Add strict=True to the zip call within the bucket
construction in the relevant metrics method, preserving the existing pairing of
_bounds and _buckets while satisfying Ruff B905.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 436a189e-3e9b-4794-8137-db9d6e0d1c5c
📒 Files selected for processing (8)
docs/index.mddocs/performance-metrics.mdovos_core/__main__.pyovos_core/_metrics.pyovos_core/_prometheus.pyovos_core/intent_services/service.pytest/unittests/test_intent_service_extended.pytest/unittests/test_runtime_metrics.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ovos_core/_metrics.py`:
- Around line 150-160: Update pipeline_matching_histogram to avoid
substring-matching built-in families within third-party identifiers such as
third-party-adapt-wrapper; classify only canonical built-in identifiers or valid
token-boundary matches, otherwise returning "other". Add a regression case
covering a third-party identifier containing a built-in family name.
In `@ovos_core/_prometheus.py`:
- Around line 112-123: In the counter branch of the Prometheus metric rendering
logic, replace the `_count` conversion for snapshot values with `_number` so
finite fractional counters such as 1.6 are accepted while existing validation
remains intact. Add coverage for a fractional counter through the `/metrics`
endpoint and verify it returns successfully with the rendered value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dd503da-ae41-4a2e-b871-1f384fec7f63
📒 Files selected for processing (6)
docs/performance-metrics.mdovos_core/_metrics.pyovos_core/_prometheus.pyovos_core/intent_services/service.pytest/unittests/test_intent_service_extended.pytest/unittests/test_runtime_metrics.py
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unittests/test_intent_service_extended.py
- docs/performance-metrics.md
- ovos_core/intent_services/service.py
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/performance-metrics.md (1)
53-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAllow numeric counter values in the collector contract.
Line 57 says that counters require an integer
value. The renderer accepts numeric values, andtest/unittests/test_runtime_metrics.pyregisters1.6. Documentvalueas numeric so valid plugin collectors are not rejected by the documented contract.Proposed fix
-`sum_ms`, and cumulative `buckets`; counters provide `type: counter` and an -integer `value`, and their names end in `_total`. +`sum_ms`, and cumulative `buckets`; counters provide `type: counter` and a +numeric `value`, and their names end in `_total`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/performance-metrics.md` around lines 53 - 59, Update the counter contract in the documented collector description to state that value accepts numeric values rather than only integers. Keep the existing counter type and _total naming requirements unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/performance-metrics.md`:
- Around line 53-59: Update the counter contract in the documented collector
description to state that value accepts numeric values rather than only
integers. Keep the existing counter type and _total naming requirements
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ff154fa-4576-4ce2-9701-60cad61a4816
📒 Files selected for processing (10)
docs/performance-metrics.mdovos_core/_metrics.pyovos_core/_performance_trace.pyovos_core/_prometheus.pyovos_core/intent_services/converse_service.pyovos_core/intent_services/dispatcher.pyovos_core/intent_services/service.pytest/unittests/test_converse_service.pytest/unittests/test_intent_service_extended.pytest/unittests/test_runtime_metrics.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unittests/test_intent_service_extended.py
- ovos_core/_prometheus.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
ovos-coreovos.performance.metricsentry-point contractArchitecture and scope
Core owns the process-local exporter and its own runtime stages. Workshop and pipeline packages own their instrumentation and contribute snapshots through entry points; they do not start more HTTP servers.
Pipeline identifiers are session-selectable, so raw pipeline IDs, utterances, intents, skills, languages, sessions, and request identifiers never become metric names or labels. Unknown matchers are aggregated into the fixed
otherfamily. The endpoint is disabled by default and binds to loopback by default.This PR is metrics-only. The earlier request-correlated tracing experiment was removed at
b13ed4dafter architecture review; it is not part of the proposed OVOS contract. This PR changes no message-bus, SDK, MCP, deployment, or CI contract.The low-level histogram storage can move to the dependency-free helper proposed in OpenVoiceOS/ovos-utils#416 after that helper is reviewed and released. Consumers will use an explicit minimum-version bump; there will be no
getattror compatibility fallback.Why
Listener stages explained only a small fraction of the 400-client canary tail. These fixed stages make 32 runtime processes aggregatable without parsing periodic logs or creating unbounded cardinality, so later performance changes can target measured matcher or dispatch work without reordering interaction semantics speculatively.
Validation
Current head
b13ed4de230a5cc0db8ff78f821de3dad5ae6114:git diff --check: passed