JEP-0013 Phase 3 - operator image - #1061
Open
RoddieKieley wants to merge 3 commits into
Open
RoddieKieley wants to merge 3 commits into
RoddieKieley wants to merge 3 commits into
Conversation
Add the MetricsStream protocol and Go hub so Prometheus can scrape merged exporter OpenMetrics from telemetry without an exporter client yet. Generated Python stubs are included for proto consistency. Co-authored-by: Cursor <cursoragent@cursor.com>
Stop silently dropping unparseable exporter snapshots. Log the exporter and error, and increment jumpstarter_metrics_parse_errors_total so reverse-scrape omissions are visible on the same /metrics response. Co-authored-by: Cursor <cursoragent@cursor.com>
…e 3) Build /telemetry into the controller image and have the operator mount cert-manager TLS, advertise the CA, and expose scrape flags plus the HTTP metrics port so reverse-scrape can run in-cluster.
Contributor
|
Warning Review limit reachedNext included review available in 54 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (23)
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 |
This was referenced Sep 2, 2026
RoddieKieley
marked this pull request as ready for review
September 2, 2026 23:21
raballew
reviewed
Sep 8, 2026
raballew
left a comment
raballew
left a comment
Member
There was a problem hiding this comment.
Just a few comments for now. Waiting for merge of the first PR.
Comment on lines
+326
to
+332
| return []string{ | ||
| fmt.Sprintf("--grpc-bind=:%d", telemetryPort), | ||
| fmt.Sprintf("-metrics-bind-address=:%d", telemetryMetricsPort), | ||
| fmt.Sprintf("-scrape-timeout=%s", timeout), | ||
| fmt.Sprintf("-driver-type-enum=%s", strings.Join(enum, ",")), | ||
| fmt.Sprintf("-exemplar-keys=%s", strings.Join(keys, ",")), | ||
| } |
Member
There was a problem hiding this comment.
Mixed use of dash and double-dash
Comment on lines
+342
to
+345
| // Max wait for parallel exporter MetricsStream responses during a /metrics fan-out. | ||
| // Should be lower than the Prometheus scrape_timeout. | ||
| // +kubebuilder:default="7s" | ||
| ScrapeTimeout *metav1.Duration `json:"scrapeTimeout,omitempty"` |
Member
There was a problem hiding this comment.
Also set an upper limit to avoid keeping connections open for very long times.
Member
There was a problem hiding this comment.
2 minutes max? (and sounds excessive)
Comment on lines
280
to
284
| srv := grpc.NewServer( | ||
| grpc.Creds(creds), | ||
| grpc.ChainUnaryInterceptor(recovery.UnaryServerInterceptor()), | ||
| grpc.ChainStreamInterceptor(recovery.StreamServerInterceptor()), | ||
| ) |
Member
There was a problem hiding this comment.
Adding grpc.MaxRecvMsgSize to the server options would provide an explicit limit especially if you are running 1000s of exporters that are scraped concurrently (each with up to 4 MB per message). Otherwise this might cause some OOM killed processes.
Comment on lines
107
to
116
| func (s *TelemetryService) PushLogs(ctx context.Context, req *pb.PushLogsRequest) (*pb.PushLogsResponse, error) { | ||
| token, err := authentication.BearerTokenFromContext(ctx) | ||
| id, err := s.authenticateExporter(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Validate token and extract the subject (format: exporter:namespace:name:uid). | ||
| subject, err := s.Signer.ParseSubject(token) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Unauthenticated, "invalid token: %v", err) | ||
| } | ||
|
|
||
| // Only exporter tokens are allowed to push logs. Any other validly-signed | ||
| // token (e.g. a client token) is rejected immediately so that the identity | ||
| // checks below always have a non-empty claimedName/claimedNamespace. | ||
| parts := strings.SplitN(subject, ":", 4) | ||
| if len(parts) != 4 || parts[0] != "exporter" { | ||
| return nil, status.Errorf(codes.PermissionDenied, "token is not an exporter token") | ||
| } | ||
| claimedNamespace := parts[1] | ||
| claimedName := parts[2] | ||
| if claimedNamespace == "" || claimedName == "" { | ||
| return nil, status.Errorf(codes.PermissionDenied, "token has incomplete exporter identity") | ||
| } | ||
| claimedNamespace := id.namespace | ||
| claimedName := id.name | ||
|
|
||
| // Use context-based logger so tests can inject their own via logf.IntoContext. | ||
| logger := log.FromContext(ctx).WithName("telemetry") |
Member
There was a problem hiding this comment.
entry.Message, entry.Component, entry.Severity, entry.Lease, entry.Client, entry.Operation, entry.Result, entry.DriverType have no size bound. Should we truncate them too?
Comment on lines
+334
to
+341
| // Allowlist of keys to include in Prometheus exemplars. Unlisted keys are omitted. | ||
| // +kubebuilder:default={"client","lease_id"} | ||
| ExemplarKeys []string `json:"exemplarKeys,omitempty"` | ||
|
|
||
| // Allowed driver_type label values. Unlisted types are remapped to "other". | ||
| // +kubebuilder:default={"power","storage","network","serial","console","video","composite"} | ||
| DriverTypeEnum []string `json:"driverTypeEnum,omitempty"` | ||
|
|
Member
There was a problem hiding this comment.
A user with CR write access can set either list to an arbitrarily large array for ExemplarKeys []string and DriverTypeEnum []string. Adding // +kubebuilder:validation:MaxItems=<insert a good limit here> and // +kubebuilder:validation:MaxLength=<insert a good limit here> per item would enforce a bound.
Member
There was a problem hiding this comment.
Could be worth limiting to 16 for example? /8 default + 8 custom, more than enough.
maboras-rh
pushed a commit
to maboras-rh/jumpstarter
that referenced
this pull request
Sep 22, 2026
## Summary
JEP-0013 Phase 3 PR A**: the `MetricsStream` contract and the telemetry
reverse-scrape hub. This is the merge-base for the rest of the Phase 3
series.
Related PRs will be linked here as they are opened (B, C, D, E). This PR
stays draft until those links are filled in.
- Add `MetricsStream` to `telemetry.proto` (register + scrape
request/response) and regenerate Go + Python stubs.
- Telemetry fans out scrapes over connected streams, merges OpenMetrics,
remaps unknown `driver_type` to `other`, and serves `GET /metrics` on a
dedicated HTTP port (not gRPC `:9093`).
- `/healthz` and `/readyz` as in DD-7.
`jumpstarter_scrape_timeouts_total` on scrape timeout.
- **Lab follow-up in the same PR:** stop silently dropping unparseable
exporter snapshots. Log `exporter metrics snapshot omitted` and
increment `jumpstarter_metrics_parse_errors_total{exporter}` on the same
`/metrics` response. Python `prometheus_client` OpenMetrics exemplars
(`# {lease_id=...}`) still fail `parseMetricFamilies`; the snapshot is
omitted so one exporter cannot 500 the hub. Fixing OpenMetrics/exemplar
parse is a later pass — not this PR.
No exporter client, no operator/image/CRD, no Loki in this PR.
## DEMO
An asciinema demo for this combined Phase 3 work is available in the[
jep-0013-phase3-demo
branch](https://github.com/RoddieKieley/jumpstarter/tree/jep-0013-phase3-demo)
in my repository. Best to [read the description in the DEMO.md
there](https://github.com/RoddieKieley/jumpstarter/blob/jep-0013-phase3-demo/DEMO.md)
and then watch (manually for now) the asciinema file contained at the
end -
[jep-0013-phase3-demo.cast](https://github.com/RoddieKieley/jumpstarter/blob/jep-0013-phase3-demo/jep-0013-phase3-demo.cast)
### IMPORTANT NOTES
* See [MetricsStream during the
lease](https://github.com/RoddieKieley/jumpstarter/blob/jep-0013-phase3-demo/DEMO.md#metricsstream-during-the-lease)
for an important caveat about the implementation about a potential
limitation pointed out in the original JEP-0013 DD-3 regarding potential
limitations for exemplar support.
* Register identity must be exporter_name after jumpstarter-dev#1058 (not Metadata.name
/ "unknown"). That is in PR C.
## How this PR fits the series
```mermaid
flowchart TB
A["PR A this PR: proto + hub"]
B["PR B: image + metrics port + scrape CR"]
C["PR C: exporter MetricsStream client"]
D["PR D: Loki HTTP push"]
E["PR E: jmp PushLogs"]
A --> B
A --> C
B --> D
D --> E
```
| PR | Branch | Status |
|----|--------|--------|
| **A** | `jep-0013-phase3-metricsstream` | **This PR** |
| B | `jep-0013-phase3-operator-image` | jumpstarter-dev#1061 |
| C | `jep-0013-phase3-exporter-metricsstream` | jumpstarter-dev#1062 |
| D | `jep-0013-phase3-loki-push` | jumpstarter-dev#1063 |
| E | `jep-0013-phase3-client-pushlogs` | jumpstarter-dev#1064 |
Reverse-scrape is useful only after **A+B+C**. This PR is still
reviewable alone with mock streams.
## Data flow (this PR)
```mermaid
sequenceDiagram
participant Prom as Prometheus
participant Tel as jumpstarter-telemetry
participant Exp as Exporter stream (PR C)
Prom->>Tel: GET /metrics
Tel->>Exp: MetricsScrapeRequest
Exp-->>Tel: OpenMetrics snapshot
alt parse OK
Tel-->>Prom: merged families + scrape_timeouts
else OpenMetrics exemplar parse fail
Tel-->>Prom: snapshot omitted + parse_errors_total
end
```
## Out of scope / later passes
- Exporter client (C), operator image/metrics port (B), Loki (D), `jmp`
logs (E)
- OpenMetrics exemplar parse (hub still omits those snapshots; now
visible)
- `ServiceMonitor` (Phase 5), driver telemetry API (Phase 4),
multi-replica sticky streams (DD-8)
- Out-of-cluster telemetry Route
## NOTES
### Known merge work (not new features)
* jumpstarter-dev#1062 conflicts with main on exporter.py because of
[jumpstarter-dev#1059](jumpstarter-dev#1059). When
C rebases, keep identity=exporter_name, _session_labels(), and jumpstarter-dev#1059’s
exporter_name= on Session(...).
* [jumpstarter-dev#1027](jumpstarter-dev#1027)
still overlaps operator telemetry files with B/D.
The series is still based on jumpstarter-dev#1058. After A lands, rebase the rest onto
current main (jumpstarter-dev#1059) as a set.
### Explicitly later (already called out as out of scope)
* OpenMetrics exemplar parse (A omits those snapshots; JEP DD-3)
* ServiceMonitor (Phase 5), driver telemetry API (Phase 4)
* Out-of-cluster telemetry Route (demo-only)
* Changing PushLogs logger.Error(nil, …) for exporter TFTP errors
* Cosmetic only: jumpstarter-dev#1060 still has the[ jep-0013-phase3-demo.
---------
Signed-off-by: Roddie Kieley <rkieley@redhat.com>
Signed-off-by: Miguel Angel Ajo Pelayo <miguelangel@ajo.es>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Miguel Angel Ajo Pelayo <miguelangel@ajo.es>
This branch has not been deployed
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.
Summary
JEP-0013 Phase 3 PR B**: make the MetricsStream hub from PR A runnable in-cluster.
Depends on PR A: #1060
Do not merge until A is on
main, then rebase this branch ontomain. Related PRs C–E will be linked here as they are opened./telemetryincontroller/Containerfileandmake build/docker-build-ci(operator alreadyCommand: ["/telemetry"];mainstill only shipsmanager+router)./healthzand/readyzprobes on that port (replacing TCP probes on gRPC:9093).spec.telemetry.metrics.scrapeTimeout,driverTypeEnum,exemplarKeys. Defaults match JEP-0013 (7s, the driver-type enum,client,lease_id). NoServiceMonitor(Phase 5).GRPC_TELEMETRY_ENDPOINTso the telemetry process advertises the in-cluster Service DNS.TLS: #1023 already landed telemetry TLS on
main. This PR keeps that path (cert-manager or manualspec.telemetry.grpc.tls.certSecret, rolling-restart hash, non-fatal missing CA). It does not reimplement TLS.Lab: in-cluster reverse-scrape of
GET /metricson Service port 8080 worked (port-forward). After driver ops, Python OpenMetrics exemplars still fail GoparseMetricFamilies; the hub from A omits that snapshot and incrementsjumpstarter_metrics_parse_errors_total.spec.telemetry.metrics.exemplarKeysonly allowlists keys on the merge path after a successful parse — it does not fix exemplar decode. That remains A / JEP DD-3, not this PR.DEMO
The lab walkthrough for the stacked Phase 3 work (A+B+C+D+E plus lab-only Route) is on the fork demo branch, not this PR:
See MetricsStream during the lease for the DD-3 exemplar limitation observed on
:8080.How this PR fits the series
jep-0013-phase3-metricsstreamjep-0013-phase3-operator-imagejep-0013-phase3-exporter-metricsstreamjep-0013-phase3-loki-pushjep-0013-phase3-client-pushlogsUnique work vs PR A: the operator/image commit on this branch (
435eebee). If GitHub shows A's commits, that is stacking againstmain; review that unique commit only.Out of scope
identity(C) — after#1058, identity must beexporter_name, notMetadata.nameServiceMonitor(Phase 5)NOTE
Open #1027 also touches telemetry operator files (log-ingest e2e).