feat: exporter prometheus_client /metrics (JEP-0013 Phase 2) - #934
Open
RoddieKieley wants to merge 11 commits into
Open
feat: exporter prometheus_client /metrics (JEP-0013 Phase 2)#934RoddieKieley wants to merge 11 commits into
RoddieKieley wants to merge 11 commits into
Conversation
- Add exporter-local `prometheus_client` registry with JEP-named series: `jumpstarter_operations_total`, `jumpstarter_operation_duration_seconds`, `jumpstarter_operation_errors_total`, `jumpstarter_stream_bytes_total`, `jumpstarter_active_sessions`, plus exemplars (`client`, `lease_id`). - Expose HTTP `GET /metrics` on the exporter process for lab/dev scrape (same registry Phase 3 will later reverse-scrape via MetricsStream). - Minimal core-path wiring so series increment under test; full per-driver telemetry architecture remains Phase 4.
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe exporter adds a local Prometheus registry, an HTTP metrics server, driver and stream instrumentation, active-session tracking, and configurable CLI bind-address propagation. ChangesExporter metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The metrics exporter change has no actionable merge-blocking risk identified at the current head and is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CLI as run command
participant Child as exporter child
participant Server as metrics HTTP server
participant Registry as MetricsRegistry
participant Driver as driver handler
CLI->>Child: pass metrics_bind_address
Child->>Server: start metrics server
Server->>Registry: expose /metrics
Driver->>Registry: record operations and stream bytes
Registry-->>Server: return OpenMetrics data
Poem
🚥 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 |
bennyz
reviewed
Jul 30, 2026
raballew
requested changes
Jul 31, 2026
Re-raise grpc AbortError before generic handlers to avoid client-controlled operation label cardinality, tighten bounded metric label types, default metrics bind host to loopback, log session metric decrements on failure, and expand exporter metrics unit coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
python/packages/jumpstarter/jumpstarter/driver/base.py (1)
161-173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecord unary success after response serialization.
encode_value(result)can raise after_record_operation_metrics(..., result="success")records success, then the generic handler records the same RPC asresult="failure". Build theDriverCallResponsebefore recording success.🤖 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 `@python/packages/jumpstarter/jumpstarter/driver/base.py` around lines 161 - 173, The unary operation handler currently records success before response serialization can fail. In the surrounding operation flow, construct the DriverCallResponse and run encode_value(result) before calling _record_operation_metrics with result="success"; keep the success log and return after serialization succeeds so encoding failures are handled only as failures.
🤖 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 `@python/packages/jumpstarter/jumpstarter/driver/base.py`:
- Around line 161-173: The unary operation handler currently records success
before response serialization can fail. In the surrounding operation flow,
construct the DriverCallResponse and run encode_value(result) before calling
_record_operation_metrics with result="success"; keep the success log and return
after serialization succeeds so encoding failures are handled only as failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75605a6c-9f92-4070-b4a6-707a6f3fe110
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
python/packages/jumpstarter-cli/jumpstarter_cli/run.pypython/packages/jumpstarter/jumpstarter/driver/base.pypython/packages/jumpstarter/jumpstarter/exporter/session.pypython/packages/jumpstarter/jumpstarter/metrics/__init__.pypython/packages/jumpstarter/jumpstarter/metrics/metrics_test.pypython/packages/jumpstarter/jumpstarter/metrics/registry.pypython/packages/jumpstarter/jumpstarter/metrics/server.pypython/packages/jumpstarter/jumpstarter/streams/common.pypython/packages/jumpstarter/pyproject.toml
Avoid repeated exporter/exemplar context lookups inside the copy_stream loop now that metrics helpers are imported at module level.
Build DriverCallResponse (including encode_value) before recording success so serialization failures are counted only as failures, with a regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
Default jmp run --metrics-bind-address to :0 so concurrent exporters do not collide on 8080, and treat metrics listen bind errors as non-fatal so the exporter continues without /metrics when a fixed port is already taken.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
325-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd or verify command-level coverage for the new option.
The supplied tests cover
start_metrics_server, but not theruncommand boundary. If no command-level test exists, cover the default:0, the disable value0, and forwarding into_serve_with_exc_handling.As per coding guidelines, Python test files must provide comprehensive package test coverage.
🤖 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 `@python/packages/jumpstarter-cli/jumpstarter_cli/run.py` around lines 325 - 345, Add command-level tests for the `run` command that verify the default `metrics_bind_address` value `:0`, the disable value `0`, and forwarding the selected value into `_serve_with_exc_handling`. Reuse the existing CLI test fixtures and preserve coverage of the `run` boundary in addition to `start_metrics_server`.Source: Coding guidelines
🤖 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 `@python/packages/jumpstarter/jumpstarter/metrics/metrics_test.py`:
- Around line 210-217: Update start_metrics_server and
test_metrics_server_ephemeral_bind_returns_concrete_port so the test receives a
shutdown handle along with the address, then invoke that handle in a finally
block surrounding the URL request and assertions to close the listening socket
and background thread.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/run.py`:
- Around line 325-345: Add command-level tests for the `run` command that verify
the default `metrics_bind_address` value `:0`, the disable value `0`, and
forwarding the selected value into `_serve_with_exc_handling`. Reuse the
existing CLI test fixtures and preserve coverage of the `run` boundary in
addition to `start_metrics_server`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c82536c3-1000-4fc8-915c-1965ef93b839
📒 Files selected for processing (3)
python/packages/jumpstarter-cli/jumpstarter_cli/run.pypython/packages/jumpstarter/jumpstarter/metrics/metrics_test.pypython/packages/jumpstarter/jumpstarter/metrics/server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter/jumpstarter/metrics/server.py
raballew
reviewed
Aug 13, 2026
…d asserts client.on still succeeds
Stop the metrics HTTP server from tests and jmp run so listen sockets are not leaked after exporter exit.
Parse failures (ValueError) now share the bind-failure path so a bad --metrics-bind-address cannot crash the exporter. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid a race where the asyncio path and metrics HTTP thread each create a MetricsRegistry and discard counters from the first instance. Co-authored-by: Cursor <cursoragent@cursor.com>
Clear the exporter correlation field after decrementing active sessions so it does not leak into later sessions or logs. Co-authored-by: Cursor <cursoragent@cursor.com>
Centralize DriverCall/StreamingDriverCall error mapping, move reset_registry_for_tests to metrics/_testing, restore log_message(format=), hoist session/stream imports, tighten exemplar regex asserts, add metrics HTTP request timeout and backlog limits, and rename inc_active_sessions to adjust_active_sessions. Co-authored-by: Cursor <cursoragent@cursor.com>
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
3 participants
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.
prometheus_clientregistry with JEP-named series:jumpstarter_operations_total,jumpstarter_operation_duration_seconds,jumpstarter_operation_errors_total,jumpstarter_stream_bytes_total,jumpstarter_active_sessions, plus exemplars (client,lease_id).GET /metricson the exporter process for lab/dev scrape (same registry Phase 3 will later reverse-scrape via MetricsStream).