feat(rpc-metrics): unified RPC latency & error-rate metrics across all services#480
feat(rpc-metrics): unified RPC latency & error-rate metrics across all services#480hmchangw wants to merge 13 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Adds route/status plumbing to Context (pattern threaded through acquireContext, Route()/SetStatus()/Status() accessors) and stamps terminal status in Register/RegisterNoBody/RegisterVoid so the new Metrics(service) middleware can record rpc_server_requests_total and rpc_server_request_duration_seconds per request. Renamed the pre-existing helpers_test.go runChain to runHandlerChain to resolve a name collision with the new metrics_test.go runChain helper (test-only, no behavior change). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Add end-to-end tests that install Metrics() in front of real Register/RegisterNoBody/RegisterVoid handlers over an in-process NATS server, asserting rpc_server_requests_total increments on the correct status label for success, errcode errors, and invalid-JSON bodies. Closes the gap where a regression swapping/dropping a SetStatus call in register.go would pass every existing test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
… request metrics Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
…vices Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
…from prod; unify listen-fail handling Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
…elpers; dedup reply path Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re53EQYKpXS3m9tBkRhbVb
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds shared RPC metrics collectors and middleware, stamps route/status metadata through NATS and Gin handlers, exposes a separate ChangesRPC Metrics Rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as natsrouter/Gin Router
participant Metrics as Metrics Middleware
participant Handler
participant rpcmetrics
participant Prometheus
Client->>Router: Request
Router->>Metrics: c.Next()
Metrics->>Handler: invoke handler
Handler->>Handler: SetStatus / errhttp.Write stamps code
Handler-->>Metrics: reply/response
Metrics->>rpcmetrics: Observe(service, route, status, duration)
rpcmetrics->>Prometheus: increment counter, observe histogram
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/superpowers/specs/2026-07-08-rpc-metrics-design.md (1)
53-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the existing
/metricsstate.This says every service already serves the default registry on
/metrics, but the rollout still adds listeners in services that lack one. Reword this so readers can distinguish the registry from the listener wiring.🤖 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 `@docs/superpowers/specs/2026-07-08-rpc-metrics-design.md` around lines 53 - 55, Clarify the `/metrics` rollout wording in the design doc by separating the Prometheus registry setup from the HTTP listener wiring; the current text implies every service already exposes `/metrics`, which conflicts with services that still need a listener. Update the section around the raw Prometheus `promauto` guidance to explicitly say the default registry is already used in existing components like search-service and pkg/atrest, while listener/server wiring is still being added where missing.docs/superpowers/plans/2026-07-08-rpc-metrics.md (2)
118-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test workflow consistent with the global rule.
This step carves out a raw
go test -raceexception even though the plan says to usemaketargets only. Either document that exception once in the global constraints or rewrite the step to usemakeso the rollout stays self-consistent.🤖 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 `@docs/superpowers/plans/2026-07-08-rpc-metrics.md` around lines 118 - 123, The test-run step in the plan is inconsistent with the global workflow rule because it introduces a raw go test -race exception. Update the workflow around the rpcmetrics test step so it either uses a make target consistently or explicitly records the sanctioned raw-go exception once in the global constraints; adjust the step text that references the rpcmetrics package and its test command so the rollout instructions stay self-consistent.
849-860: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove
Metricsahead of the existing middleware chain.Registering it after
Default(...)/RequestIDstarts the timer too late and excludes earlier middleware from the sample. If you want end-to-end RPC latency, putMetricsfirst; otherwise rename the metric as handler-only latency.🤖 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 `@docs/superpowers/plans/2026-07-08-rpc-metrics.md` around lines 849 - 860, `Metrics` is currently being registered too late in the router middleware chain, so it misses earlier middleware time. Update each affected router setup to place `natsrouter.Metrics("<service>")` before the existing `Logging` middleware and as early as possible after `Recovery`/`RequestID`; in `user-presence-service/main.go`, move the metrics registration out of the `natsrouter.Default(...)`-only setup by adding a separate `router.Use(natsrouter.Metrics("user-presence-service"))` immediately after the default middleware is installed. Use the existing `router.Use(...)` chains in `room-service`, `room-worker`, `history-service`, `user-service`, and `user-presence-service` as the anchors for the change.
🤖 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 `@pkg/rpcmetrics/metrics_test.go`:
- Around line 59-68: TestObserve currently asserts a fixed CounterValue of 1,
which is not idempotent because it reads from the cumulative global Prometheus
registry. Update the test to capture the counter value before calling Observe,
invoke Observe with the same label tuple, then assert the after value equals
before plus 1, matching the pattern used in the other tests. Keep the existing
checks around requestDuration and use the TestObserve, Observe, and CounterValue
symbols to locate the change.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-07-08-rpc-metrics.md`:
- Around line 118-123: The test-run step in the plan is inconsistent with the
global workflow rule because it introduces a raw go test -race exception. Update
the workflow around the rpcmetrics test step so it either uses a make target
consistently or explicitly records the sanctioned raw-go exception once in the
global constraints; adjust the step text that references the rpcmetrics package
and its test command so the rollout instructions stay self-consistent.
- Around line 849-860: `Metrics` is currently being registered too late in the
router middleware chain, so it misses earlier middleware time. Update each
affected router setup to place `natsrouter.Metrics("<service>")` before the
existing `Logging` middleware and as early as possible after
`Recovery`/`RequestID`; in `user-presence-service/main.go`, move the metrics
registration out of the `natsrouter.Default(...)`-only setup by adding a
separate `router.Use(natsrouter.Metrics("user-presence-service"))` immediately
after the default middleware is installed. Use the existing `router.Use(...)`
chains in `room-service`, `room-worker`, `history-service`, `user-service`, and
`user-presence-service` as the anchors for the change.
In `@docs/superpowers/specs/2026-07-08-rpc-metrics-design.md`:
- Around line 53-55: Clarify the `/metrics` rollout wording in the design doc by
separating the Prometheus registry setup from the HTTP listener wiring; the
current text implies every service already exposes `/metrics`, which conflicts
with services that still need a listener. Update the section around the raw
Prometheus `promauto` guidance to explicitly say the default registry is already
used in existing components like search-service and pkg/atrest, while
listener/server wiring is still being added where missing.
🪄 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: 72812cb3-acb7-40a2-8a69-513bf548ce1a
📒 Files selected for processing (34)
auth-service/main.godocs/superpowers/plans/2026-07-08-rpc-metrics.mddocs/superpowers/specs/2026-07-08-rpc-metrics-design.mdgo.modhistory-service/cmd/main.gomedia-service/config.gomedia-service/main.gopkg/errcode/errhttp/write.gopkg/ginutil/metrics.gopkg/ginutil/metrics_test.gopkg/natsrouter/context.gopkg/natsrouter/context_test.gopkg/natsrouter/helpers_test.gopkg/natsrouter/metrics.gopkg/natsrouter/metrics_test.gopkg/natsrouter/params.gopkg/natsrouter/register.gopkg/natsrouter/router.gopkg/natsrouter/router_test.gopkg/otelutil/otel.gopkg/rpcmetrics/doc.gopkg/rpcmetrics/metrics.gopkg/rpcmetrics/metrics_test.goportal-service/main.goroom-service/main.goroom-worker/main.gosearch-service/handler.gosearch-service/main.gosearch-service/metrics.gosearch-service/metrics_test.goupload-service/main.gouser-presence-service/main.gouser-service/config/config.gouser-service/main.go
💤 Files with no reviewable changes (1)
- search-service/metrics_test.go
| func TestObserve(t *testing.T) { | ||
| Observe("svc-test", "chat.route.{id}.get", "ok", 12*time.Millisecond) | ||
|
|
||
| // Counter incremented for the exact label tuple (via the exported seam). | ||
| assert.Equal(t, float64(1), CounterValue("svc-test", "chat.route.{id}.get", "ok")) | ||
|
|
||
| // The duration histogram registered a series for this service/route pair. | ||
| count := testutil.CollectAndCount(requestDuration, "rpc_server_request_duration_seconds") | ||
| assert.GreaterOrEqual(t, count, 1) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use before/after pattern in TestObserve for counter idempotency.
CounterValue returns a cumulative value from the global Prometheus registry, which cannot be reset between test runs. The assertion assert.Equal(t, float64(1), ...) will fail with go test -count 2 or if another test happens to use the same label tuple. All other test files in this PR correctly use the before+1 == after pattern — this one should match.
💚 Proposed fix
func TestObserve(t *testing.T) {
+ before := CounterValue("svc-test", "chat.route.{id}.get", "ok")
Observe("svc-test", "chat.route.{id}.get", "ok", 12*time.Millisecond)
// Counter incremented for the exact label tuple (via the exported seam).
- assert.Equal(t, float64(1), CounterValue("svc-test", "chat.route.{id}.get", "ok"))
+ assert.Equal(t, before+1, CounterValue("svc-test", "chat.route.{id}.get", "ok"))
// The duration histogram registered a series for this service/route pair.
count := testutil.CollectAndCount(requestDuration, "rpc_server_request_duration_seconds")
assert.GreaterOrEqual(t, count, 1)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestObserve(t *testing.T) { | |
| Observe("svc-test", "chat.route.{id}.get", "ok", 12*time.Millisecond) | |
| // Counter incremented for the exact label tuple (via the exported seam). | |
| assert.Equal(t, float64(1), CounterValue("svc-test", "chat.route.{id}.get", "ok")) | |
| // The duration histogram registered a series for this service/route pair. | |
| count := testutil.CollectAndCount(requestDuration, "rpc_server_request_duration_seconds") | |
| assert.GreaterOrEqual(t, count, 1) | |
| } | |
| func TestObserve(t *testing.T) { | |
| before := CounterValue("svc-test", "chat.route.{id}.get", "ok") | |
| Observe("svc-test", "chat.route.{id}.get", "ok", 12*time.Millisecond) | |
| // Counter incremented for the exact label tuple (via the exported seam). | |
| assert.Equal(t, before+1, CounterValue("svc-test", "chat.route.{id}.get", "ok")) | |
| // The duration histogram registered a series for this service/route pair. | |
| count := testutil.CollectAndCount(requestDuration, "rpc_server_request_duration_seconds") | |
| assert.GreaterOrEqual(t, count, 1) | |
| } |
🤖 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 `@pkg/rpcmetrics/metrics_test.go` around lines 59 - 68, TestObserve currently
asserts a fixed CounterValue of 1, which is not idempotent because it reads from
the cumulative global Prometheus registry. Update the test to capture the
counter value before calling Observe, invoke Observe with the same label tuple,
then assert the after value equals before plus 1, matching the pattern used in
the other tests. Keep the existing checks around requestDuration and use the
TestObserve, Observe, and CounterValue symbols to locate the change.
…tency-errors-w8z4ba # Conflicts: # media-service/main.go
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
PR #480 Review: Unified RPC Metrics
Author: hmchangw
Stats: +2012 / -227 across 33 files
Verdict: ✅ APPROVE (with minor suggestions)
Summary
This PR introduces uniform rpc_server_requests_total and rpc_server_request_duration_seconds metrics across all synchronous RPC handlers (6 NATS services + 4 HTTP services). A shared pkg/rpcmetrics package owns the Prometheus collectors and status taxonomy, while thin middlewares in pkg/natsrouter and pkg/ginutil feed them identically.
What's Good
- Architecture: Centralized at the two transport seams (natsrouter, ginutil), not per-handler — clean, consistent, near-zero per-handler code.
- Cardinality safety:
routeis always a pattern/template ({name}placeholders orFullPath()), never a live subject/URL.statusis normalized through a sharedNormalizeStatususingerrcode.Code.Valid(). - Testing: Excellent TDD coverage — unit tests for
pkg/rpcmetrics(status taxonomy), unit + end-to-end tests fornatsrouter.Metrics(including real in-process NATS withRegister/RegisterVoid), and Gin middleware tests covering errcode status, HTTP fallback, normalization, and/healthz/unmatched skips. - No double-logging:
StatusLabelis a pureerrors.Asextractor;errhttp.Writestamps the classified Code on gin context so the metrics middleware reads it without re-classifying. - Graceful shutdown: All services wire
stopMetricsinto theirshutdown.Waitsequence. - search-service migration: Bespoke per-handler metrics correctly removed; ES-specific
search_service_es_duration_secondspreserved. - Refactoring:
replyResulthelper DRYs up the threeRegister*wrappers.
Suggestions (Non-Blocking)
1. Empty errcode.Code edge case
StatusLabel calls NormalizeStatus(string(ee.Code)) without checking if ee.Code is empty. If errcode.Code("").Valid() returns true, an empty string would pass through as a label. Verify this behavior or add a guard.
2. Missing 5xx test for Gin middleware
TestGinMetrics_HTTPClassFallback only tests 200 → "ok". Add a test case for 500 → "internal" to cover the 5xx branch of statusForHTTP.
3. HTTP 4xx fallback granularity
The HTTP fallback maps all 4xx responses to "bad_request". A 404 without an errcode would appear as "bad_request", not "not_found". Consider adding a special case for 404 → "not_found" or documenting this explicitly.
4. Superpowers docs in repo
docs/superpowers/plans/2026-07-08-rpc-metrics.md (1086 lines) and docs/superpowers/specs/2026-07-08-rpc-metrics-design.md (205 lines) are development process artifacts. The PR body notes they can be dropped — consider whether they belong in the main branch.
5. user-presence-service latency window
user-presence-service installs Metrics after natsrouter.Default()'s chain (RequestID/Recovery/Logging), so its latency excludes those middlewares — narrower than peers. The inline comment documents this, but operators should be aware if comparing latency across services.
Detailed Checklist
| Category | Status | Notes |
|---|---|---|
| Correctness | ✅ | Status taxonomy correct, stamping order correct, route = pattern |
| Security | ✅ | /metrics on separate port, no sensitive data in labels |
| Code Quality | ✅ | Clean package design, DRY, well-commented |
| Test Coverage | ✅ | 90%+ on new pkg/ code, end-to-end NATS tests |
| Performance | ✅ | Negligible overhead (sub-µs per request) |
| Documentation | ✅ | Package doc, design spec, clear PR description |
| Dependencies | ✅ | One indirect transitive dep (kylelemons/godebug) |
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
PR #480 Review — feat(rpc-metrics): unified RPC latency & error-rate metrics across all services
Verdict: REQUEST_CHANGES
Summary: Well-designed centralized metrics architecture with strong test coverage and solid cardinality guards. However, two services (history-service, room-worker) have metrics middleware installed but lack a /metrics HTTP endpoint, making their collected metrics invisible to Prometheus. This must be fixed before merge.
Critical Issues
- history-service/cmd/main.go (~L198): Missing
/metricsendpoint. Thenatsrouter.Metrics("history-service")middleware was added butotelutil.ServeMetrics,METRICS_ADDRconfig, andstopMetricsshutdown are absent. Metrics are collected into the default Prometheus registry but never exposed for scraping. - room-worker/main.go (~L205): Missing
/metricsendpoint. Same gap as history-service — middleware installed but noServeMetricswiring. Metrics are unreachable by Prometheus.
Medium Issues
- search-service/main.go (~L182):
/metricsexposure unclear. The search-service gets the Metrics middleware but no explicitServeMetricscall. The service previously had its ownmetricsHandler()— verify the existing/metricswiring still works and covers the newrpc_server_*series. - user-presence-service/main.go (~L135): Inconsistent latency window. Metrics is installed after
natsrouter.Default's built-in chain (RequestID/Recovery/Logging), producing a narrower latency window than peers where Metrics wraps the full chain. Cross-service latency comparison in dashboards will be misleading. - All service mains: Hardcoded service name strings. Service names are duplicated between
natsrouter.New(nc, "name")andMetrics("name"). A typo or rename in one but not the other would silently create a separate metrics label. Consider a shared constant or deriving from the router.
Low / Nice-to-Have
- pkg/natsrouter/metrics.go (~L13): Doc comment says "place it outermost (first) to capture the full chain" but actual installations place Metrics after Recovery+RequestID. Align the comment with practice.
- docs/superpowers/plans/2026-07-08-rpc-metrics.md (+1086 lines) and docs/superpowers/specs/2026-07-08-rpc-metrics-design.md (+205 lines): These are Claude Code internal planning artifacts. Remove before merge — they contain AI-agent instructions, not human-readable design docs.
- pkg/otelutil/otel.go:
ServeMetricshelper has no unit test. A simple integration test for port binding and graceful shutdown would improve confidence.
Positive Highlights
- Clean architecture: Shared
pkg/rpcmetricspackage owns collectors; two thin middlewares feed it. No circular deps, single responsibility per layer. - Forward-compatible status allowlist:
NormalizeStatususeserrcode.Code.Valid()instead of a hardcoded map — new canonical Codes auto-join the allowlist. - Strong test coverage: 68 lines of rpcmetrics tests (status taxonomy + Observe), 192 lines of natsrouter tests (unit + E2E with real NATS including Register/RegisterVoid/invalid-JSON paths), 93 lines of ginutil tests (errcode status, HTTP fallback, normalization, healthz/unmatched skips).
- Smart test seam:
CounterValueusesdto.Metricfromclient_model/goto avoid importing test-onlyprometheus/testutilinto production binaries. - Synchronous port binding:
ServeMetricscallsnet.Listenbefore spawning the goroutine, so port conflicts fail at startup rather than silently in the background. - Cardinality guarded:
routeis always a pattern/template, never a live subject/URL.statusbounded to 9 values. No unbounded label explosion possible. - Security:
/metricson separate port (:9090) from public API. No sensitive data in labels. - No double-logging:
errhttp.Writestamps the classified Code on gin context; the middleware reads it without re-classifying.
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
Review: APPROVE ✅
Excellent PR. Clean architecture, thorough tests, and consistent wiring across all 10 services. The shared pkg/rpcmetrics + thin middlewares approach is the right design.
What's great
- Cardinality guards are solid:
routeis always a pattern template (never live),statusis pinned to a 9-value allowlist viaNormalizeStatus()/errcode.Code.Valid(). Both transports funnel through the same funnel — no divergence. - Test coverage is impressive: unit tests (table-driven status taxonomy), chain-level tests (runChain), and real NATS integration tests with
require.Eventuallyfor race-free assertions. E2E covers success, errcode errors, invalid JSON, and RegisterVoid. - Clean refactor of search-service: ~110 lines of bespoke per-handler metrics deleted in favor of shared middleware. Only
search_service_es_duration_secondsretained. replyResult[Resp any]is a nice dedup acrossRegister/RegisterNoBody/RegisterVoid.otelutil.ServeMetricswith synchronous bind is the right pattern — port conflicts fail at startup, not silently in a goroutine.- No re-classification / no double-log:
errhttp.WritestampsErrCodeKey,ginutil.Metricsreads it without callingClassifyagain. NATS side useserrors.Asextraction. - Graceful shutdown wired correctly everywhere (
stopMetricsinshutdown.Wait).
Suggestions (non-blocking)
-
Drop
docs/superpowers/files before merge — these are design artifacts, not code. (Author already offered.) -
user-presence-serviceMetrics ordering: installed afternatsrouter.Default, so its latency window excludes Recovery/RequestID/Logging (peers measure a wider window). The comment acknowledges this, but users comparing latencies across services should be aware. Consider refactoringnatsrouter.Defaultto accept optional middleware args. -
errhttp.ErrCodeKey = "errcode"is very generic — could collide with otherc.Set("errcode", ...)calls. Consider"errhttp.code"for namespacing. -
ginutil.statusForHTTPswitch syntax nit:switch code := c.Writer.Status(); {— thecodevariable is declared but never used. Simplify toswitch { case c.Writer.Status() >= 500: .... -
ginutil.Metricshardcodes/healthzas a magic skip string. A configurable skip list would be more future-proof. -
Test helper naming:
runChaininmetrics_test.goandrunHandlerChaininhelpers_test.go(same package) have different signatures but similar names — slightly confusing.
Go-specific checks passed
- ✅ Context propagation (natsrouter.Context embeds context.Context, errhttp.Write receives standalone ctx)
- ✅ Error handling (errors.As, no swallowed errors, proper fmt.Errorf wrapping)
- ✅ Defer usage (no resource leaks, shutdown.Wait handles ordering)
- ✅ Structured logging (slog.Info/slog.Error with key=value pairs)
- ✅ Concurrency (route: set-once read-only; status: single-goroutine sequential; promauto: internally safe)
- ✅
make test -racegreen,make lintclean,gosecclean
Verdict
APPROVE. Merge after dropping the docs/superpowers/ files. The low-priority nits can be addressed in follow-ups.
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
Code Review: PR #480 — Unified RPC Metrics
VERDICT: APPROVE
This is a well-architected, well-tested PR. The shared pkg/rpcmetrics + transport-specific middlewares pattern is clean and the cardinality guards (route always a pattern, status pinned to 9 canonical codes via NormalizeStatus) are correctly implemented. Test coverage is excellent — unit tests for the core package, middleware unit tests, and end-to-end NATS integration tests that exercise the real Register/RegisterVoid paths. The search-service migration is clean and removes ~176 lines of duplicated metric code.
Non-blocking comments
-
user-presence-serviceMetrics placement:natsrouter.Default()installs Recovery/RequestID/Logging beforeMetrics, making latency measurements systematically narrower than other services. Consider standardizing the middleware order across all services in a follow-up. -
HTTP status fallback in
ginutil.statusForHTTP: All 4xx responses without an errcode stamp map to"bad_request"— a 404 or 403 is aggregated the same as a 400. This is safe (bounded cardinality) but coarser than it could be. Consider mapping specific HTTP status codes to their corresponding errcode codes (404→not_found, 403→forbidden, 401→unauthenticated) in a follow-up. -
CounterValuetest seam: Lives in production code (pkg/rpcmetrics/metrics.go) and returns 0 on error, which could silently mask a test bug if a label name is misspelled. Acceptable as-is given promauto guarantees registration.
What's solid
otelutil.ServeMetricsbinds synchronously before spawning serve goroutine — port conflicts are startup errors, not silentreplyResulthelper eliminates duplicate status-stamping inregister.goNormalizeStatususeserrcode.Code.Valid()(the authoritative check) instead of duplicating the allowlist- 4 end-to-end NATS tests protect against regressions in status stamping
/healthzand unmatched routes are correctly excluded from Gin metrics- All services properly wire
stopMetricsinto graceful shutdown - Error wrapping with
%w, slog structured logging, context propagation — all compliant with project standards
Summary: Ship it. The two non-blocking comments (user-presence-service middleware ordering, HTTP status fallback granularity) are worth tracking as follow-ups but don't block this PR. No security concerns, no incorrect behavior, no test gaps.
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
Review: APPROVE ✅
This is a clean, well-architected PR that introduces uniform RPC latency and error-rate metrics across all synchronous services. The design is sound, tests are thorough, and edge cases are handled correctly.
What works well
- Architecture: Shared
pkg/rpcmetricsowns collectors + taxonomy; thin middlewares at the two transport seams (natsrouter,ginutil). No duplication, no per-handler code. - Cardinality guard:
routeis always a template/pattern (natsrouter pattern with{name}placeholders, or GinFullPath()), never a live subject/URL.statusis pinned to a 9-value allowlist viaerrcode.Code.Valid(). - Status taxonomy is consistent across transports: Both NATS (
StatusLabel) and HTTP (statusForHTTP) funnel throughNormalizeStatus. - Correct status stamping:
replyResulthelper inregister.gostamps status before reply on all paths.RegisterVoidnow stamps status on errors too (previously only logged). Unmarshal failures getbad_request. - Graceful shutdown:
stopMetricsplaced after router shutdown but beforenc.Drain— final scrape window preserved.ServeMetricsbinds synchronously (port conflicts fail at startup). - Test coverage is excellent: Three layers — unit tests for
rpcmetricstaxonomy, chain tests for each middleware, and end-to-end NATS tests (Register,RegisterVoid, invalid JSON, errcode errors). Gin tests cover errcode, HTTP fallback, non-canonical normalization, and/healthz/unmatched skip. - search-service migration: Bespoke per-handler metrics removed cleanly;
search_service_es_duration_secondsretained as a service-specific histogram.
Minor observations (non-blocking)
-
user-presence-service latency window: Metrics is installed after
natsrouter.Default's chain (Recovery/RequestID/Logging), so its latency series excludes those middlewares — marginally narrower than peers. The PR documents this with a comment. Consider aligning for uniformity. -
HTTP status fallback granularity:
statusForHTTPmaps all 4xx tobad_request. Handlers that bypasserrhttp.Writelose the distinction between 401/403/404/409/429. Theerrhttp.Writepath works correctly. Consider mapping specific HTTP status codes to their errcode equivalents in a follow-up. -
Plan document size:
docs/superpowers/plans/2026-07-08-rpc-metrics.mdis 1086 lines of development instructions. Consider whether to keep it in the repo or drop it — the design spec alone is sufficient. -
Dependency:
kylelemons/godebugadded as indirect (pulled throughprometheus/client_model→ protobuf). Harmless, but confirmgo mod tidywas run.
Verdict
APPROVE — No correctness issues, no security concerns. The PR delivers exactly what it promises with strong test coverage and clear documentation. The observations above are suggestions for future refinement, not blockers.
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
APPROVE
Summary
This is a well-architected, thoroughly tested, and cleanly implemented PR that adds uniform RPC latency and error-rate Prometheus metrics across all 10 synchronous RPC services (6 NATS + 4 HTTP). The centralized approach in the two router seams (pkg/natsrouter and pkg/ginutil) is the correct design — minimal per-handler code, consistent metric emission.
What I reviewed
pkg/rpcmetrics(new): Collectors,StatusLabel,NormalizeStatus,Observe,CounterValuetest seam. Clean status taxonomy with centralized allowlist viaerrcode.Code.Valid()— an improvement over the old search-service's manualallowedStatusLabelsmap.pkg/natsrouter:route/statusfields onContext,SetStatus/Status/Routeaccessors,Metrics(service)middleware,replyResulthelper in register.go, e2e tests over in-process NATS.pkg/ginutil:Metrics(service)middleware withFullPath()-based route label,/healthzand unmatched skip, HTTP-class fallback when no errcode Code.pkg/errcode/errhttp:ErrCodeKeyconstant +c.Set(...)inWrite— avoids double-classification.pkg/otelutil:ServeMetrics(addr)helper with synchronous bind for early port-conflict detection.- Service wiring: All 10 services, consistent
METRICS_ADDRconfig + middleware + graceful shutdown. search-servicemigration: Clean removal of ~170 lines of bespoke request metrics, preserving onlysearch_service_es_duration_seconds.
Strengths
- Cardinality guards:
routeis always a pattern/template (never a live subject/URL);statusis pinned to 9 values viaNormalizeStatus. Both transports funnel through the same guard. - No double-logging:
StatusLabelis a pureerrors.Asextractor, not a classifier — safe to call on the reply path alongsideerrcode.Classify. - Test coverage: TDD throughout. Unit tests for
rpcmetrics(status taxonomy, observe),natsrouter(chain-level + 4 e2e tests over real NATS server exercising actualRegister*wrappers),ginutil(errcode status, HTTP fallback,/healthz/unmatched skip, non-canonical normalization). All pass with-race. - Consistent wiring: Every service follows the same pattern — config field, middleware,
ServeMetrics, shutdown hook. - Clean migration:
search-servicesheds its bespoke metrics without losing observability; the ES-specific histogram is preserved.
Suggestions (non-blocking)
-
user-presence-serviceMetrics position: Installed afternatsrouter.Default()'s chain, so its latency window excludes Default'sRequestID/Recovery/Logging. Other services install Metrics before Logging. Consider migrating tonatsrouter.New()+ explicit ordering for consistency. The comment in the diff acknowledges this; status stamping is unaffected. -
upload-servicemiddleware: Uses its ownrequestIDMiddleware()/accessLogMiddleware()instead ofginutilequivalents. This is a pre-existing inconsistency; addingginutil.Metricsis a step in the right direction. -
HTTP 4xx fallback granularity:
statusForHTTPmaps all 4xx responses without an errcode Code to"bad_request", meaning a plain404shows as"bad_request"not"not_found". This is by design — without an errcode Code we can't reliably distinguish subtypes. Handlers should useerrhttp.Writefor precise status. Documented behavior, no action needed. -
CounterValueimportsclient_model: The test seam inmetrics.goimportsprometheus/client_model/goto avoid linkingprometheus/testutilinto production. This is a reasonable trade-off;client_modelis already a transitive dependency.
Testing
make test(full,-race): greenmake lint: 0 issuesgosec: clean- New tests:
pkg/rpcmetrics(3 test funcs, 16+ cases),pkg/natsrouter(7 test funcs including 4 e2e NATS),pkg/ginutil(4 test funcs)
Verdict
APPROVE — Ready to merge. No correctness or security issues found. All concerns are minor and can be addressed in follow-ups.
julianshen
left a comment
julianshen
left a comment
There was a problem hiding this comment.
Hermes Agent Code Review
This PR has been reviewed by Hermes Agent. See the review above for detailed findings.
Automated review bot — do not reply to this message.
3 participants
Summary
Adds uniform request latency and error-rate metrics to every synchronous RPC handler across the system, so a single set of Grafana panels / alert rules works for any service via a
servicelabel.Two new time series, emitted identically by both transports:
rpc_server_requests_total{service, route, status}(counter)rpc_server_request_duration_seconds{service, route}(histogram,DefBuckets)Design decisions:
pkg/natsrouter, one inpkg/ginutil, so coverage is near-zero-per-handler and consistent.routeis always a low-cardinality template — thenatsrouterpattern with{name}placeholders, or Ginc.FullPath()— never a live subject/URL. This is the cardinality guard.statusis the errcodeCode, pinned to a 9-value allowlist (ok+ 8 canonical codes); anything else collapses tointernal, enforced identically on both transports via a sharedNormalizeStatus.promautoon the default registry (matches the existing metrics pattern; OTel stays for tracing).Changes
New shared package
pkg/rpcmetrics— collectors +Observe+StatusLabel/NormalizeStatus(reuseserrcode.Code.Valid()).pkg/natsrouter— newMetrics(service)middleware;Contextcarries the matched route pattern and terminal status (stamped by theRegister*wrappers via a sharedreplyResulthelper).pkg/ginutil+pkg/errcode/errhttp— newMetrics(service)middleware (route=FullPath,/healthzand unmatched routes skipped);errhttp.Writestamps the classified Code onto the gin context for the middleware to read (no re-classify, no double-log).pkg/otelutil— newServeMetrics(addr)helper that binds and serves/metricsin the background and returns a shutdown func.Wiring —
Metricsinstalled in all 6 NATS request/reply services (room-service, room-worker, user-service, user-presence-service, history-service, search-service) and all 4 Gin HTTP services (auth-service, portal-service, media-service, upload-service). A/metricslistener was added to the 7 services that lacked one, via the newServeMetricshelper.search-service— migrated off its bespoke per-handler request metrics onto the shared middleware; kept only its service-specificsearch_service_es_duration_seconds.Testing
pkg/rpcmetrics(status taxonomy),pkg/natsrouter.Metrics(incl. end-to-end coverage of theRegister*status stamping over an in-process NATS server), andpkg/ginutil.Metrics(errcode status, HTTP-class fallback,/healthz/unmatched skips, non-canonical-code normalization).make test(full,-race) green;make lint0 issues;gosecclean.docs/client-api.mdchange — this is server-side observability only (no request/response schema or event struct touched).Notes
docs/superpowers/design + implementation-plan docs are included for reviewer context; happy to drop them if you'd prefer the PR carry code only.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
/metricsendpoint with configurableMETRICS_ADDR(default:9090) across multiple services.Bug Fixes