feat: exporter prometheus_client /metrics (JEP-0013 Phase 2) - #934
feat: exporter prometheus_client /metrics (JEP-0013 Phase 2)#934RoddieKieley wants to merge 13 commits into
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:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Review Summary — PR #934: Exporter Prometheus metrics (JEP-0013 Phase 2)
This PR adds exporter-local Prometheus metrics with a prometheus_client registry, an HTTP /metrics endpoint, and instrumentation of the DriverCall/StreamingDriverCall/Stream code paths. Previous review feedback (26 threads from raballew and CodeRabbit) has been thoroughly addressed.
JEP-0013 Compliance Assessment
What Phase 2 should deliver per the JEP:
/metricsscrape endpoints on Controller and Router (covered by PR #933)- Exporter-local
prometheus_clientcounters/histograms/gauges withdriver_type - Prometheus exemplars for
client,lease_id,trace_id(when present)
Metric series implemented vs. JEP specification:
| JEP Series | Implemented | Notes |
|---|---|---|
jumpstarter_operations_total |
Yes | Labels: exporter, operation, result, driver_type — matches JEP |
jumpstarter_operation_duration_seconds |
Yes | Histogram with default buckets — matches JEP |
jumpstarter_operation_errors_total |
Yes | Labels include error_type — matches JEP |
jumpstarter_stream_bytes_total |
Yes | Labels: exporter, driver_type, direction — matches JEP |
jumpstarter_active_sessions |
Yes | Gauge with exporter label — matches JEP |
jumpstarter_lease_acquisitions_total |
No | Controller-side metric — not in scope for this PR |
jumpstarter_telemetry_dropped_total |
No | Phase 3 (Telemetry service) — correctly deferred |
jumpstarter_scrape_timeouts_total |
No | Phase 3 (Telemetry service) — correctly deferred |
Gaps / deviations from JEP:
trace_idexemplar: JEP liststrace_idas an exemplar key. This PR only implementsclientandlease_id. Likely a Phase 3+ concern.driver_typeunbounded: JEP specifies adriverTypeEnumallowlist with remap to"other". The implementation accepts any string. The codebase already has 6 driver types outside the JEP's predefined set (automotive,bluetooth,gpio,debug,shell,testing). No remapping is done.operationunbounded: The label comes from@exportmethod names. Bounded per-process by loaded drivers but not validated against an allowlist.
All three gaps are documented in inline comments. None are blockers for Phase 2.
What looks good
- Isolation from RPC path:
_record_operation_metricswraps all metrics calls in try/except, ensuring metrics failures never discard gRPC responses or change abort status codes. This is tested bytest_driver_call_succeeds_when_metrics_recording_raises. - AbortError bypass: The
grpc.aio.AbortErrorcatch before the genericexcept Exceptionprevents cardinality explosion from client-controlled invalid method names. Tested bytest_unknown_driver_method_does_not_record_operation_metric. - Encode-before-record: Success metrics are recorded only after the response is encoded, so serialization failures correctly count as failures.
- Dedicated registry: Uses a separate
CollectorRegistry(notprometheus_client.REGISTRY), avoiding pollution from/to default Go-style process metrics. This is the right choice for the exporter. - Non-fatal server binding: Metrics server bind failures log a warning and return
("", None), so the exporter continues operating without metrics. - Error type consolidation: The
_DRIVER_CALL_ERRORStable replaces duplicated exception handling blocks inDriverCallandStreamingDriverCall, reducing code from ~90 lines to ~35 — a nice refactor. - Stream metrics:
copy_streamcaptures metrics context once before the loop, not per-chunk, avoiding repeated structlog lookups in the hot path. - Test coverage: 400+ lines of tests covering the registry, server lifecycle, driver error mapping, exemplars, cardinality protection, and graceful degradation.
Findings (see inline comments)
driver_typeunbounded — No remapping of unknown driver types to"other"as the JEP specifies. Not a blocker but worth tracking.operationlabel unbounded — Bounded in practice by loaded drivers but not documented/validated.error_typetype safety —_record_operation_metricsacceptsstr | Nonerather thanErrorType | None, so the Literal types can't catch typos.- Stream metrics hot-path — Per-chunk
Counter.inc()could add overhead for high-throughput streams; monitor and batch if needed. - Ephemeral port default —
--metrics-bind-addressdefaults to:0(always enabled, random port). Question about intent for controller-mode exporters. trace_idexemplar missing — JEP includes it; presumably Phase 3.
No blocking issues. The implementation is well-aligned with JEP-0013 Phase 2 scope.
AI generated, human reviewed/modified.
| exporter: str, | ||
| operation: str, | ||
| result: OperationResult, | ||
| driver_type: str, |
There was a problem hiding this comment.
This comment can be ignored, the plan is to do it metrics server side, and anyway I believe the bigger problem is going to be the operations. (previous comment L93)
JEP-0013 conformance — driver_type is unbounded
The JEP specifies a bounded driverTypeEnum allowlist (storage, power, network, serial, console, video, composite) with unmapped values remapped to "other". However driver_type here is accepted as a plain str with no validation.
In practice, the codebase already has drivers using automotive, bluetooth, gpio, debug, shell, and testing — all outside the JEP's predefined set. There's no enforcement at the registry level, so each unique driver_type string creates new Prometheus label combinations, growing series cardinality.
The JEP envisions server-side remapping in spec.telemetry.metrics.driverTypeEnum, but that's a Phase 5 concern. For Phase 2, consider at least:
- Documenting that
driver_typecardinality is bounded by the number of loaded drivers (finite per process), or - Adding a
_KNOWN_DRIVER_TYPESset and remapping unknown values to"other"inrecord_operation().
Not a blocker — just flagging a gap between the JEP's design and the current implementation.
AI generated, human reviewed/modified.
| self, | ||
| *, | ||
| exporter: str, | ||
| operation: str, |
There was a problem hiding this comment.
JEP-0013 conformance — operation label is also unbounded
The operation label comes from request.method in DriverCall, which is the driver method name (e.g., "on", "off", "flash"). While the grpc.aio.AbortError catch prevents recording metrics for unknown methods rejected by __lookup_drivercall, the set of valid operations is still unbounded across all driver types.
The JEP mentions operation in the label set but doesn't explicitly constrain it. In practice this is bounded by the number of @export methods across all loaded drivers per process, so it's unlikely to cause cardinality explosion. But it's worth documenting this assumption.
AI generated, human reviewed/modified.
There was a problem hiding this comment.
This is true... We may want to keep an eye on this and move them to exemplars if necessary later. I would make a note of this on the JEP somewhere.
May be after the Unresolved questions, add a Risks section with a note on evaluating how this works? https://jumpstarter.dev/main/contributing/jeps/JEP-0013-observability-telemetry-logs.html#unresolved-questions
| metrics_registry = get_registry() | ||
| async for v in src: | ||
| if metrics_direction is not None: | ||
| metrics_registry.add_stream_bytes( |
There was a problem hiding this comment.
Good that the metrics context (exporter, exemplars, registry) is captured once before the loop rather than per-chunk. This avoids per-iteration structlog lookups in the hot path.
However, add_stream_bytes is called for every chunk in the stream. For high-throughput streams (e.g., storage flashing), this means one Counter.inc() call per chunk. The prometheus_client Counter is thread-safe (uses a lock internally), so this could introduce contention if tx and rx run concurrently in the task group.
For Phase 2 this is likely fine (the contention is on different label sets so different _ValueClass instances). Just something to monitor if stream throughput becomes a concern — batching byte counts and flushing periodically would reduce overhead.
AI generated, human reviewed/modified.
There was a problem hiding this comment.
Can we add this to risks as well?, if we notice degradation in flashing performance we may want to batch the increments.
| from prometheus_client import CollectorRegistry, Counter, Gauge, Histogram | ||
| from prometheus_client.openmetrics.exposition import generate_latest as generate_latest_openmetrics | ||
|
|
||
| DEFAULT_EXEMPLAR_KEYS = ("client", "lease_id") |
There was a problem hiding this comment.
JEP-0013 conformance — exemplar keys
The JEP lists exemplar keys as client, lease_id, and trace_id (when present). This implementation only includes client and lease_id. The trace_id is presumably a Phase 3+ concern (telemetry service), but it's worth noting the gap for tracking purposes.
AI generated, human reviewed/modified.
| server = ThreadingHTTPServer((host, port), Handler) | ||
| server.request_queue_size = _METRICS_REQUEST_QUEUE_SIZE | ||
| server.timeout = _METRICS_REQUEST_TIMEOUT_S | ||
| except (OSError, ValueError) as e: |
There was a problem hiding this comment.
I think this should be fatal, if we want metrics this should become a more evident error to the administrator. Probably if we exit, a new random port will be tried, this risk could be avoided by listening on a unix socket or memory stream if that's possible with the prometheus stack.
| "Operation failed", | ||
| extra={"operation": op, "driver_type": self.driver_type, | ||
| "result": "failure", "error_type": "connection_error"}, | ||
| self._record_operation_metrics( |
There was a problem hiding this comment.
The try/except wrapping around _record_operation_metrics is critical and well-designed — a metrics failure must never discard an already-computed gRPC response. The test test_driver_call_succeeds_when_metrics_recording_raises validates this, which is excellent.
One subtlety: this line falls through to _record_operation_metrics which itself has a try/except. So there's double protection — the method-level wrapper and the internal wrapper. That's fine for defense-in-depth, but the outer wrapper here might mask bugs in the metrics code during development. Consider logging at DEBUG rather than WARNING for the inner catch if both levels are active.
AI generated, human reviewed/modified.
| await exporter.serve() | ||
| except* Exception as excgroup: | ||
| _handle_exporter_exceptions(excgroup) | ||
| from jumpstarter.metrics import start_metrics_server |
There was a problem hiding this comment.
why do we import here and not at the top?
| default=False, | ||
| help="Exit after the current lease ends instead of waiting for a new one.", | ||
| ) | ||
| @click.option( |
There was a problem hiding this comment.
more than an option itself, this should be something internal, that the exporter decides to start or not based on the availability of a metrics endpoint in jumpstarter.
What I mean is that this is an internal detail, the exporter should recognize if the endpoint should be created, probably we don't even need to listen on a TCP port, since the exporter itself will funnel the scrapping requests down to the metrics server. Could be a unix socket, or a memory stream.
Probably until we add a mechanism to look at the server availability of a metrics endpoint we can start it always by default in the main branch.
| self, | ||
| *, | ||
| exporter: str, | ||
| operation: str, |
There was a problem hiding this comment.
This is true... We may want to keep an eye on this and move them to exemplars if necessary later. I would make a note of this on the JEP somewhere.
May be after the Unresolved questions, add a Risks section with a note on evaluating how this works? https://jumpstarter.dev/main/contributing/jeps/JEP-0013-observability-telemetry-logs.html#unresolved-questions
| metrics_registry = get_registry() | ||
| async for v in src: | ||
| if metrics_direction is not None: | ||
| metrics_registry.add_stream_bytes( |
There was a problem hiding this comment.
Can we add this to risks as well?, if we notice degradation in flashing performance we may want to batch the increments.
- 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.
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.
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.
…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>
Co-authored-by: Cursor <cursoragent@cursor.com>
e6fca45 to
35674c9
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
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).