feat: migrate to Connect-RPC; new wire path /applogger.v2.AppLoggerService/#78
Merged
Conversation
…rvice/
Switches the app-logger plugin's RPC layer from net/rpc + goridge codec
to Connect-RPC over HTTP/2. Plugin.RPC() now returns the generated
apploggerV2connect.NewAppLoggerServiceHandler mounted at
/applogger.v2.AppLoggerService/ on the rpc plugin's HTTP/2 mux.
The 10 wire methods are reshaped to the Connect handler interface
(Error, Info, Warning, Debug, Log + their *WithContext variants).
Primitive-string variants take the domain-specific LogMessage proto
({string message = 1;}); the *WithContext variants keep LogEntry.
Log/LogWithContext wrap io.WriteString failures with
connect.NewError(connect.CodeInternal, err).
rpc_test.go: in-package unit tests rewritten to call methods through
the Connect signatures.
tests/app_logger_test.go: rewritten as a Go-side integration test that
boots rpc + applogger + config + mock logger, then drives log calls via
an h2c Connect client. Drops the previously required server + http
plugins and the PHP worker — those had no role in app-logger's RPC flow
and only existed because the old goridge wire was driven from PHP.
Configs (.rr-appl*.yaml): drop server/http blocks; keep rpc.listen + logs.
Deps:
- api-go/v6 v6.0.0-beta.4 -> v6.0.0-beta.5
- goridge/v4 v4.0.0-beta.1 -> v4.0.0-beta.2
- rpc/v6 v6.0.0-beta.3 -> v6.0.0-beta.4
- + connectrpc.com/connect, + golang.org/x/net (h2c transport)
Breaking changes:
- Wire protocol changes from goridge codec to Connect/gRPC over HTTP/2.
Downstream Go consumers must use apploggerV2connect.NewAppLoggerServiceClient.
PHP migrates separately on its own timeline.
- Primitive-string methods take LogMessage{Message: ...} instead of a
bare string.
The PHP test files under tests/php_test_files/ are no longer referenced
by any test config but remain in the tree as a template for the future
PHP-side migration.
|
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 (2)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR migrates the app-logger plugin from legacy RPC handlers to ConnectRPC-compatible handlers. The ChangesConnectRPC Handler & HTTP Handler Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Migrates the app-logger plugin’s RPC surface from the legacy net/rpc + goridge codec to Connect-RPC handlers mounted under the generated service path (/applogger.v2.AppLoggerService/), and updates unit/integration tests plus test configs to exercise the new HTTP/2 (h2c) transport.
Changes:
- Replace net/rpc-style RPC methods with Connect-RPC handler method signatures and Connect error mapping for
Log/LogWithContext. - Update plugin RPC exposure to return the generated Connect handler (pattern +
http.Handler). - Rewrite tests/configs to use an h2c Connect client and remove now-unneeded
server/httpconfig blocks.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Bumps api-go to v6.0.0-beta.5 and updates indirect deps for the new RPC stack. |
go.sum |
Updates root module sums due to dependency changes. |
go.work.sum |
Updates workspace sums for new/changed dependencies. |
plugin.go |
Changes Plugin.RPC() to expose the generated Connect handler. |
rpc.go |
Converts RPC methods to Connect request/response handlers; wraps stderr write failures as Connect internal errors. |
rpc_test.go |
Updates unit tests to call Connect-style RPC methods. |
tests/app_logger_test.go |
Reworks integration tests to boot plugins and issue log calls via an h2c Connect client. |
tests/configs/.rr-appl.yaml |
Removes server/http blocks; keeps rpc.listen and logging config. |
tests/configs/.rr-appl-context.yaml |
Same config simplification for context-logging test case. |
tests/go.mod |
Updates test module deps to include Connect + newer RR modules. |
tests/go.sum |
Updates test module sums for the new client-side dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@go.mod`:
- Around line 7-10: rpc.go imports connectrpc.com/connect but go.mod doesn't
explicitly require it; add an explicit require entry for connectrpc.com/connect
in the require block of go.mod (matching the version currently used by your
build or CI) so the module graph is pinned and -mod=readonly CI won't fail;
locate rpc.go to confirm the import and update the require block in go.mod to
include connectrpc.com/connect at the appropriate version.
In `@tests/app_logger_test.go`:
- Around line 94-109: Replace the fixed time.Sleep calls around server startup
and after logging with readiness/eventual checks: instead of
time.Sleep(time.Second) before creating newAppLoggerClient, poll/connect with
require.Eventually (or a retry loop) until newAppLoggerClient("127.0.0.1:6001")
succeeds or the server port is reachable; after sending logs via
client.Debug/Info/Warning/Error, replace the trailing time.Sleep with
require.Eventually that asserts the expected log side-effect (e.g., a function
that verifies logs were emitted, a mock/collector received entries, or the
server health/last-received timestamp) and use t.Context() for deadlines; ensure
stopCh send remains but only after the eventual assertions pass.
- Around line 34-41: The http.Client instance "httpc" used to create the RPC
client (passed to NewAppLoggerServiceClient) has no timeout and can hang; set a
request timeout on httpc (e.g., httpc.Timeout = <reasonable duration>) or ensure
calls use a context with deadline before constructing the client so RPC calls
cannot block indefinitely; update the httpc initialization near
DialTLSContext/AllowHTTP to include the timeout and adjust any tests to respect
that deadline.
🪄 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: f8fe3bb4-5555-4e51-8792-f202544c0cf5
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modplugin.gorpc.gorpc_test.gotests/app_logger_test.gotests/configs/.rr-appl-context.yamltests/configs/.rr-appl.yamltests/go.mod
💤 Files with no reviewable changes (1)
- tests/configs/.rr-appl-context.yaml
Code review (pr-review-toolkit + bot-style triage): - newAppLoggerClient: add Timeout to http.Client to fail fast on unreachable rpc plugin endpoint instead of blocking until the test global deadline. - Replace fragile time.Sleep(time.Second) for rpc readiness with a waitForRPC helper that polls via net.Dialer.DialContext + retries via require.Eventually. - go mod tidy on root + tests: promotes connectrpc.com/connect to a direct require, drops the no-longer-used http/v6 and server/v6 direct deps from tests/go.mod. Trim PSR-3 comment in rpc.go to describe the implemented subset (error/info/warning/debug/log + *WithContext) rather than restating the full PSR-3 interface; producers can map emergency/alert/critical/ notice onto the implemented levels as needed. Simplify: - Unexport the RPC handler type — rename to `service` since it's constructed only inside the package and passed to the generated AppLoggerServiceHandler factory. No external consumer references it. - Extract the duplicated container-watcher goroutine (was identical in TestAppLogger and TestAppLoggerWithContext) into a serveContainer helper that returns a stop function. Cuts ~30 lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #78 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Inject the stderr writer onto the service struct (defaulted to os.Stderr in Plugin.RPC) so the io.WriteString failure branch in Log and LogWithContext is reachable from tests. Add TestRPCLogWriteFailureMaps- ToCodeInternal that injects an errWriter and asserts the response is nil, the returned error wraps via connect.CodeInternal, and errors.Is reaches the underlying cause. Drop the captureStderr helper that redirected os.Stderr globally — no longer needed now that TestRPCLog and the new TestRPCLogWithContext inject their own bytes.Buffer.
Use the domain-aware alias (apploggerV2) for the api-go applogger/v2 import instead of the bare v2 alias. v2 is ambiguous when multiple proto domains are imported in a single file and hides which domain each type belongs to. apploggerV2 matches the canonical name declared by the proto's go_package option.
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
Switches the app-logger plugin's RPC layer from net/rpc + goridge codec to Connect-RPC over HTTP/2.
Plugin.RPC()now returns the generatedapploggerV2connect.NewAppLoggerServiceHandlermounted at/applogger.v2.AppLoggerService/on the rpc plugin's HTTP/2 mux.The 10 wire methods are reshaped to the Connect handler interface (
Error,Info,Warning,Debug,Log+ their*WithContextvariants). Primitive-string variants take the domain-specificLogMessageproto ({ string message = 1; }); the*WithContextvariants keepLogEntry.Log/LogWithContextwrapio.WriteStringfailures withconnect.NewError(connect.CodeInternal, err).rpc_test.go: in-package unit tests rewritten to call methods through the Connect signatures.tests/app_logger_test.go: rewritten as a Go-side integration test that bootsrpc+applogger+config+ mock logger, then drives log calls via an h2c Connect client. Drops the previously requiredserver+httpplugins and the PHP worker — those had no role in app-logger's RPC flow and only existed because the old goridge wire was driven from PHP.Configs (
.rr-appl*.yaml): dropserver/httpblocks; keeprpc.listen+logs.Deps:
api-go/v6 v6.0.0-beta.4 → v6.0.0-beta.5goridge/v4 v4.0.0-beta.1 → v4.0.0-beta.2rpc/v6 v6.0.0-beta.3 → v6.0.0-beta.4+ connectrpc.com/connect,+ golang.org/x/net(h2c transport for tests)The PHP test files under
tests/php_test_files/are no longer referenced by any test config but remain in the tree as a template for the future PHP-side migration.Breaking changes
apploggerV2connect.NewAppLoggerServiceClient(or any gRPC client). PHP migrates separately on its own timeline.Error/Info/Warning/Debug/Log) takeLogMessage{Message: ...}instead of a bare string.Summary by CodeRabbit
Release Notes
Refactor
Tests
Chores