feat: migrate RPC surface to Connect-RPC#174
Merged
Merged
Conversation
- plugin.go RPC() now returns (string, http.Handler) via metricsV1connect.NewMetricsServiceHandler from api-go v6.0.0-beta.9 - rpc.go reshaped to the Connect handler interface with 6 methods: Add / Sub / Observe / Set / Declare / Unregister - handler-side lookup with typed Connect codes: - unknown collector → CodeNotFound - operation unsupported by collector kind → CodeFailedPrecondition - missing labels on a *Vec collector → CodeInvalidArgument - prometheus registry errors → CodeInternal - vecMetric / vecObserver generic helpers consolidate the GaugeVec / CounterVec / HistogramVec / SummaryVec label-validation + lookup path - tests/metrics_test.go: 15 goridge client.Call sites + 14 client constructions collapsed into a single newMetricsClient helper; tests now call typed Connect methods with toProtoCollector translating metrics.NamedCollector → metricsV1.NamedCollector - new tests/metrics_api_test.go covers Connect, plain HTTP+protojson, plain gRPC against the same handler, plus HTTP-GET idempotency (all 6 methods POST-only — no GET candidates) - new tests/configs/.rr-metrics-api.yaml minimal config for the API tests - bump api-go to v6.0.0-beta.9, goridge to v4.0.0-beta.2, rpc/v6 to v6.0.0-beta.4, http/v6 to v6.0.0-beta.7 (typecheck-fix upstream) - replace http.NewRequest/http.Get with NewRequestWithContext + Client.Do in unrelated test helpers (lint surfaced after http/v6 typecheck started working)
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMigrates the metrics RPC from the legacy goridge in-process interface to a ConnectRPC HTTP/HTTP2 service using Protobuf types. Updates module deps (adds Connect, pins gRPC/protobuf), replaces the old Metric API and rpc wrappers with Connect handlers, rewires the plugin to expose an HTTP handler, and converts/extends tests to exercise Connect, HTTP, and gRPC endpoints. ChangesConnectRPC metrics API and tests
sequenceDiagram
participant Client as Client (Connect/HTTP/gRPC)
participant HTTP as Plugin HTTP handler
participant Plugin as Metrics Plugin (sync.Map)
participant Prom as Prometheus Registry
Note over Client,Prom: Declare / Add / Observe / Set / Unregister flows
Client->>HTTP: POST /metrics (Connect req proto)
HTTP->>Plugin: Invoke Connect handler (Declare/Add/...)
Plugin->>Plugin: lookupCollector(name) / buildPromCollector(...)
alt Declare new collector
Plugin->>Prom: Register collector
Prom-->>Plugin: OK / error
Plugin-->>HTTP: connect.Response{Ok: true} / error (mapped)
else Mutate metric (Add/Sub/Observe/Set)
Plugin->>Plugin: resolve collector type
Plugin->>Plugin: if vector -> vecMetric(labelValues) -> get metric instance
Plugin->>Prom: update metric or return unsupported
Prom-->>Plugin: OK / error
Plugin-->>HTTP: connect.Response{Ok:true} or Connect error code
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
There was a problem hiding this comment.
Pull request overview
Migrates the metrics plugin’s RPC interface from net/rpc/goridge to a Connect-RPC surface served over HTTP/2 (h2c), aligning the plugin with the metrics.v1.MetricsService API from api-go/v6.
Changes:
- Replaced the in-process RPC implementation with Connect-RPC handlers (
metricsV1connect.NewMetricsServiceHandler) and updated request/response handling + error mapping. - Updated existing tests to call the Connect client and added a new API test suite covering Connect, plain HTTP+protojson, and gRPC.
- Bumped/added Go module dependencies to support Connect-RPC, HTTP/2, and gRPC/protobuf in tests.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
rpc.go |
Implements Connect-RPC methods (Add/Sub/Observe/Set/Declare/Unregister) and helper functions for collector lookup/building. |
plugin.go |
Changes RPC() to return a Connect handler mount (string, http.Handler). |
tests/metrics_test.go |
Migrates existing RPC tests from goridge/net-rpc to a Connect client. |
tests/metrics_api_test.go |
Adds end-to-end API coverage for Connect, plain HTTP JSON, and gRPC, plus GET rejection. |
tests/configs/.rr-metrics-api.yaml |
Adds a dedicated config for the new API tests. |
go.mod / go.sum |
Adds Connect-RPC + related networking/proto deps. |
tests/go.mod / tests/go.sum |
Updates test-module deps to include Connect-RPC, gRPC, protojson, and bumps RoadRunner module versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- restore exact legacy log-message strings to preserve TestMetricsDifferentRPCCalls
count assertions ("metric successfully added", "subtracting value from metric",
"subtracting operation finished successfully", "observe operation finished
successfully", "set operation finished successfully", "declaring new metric",
"failed to get metrics with label values", "required labels for collector",
"collector was successfully unregistered")
- map GetMetricWithLabelValues / missing-labels errors to CodeInvalidArgument
(caller-input class, not internal)
- bump test config metrics.address 127.0.0.1:2115 → :2118 to avoid port
collision with .rr-metrics-http.yaml
- add Summary/SummaryVec Observe + unspecified-CollectorType Declare coverage
in TestMetricsConnectAPI
- drop rpc_test.go: msgpack/json wire-format tests for the deleted Go-side
Metric struct (relic of the goridge codec)
- end-of-migration go get -u all: server/v6 beta.5→beta.6, quic-go, sysconf
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@rpc.go`:
- Around line 45-53: The code currently forwards client-supplied metric value
m.GetValue() directly into Counter.Add (both in the prometheus.Counter case and
after vecMetric returns cv for *prometheus.CounterVec), which will panic if the
value is negative; before calling c.Add(...) or cv.Add(...), check if
m.GetValue() < 0 and return an error (e.g., fmt.Errorf("negative value for
counter: %v", m.GetValue())) to reject bad input; update the branch handling
prometheus.Counter and the branch that calls vecMetric (and then cv.Add) to
perform this validation and return the error rather than calling Add with a
negative value.
🪄 Autofix (Beta)
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
Run ID: 51fdf622-d328-4015-a820-69fb831fce64
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modplugin.gorpc.gotests/configs/.rr-metrics-api.yamltests/go.modtests/metrics_api_test.gotests/metrics_test.go
When prometheus.registry.Unregister returns false, the old goridge handler left the RPC return value at its zero (false) — collector removed from RR map but not from prometheus, caller could detect the divergence. I had collapsed both branches to Ok:true. Restoring the legacy contract.
rpc_test.go was removed in efb2ab1 (msgpack/json wire-format tests for the deleted Go-side Metric struct, goridge codec relic). The workflow's explicit file list still referenced it, so unit tests failed with 'no Go files' or 'file not found' on the runner.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rpc.go (1)
45-52:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject negative counter deltas before calling
Add.Line 46 and Line 52 pass client input directly into
Counter.Add; negative values can panic and crash the handler.Suggested fix
case prometheus.Counter: + if m.GetValue() < 0 { + return nil, connect.NewError(connect.CodeInvalidArgument, + fmt.Errorf("counter %s cannot be incremented by a negative value", m.GetName())) + } c.Add(m.GetValue()) case *prometheus.CounterVec: cv, err := vecMetric(r, c, m) if err != nil { return nil, err } + if m.GetValue() < 0 { + return nil, connect.NewError(connect.CodeInvalidArgument, + fmt.Errorf("counter %s cannot be incremented by a negative value", m.GetName())) + } cv.Add(m.GetValue())In github.com/prometheus/client_golang v1.23.2, does Counter.Add panic when passed a negative value?🤖 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 `@rpc.go` around lines 45 - 52, Reject negative counter deltas before calling Add: check the incoming metric value (m.GetValue()) for < 0 in both branches handling prometheus.Counter and *prometheus.CounterVec (the code paths that call Counter.Add and cv.Add), and return a descriptive error instead of passing it to Counter.Add; for the CounterVec branch, call vecMetric(r, c, m) first as now but validate m.GetValue() is non-negative before invoking cv.Add. Ensure the error message references the metric and the negative value for easier debugging.
🤖 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 `@rpc.go`:
- Around line 98-113: The Observe switch in function Observe is missing handling
for scalar prometheus.Summary values: add a case for prometheus.Summary (similar
to the existing prometheus.Histogram case) that calls c.Observe(m.GetValue());
keep the existing *prometheus.SummaryVec branch for vector summaries and leave
the default error handling unchanged.
---
Duplicate comments:
In `@rpc.go`:
- Around line 45-52: Reject negative counter deltas before calling Add: check
the incoming metric value (m.GetValue()) for < 0 in both branches handling
prometheus.Counter and *prometheus.CounterVec (the code paths that call
Counter.Add and cv.Add), and return a descriptive error instead of passing it to
Counter.Add; for the CounterVec branch, call vecMetric(r, c, m) first as now but
validate m.GetValue() is non-negative before invoking cv.Add. Ensure the error
message references the metric and the negative value for easier debugging.
🪄 Autofix (Beta)
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
Run ID: acaabf7a-4f35-4b2b-be3c-1aff7ed49b51
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modrpc.gorpc_test.gotests/configs/.rr-metrics-api.yamltests/go.modtests/metrics_api_test.go
💤 Files with no reviewable changes (1)
- rpc_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/configs/.rr-metrics-api.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/metrics_api_test.go
…itch branch prometheus.Histogram and prometheus.Summary share identical method sets, so a scalar Summary value matches the existing case prometheus.Histogram branch (Go type-switch picks the first interface match in source order). The new test locks this in; the type-switch comment explains why a dedicated Summary case would be dead code (staticcheck SA4020).
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
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
Migrates the metrics plugin's RPC surface from net/rpc + goridge codec to Connect-RPC over HTTP/2 (h2c), using
metricsV1connect.NewMetricsServiceHandlerfrom api-go v6.0.0-beta.9.Add/Sub/Observe/SetMetricin its own dedicated request typeDeclareNamedCollector; idempotent on existing namesUnregisterNo GET candidates — every method mutates Prometheus registry state. Scraping continues to happen on the separate Prometheus HTTP endpoint configured under
metrics.address.Typed Connect error codes from the handler:
CodeNotFoundSeton a Histogram) →CodeFailedPrecondition*Veccollector →CodeInvalidArgumentCodeInternalAll three wire protocols reachable on the same
rpc.listenport. Newtests/metrics_api_test.gocovers Connect / plain HTTP+protojson / plain gRPC, plus an HTTP-GET idempotency table verifying all six methods reject GET.Breaking changes
Wire protocol change: callers using
net/rpc+ goridge codec must switch to a Connect / HTTP / gRPC client targeting/metrics.v1.MetricsService/{Add,Sub,Observe,Set,Declare,Unregister}. Note: the wire encoding ofCollector.Objectiveschanges frommap<double, double>(Go-side) torepeated Objective { quantile, error }(proto3 forbids float-keyed maps).Dep bumps:
github.com/roadrunner-server/api-go/v6→v6.0.0-beta.9github.com/roadrunner-server/goridge/v4→v4.0.0-beta.2github.com/roadrunner-server/rpc/v6(tests) →v6.0.0-beta.4github.com/roadrunner-server/http/v6(tests, transitive) →v6.0.0-beta.7Summary by CodeRabbit
New Features
Refactor
Tests
Chores