Add exporter status reporting #27
Conversation
WalkthroughMoves ExporterStatus into a new shared proto, updates Exporter.status to reference it, and adds ReportStatus and GetStatus RPCs plus log source metadata to jumpstarter.v1 proto definitions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Exporter
participant Controller as ControllerService
note over Exporter,Controller: Exporter reports its status to controller
Exporter->>Controller: ReportStatus(ReportStatusRequest{status, message})
Controller-->>Exporter: ReportStatusResponse{}
sequenceDiagram
autonumber
actor Client
participant ExporterSvc as ExporterService
note over Client,ExporterSvc: Client queries exporter status
Client->>ExporterSvc: GetStatus(GetStatusRequest{uuid})
ExporterSvc-->>Client: GetStatusResponse{status, message, ts}
sequenceDiagram
autonumber
actor Client
participant ExporterSvc as ExporterService
note over Client,ExporterSvc: LogStream responses now include optional source metadata
Client-)ExporterSvc: LogStream(...)
ExporterSvc-->>Client: LogStreamResponse{line, source?}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (6)
proto/jumpstarter/v1/common.proto (2)
3-6: Add language-specific package options (e.g., go_package/java_package) to avoid generator/import issues.
Without go_package (and peers), Go and other generators can produce inconsistent import paths or conflicting packages.Consider adding, aligned with existing files:
- option go_package = "…/proto/jumpstarter/v1;jumpstarterv1";
- option java_package = "dev.jumpstarter.v1";
- option csharp_namespace = "Jumpstarter.V1";
Please confirm what options are used in jumpstarter/v1/kubernetes.proto and mirror them here for consistency.
21-27: LogSource enum looks good; consider noting extensibility guidance.
Optional: add a brief note that new sources may be appended and consumers should treat unknown values gracefully.proto/jumpstarter/v1/jumpstarter.proto (4)
25-28: ReportStatus RPC: clarify auth/trust model and call frequency.
Document expected authentication (mTLS/token) and whether exporters should report periodically or on change; this affects server load and storage semantics.
117-121: Consider including a timestamp in ReportStatusRequest.
If the controller and exporter clocks can diverge or messages are delayed, a client-supplied timestamp can help order events; the server can still record its receive time.
134-139: Naming consistency: align Status/StatusStream/GetStatus across services.
ControllerService has Status(stream StatusResponse) for lease info, while ExporterService introduces GetStatus/StatusStream for exporter status. Consider renaming to disambiguate (e.g., LeaseStatus/LeaseStatusStream vs ExporterStatus/GetExporterStatus).Please confirm if API consumers might confuse these similarly named methods.
224-230: GetStatusResponse: consider optionally mirroring timestamp for parity.
StatusStreamResponse includes timestamp; parity in GetStatusResponse can simplify clients that don’t use the stream.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
proto/jumpstarter/client/v1/client.proto(2 hunks)proto/jumpstarter/v1/common.proto(1 hunks)proto/jumpstarter/v1/jumpstarter.proto(7 hunks)
🔇 Additional comments (7)
proto/jumpstarter/client/v1/client.proto (2)
21-21: Importing common.proto is correct; ensure codegen/build includes it.
Looks good. Verify buf/bazel/make rules include the new proto path so generators pick up jumpstarter/v1/common.proto.If you use buf, confirm common.proto is in your module roots; with bazel, ensure a proto_library target includes it.
72-72: Confirm shared ExporterStatus numeric mapping
Field 4 is wire-compatible; cross-check that the values in proto/jumpstarter/v1/common.proto’s ExporterStatus exactly match the previous local enum definition to avoid semantic regressions.proto/jumpstarter/v1/common.proto (1)
10-18: Enum design LGTM; defaults and append-only numbering are appropriate.
Clear values with 0 as UNSPECIFIED and stable numbering. Good for cross-service reuse.proto/jumpstarter/v1/jumpstarter.proto (4)
12-12: Importing common.proto here is correct.
This enables the shared enums across services.
67-70: Minor comment fix LGTM.
The uuid comment correction improves clarity.
180-185: Adding LogSource as field 4 is backward compatible; ensure clients ignore unknown fields.
Most clients do, but double-check any JSON/HTTP gateways that might require updates to include the new source field in responses.
231-237: Status streaming shape LGTM.
Status + optional message + timestamp is sufficient and future‑proof.
Summary by CodeRabbit
New Features
Documentation
Chores