Conversation
WalkthroughIntroduces HashiCorp hclog-based logging and recovery middleware into the router-plugin, adds logger options to plugin configuration, chains interceptors in gRPC setup, and integrates logging in the projects subgraph service. Updates go.mod files, adds a local replace for router-plugin, and adjusts one service method signature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Noroth
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
router-plugin/middleware/logging.go (1)
12-18: Consider adding error handling and documentation improvements.The logging interceptor implementation is functional but could be enhanced:
- The interceptor doesn't handle cases where
hclog.WithContextmight fail- The trace logging could include request duration for better observability
- Consider logging at info level for method invocations rather than trace
Here's an enhanced version with better error handling and observability:
-func Logging(logger hclog.Logger) grpc.UnaryServerInterceptor { - return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { - logger.Trace("LoggingInterceptor", "method", info.FullMethod) - ctx = hclog.WithContext(ctx, logger) - return handler(ctx, req) - } -} +func Logging(logger hclog.Logger) grpc.UnaryServerInterceptor { + return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + start := time.Now() + logger.Info("gRPC method invoked", "method", info.FullMethod) + + ctx = hclog.WithContext(ctx, logger) + resp, err := handler(ctx, req) + + duration := time.Since(start) + if err != nil { + logger.Error("gRPC method failed", "method", info.FullMethod, "duration", duration, "error", err) + } else { + logger.Info("gRPC method completed", "method", info.FullMethod, "duration", duration) + } + + return resp, err + } +}You'll also need to add the time import:
import ( "context" + "time" "github.com/hashicorp/go-hclog" "google.golang.org/grpc" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
demo/pkg/subgraphs/projects/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
demo/pkg/subgraphs/projects/go.mod(1 hunks)demo/pkg/subgraphs/projects/src/main.go(1 hunks)demo/pkg/subgraphs/projects/src/service/service.go(23 hunks)router-plugin/middleware/logging.go(1 hunks)router-plugin/middleware/recovery.go(1 hunks)router-plugin/plugin.go(3 hunks)router-plugin/setup/init.go(3 hunks)router-tests/go.mod(1 hunks)router/pkg/grpcconnector/grpc_plugin_client.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
demo/pkg/subgraphs/projects/src/main.go (3)
demo/pkg/subgraphs/projects/generated/service_grpc.pb.go (1)
ProjectsService_ServiceDesc(906-1009)demo/pkg/subgraphs/projects/src/service/service.go (1)
ProjectsService(35-39)router-plugin/plugin.go (2)
NewRouterPlugin(125-173)WithJSONLogger(108-117)
router-plugin/plugin.go (2)
router-plugin/setup/init.go (2)
GrpcServer(26-75)GrpcServerInitOpts(20-24)router-plugin/config/settings.go (1)
StartupConfig(32-35)
demo/pkg/subgraphs/projects/src/service/service.go (1)
demo/pkg/subgraphs/projects/generated/service.pb.go (12)
Milestone(3698-3714)Milestone(3729-3729)Milestone(3744-3746)Task(3825-3847)Task(3862-3862)Task(3877-3879)QueryProjectStatusesRequest(1913-1917)QueryProjectStatusesRequest(1932-1932)QueryProjectStatusesRequest(1947-1949)QueryProjectStatusesResponse(1952-1958)QueryProjectStatusesResponse(1973-1973)QueryProjectStatusesResponse(1988-1990)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration_test (./events)
🔇 Additional comments (11)
router-tests/go.mod (1)
29-29: Version update looks good.The router-plugin dependency has been updated to a newer commit (v0.0.0-20250808194725-de123ba1c65e) which includes the new logging and recovery middleware functionality. This aligns with the PR's objective to improve plugin handling.
router/pkg/grpcconnector/grpc_plugin_client.go (1)
10-12: Import formatting improvement.The import reorganization enhances readability by grouping related OpenTelemetry and gRPC packages together with proper spacing. This aligns with Go's import grouping conventions.
demo/pkg/subgraphs/projects/go.mod (3)
6-6: Good: Direct dependency promotion for logging.Moving
go-hclogfrom indirect to direct dependencies is appropriate since the demo project now directly uses logging functionality from the updated router-plugin.
8-8: Version update aligns with enhanced plugin functionality.The router-plugin version update (v0.0.0-20250808194725-de123ba1c65e) is consistent with the new logging and recovery middleware features being introduced.
47-47: Local replace directive is appropriate for development.The local replace directive enables development against the local router-plugin copy, which is useful during active development. Ensure this is documented in the development setup instructions.
demo/pkg/subgraphs/projects/src/service/service.go (1)
618-620: Consistent signature: parameter renamed to underscoreThe function signature correctly uses
_for the unused request parameter andctxas the first parameter, maintaining consistency with Go conventions for unused parameters.demo/pkg/subgraphs/projects/src/main.go (1)
15-23: Clean refactoring to named function and JSON loggingThe code properly extracts the service registration logic into a named function and configures JSON logging with Info level, improving readability and observability.
router-plugin/setup/init.go (2)
29-37: Proper interceptor chaining with logging and recoveryThe implementation correctly ensures the logger is injected into the context before the recovery interceptor, enabling proper panic logging. The interceptors are collected in a slice with appropriate initial capacity.
64-67: Consistent interceptor managementThe tracing interceptor is now properly appended to the interceptors slice and chained together, maintaining consistency with the logging and recovery interceptors.
router-plugin/plugin.go (2)
107-117: JSON logger properly configuredThe JSON logger implementation with timestamps enabled is appropriate for production use and structured logging.
154-160: Good default logger configurationSetting a default JSON logger with Trace level ensures logging is always available, which is essential for debugging and observability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
demo/pkg/subgraphs/projects/src/service/service.go (5)
111-115: LGTM! Empty keys validation fixed.This addresses the previous review comment about potential null pointer dereference. The early return with an empty result slice is appropriate for lookup operations when no keys are provided.
146-150: LGTM! Empty keys validation fixed.This addresses the previous review comment about potential null pointer dereference for
LookupTaskById.
181-186: LGTM! Empty keys validation fixed.This addresses the previous review comment about potential null pointer dereference for
LookupProductByUpc.
522-525: LGTM! Empty keys validation fixed.This addresses the previous review comment about potential null pointer dereference for
LookupEmployeeById.
561-564: LGTM! Empty keys validation fixed.This addresses the previous review comment about potential null pointer dereference for
LookupProjectById.
🧹 Nitpick comments (2)
router-plugin/middleware/recovery.go (2)
12-14: Doc comment is misleading; the RPC is aborted with an Internal error.The request is not “continued”; it returns a gRPC Internal error while the server stays up. Recommend updating the comment for accuracy.
-// Recovery is a middleware that recovers from panics and logs the error. -// It is used to ensure that the panic is logged and the request is not aborted. +// Recovery is a middleware that recovers from panics in unary handlers. +// It logs the panic and ensures the server stays up while the RPC returns codes.Internal.
14-24: Add a focused unit test for panic recovery behavior.A simple test that asserts codes.Internal and no crash will prevent regressions.
Example:
package middleware_test import ( "context" "testing" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "router-plugin/middleware" ) func TestRecoveryReturnsInternalOnPanic(t *testing.T) { ctx := context.Background() info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Call"} handler := func(ctx context.Context, req interface{}) (interface{}, error) { panic("boom") } resp, err := middleware.Recovery(ctx, nil, info, handler) if resp != nil { t.Fatalf("expected nil response on panic, got %#v", resp) } if status.Code(err) != codes.Internal { t.Fatalf("expected codes.Internal, got %v (err=%v)", status.Code(err), err) } }I can add this test (and a complementary “no panic” test) if you’d like me to push a commit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
demo/pkg/subgraphs/projects/src/service/service.go(23 hunks)router-plugin/middleware/recovery.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
demo/pkg/subgraphs/projects/src/service/service.go (1)
demo/pkg/subgraphs/projects/generated/service.pb.go (36)
LookupMilestoneByIdResponse(1189-1208)LookupMilestoneByIdResponse(1223-1223)LookupMilestoneByIdResponse(1238-1240)Milestone(3698-3714)Milestone(3729-3729)Milestone(3744-3746)LookupTaskByIdResponse(1349-1368)LookupTaskByIdResponse(1383-1383)LookupTaskByIdResponse(1398-1400)Task(3825-3847)Task(3862-3862)Task(3877-3879)LookupProductByUpcResponse(1669-1688)LookupProductByUpcResponse(1703-1703)LookupProductByUpcResponse(1718-1720)Product(4095-4103)Product(4118-4118)Product(4133-4135)LookupEmployeeByIdResponse(1509-1528)LookupEmployeeByIdResponse(1543-1543)LookupEmployeeByIdResponse(1558-1560)Employee(4000-4012)Employee(4027-4027)Employee(4042-4044)LookupProjectByIdResponse(1029-1048)LookupProjectByIdResponse(1063-1063)LookupProjectByIdResponse(1078-1080)Project(3507-3531)Project(3546-3546)Project(3561-3563)QueryProjectStatusesRequest(1913-1917)QueryProjectStatusesRequest(1932-1932)QueryProjectStatusesRequest(1947-1949)QueryProjectStatusesResponse(1952-1958)QueryProjectStatusesResponse(1973-1973)QueryProjectStatusesResponse(1988-1990)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
router-plugin/middleware/recovery.go (3)
14-21: Resolved: Recovery now returns proper gRPC error on panic.Good fix. The interceptor now recovers, logs, and returns codes.Internal instead of silently succeeding or crashing the process.
Also applies to: 23-24
14-24: Ensure Recovery is the outermost interceptor in the chain.For complete coverage, Recovery should wrap all subsequent interceptors and the handler. In gRPC-Go, interceptors are invoked in the order provided; the first wraps the rest. Verify it’s added first in the chain.
15-21: Ensure safe panic recovery and include a stack tracePlease update router-plugin/middleware/recovery.go to guard against a nil logger and log a stack trace:
• Import the debug package
import ( "context" "github.com/hashicorp/go-hclog" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "runtime/debug" )• Change the deferred recovery block as follows
@@ defer func() { - if r := recover(); r != nil { - hclog.FromContext(ctx).Error("panic", "error", r) + if r := recover(); r != nil { + logger := hclog.FromContext(ctx) + if logger == nil { + // Fallback to a new logger to avoid a nested panic + logger = hclog.New(&hclog.LoggerOptions{Name: "grpc-recovery"}) + } + // Include the full stack for better debuggability + logger.Error("panic recovered", "error", r, "stacktrace", string(debug.Stack())) resp = nil err = status.Errorf(codes.Internal, "internal server error") } }()Please verify whether
hclog.FromContext(ctx)is guaranteed to return a non-nil logger in every gRPC invocation. If it always does (for example, enforced by a global logging interceptor), the nil check may be unnecessary; otherwise, keeping this guard is required to prevent a nested panic.demo/pkg/subgraphs/projects/src/service/service.go (13)
11-11: LGTM! Good logging integration.The addition of
github.com/hashicorp/go-hclogaligns with the PR's objective to improve plugin handling with structured logging capabilities.
117-117: LGTM! Context-aware logging implemented.The structured logging with milestone ID provides good observability for debugging and monitoring.
152-152: LGTM! Consistent logging pattern.The logging follows the same structured pattern as other lookup methods.
188-188: LGTM! Consistent UPC logging.The logging correctly uses the UPC field for product lookups.
215-216: LGTM! Comprehensive mutation logging.The logging captures both the operation and the relevant project ID for tracking milestone additions.
252-253: LGTM! Task creation logging.Consistent logging pattern for task mutations with project ID tracking.
287-288: LGTM! Status update logging with context.The logging captures both project ID and the new status, providing good audit trail for status changes.
337-338: LGTM! Comprehensive query operation logging.All query methods now have consistent structured logging with relevant parameters (project_id, query terms, etc.). This provides excellent observability for debugging and monitoring.
Also applies to: 352-353, 367-368, 404-405, 461-462
527-527: LGTM! Employee lookup logging.Consistent logging pattern with employee ID tracking.
566-566: LGTM! Project lookup logging.Consistent logging pattern with project ID tracking.
593-594: LGTM! Project creation logging.Simple but effective logging for project mutation operations.
630-631: LGTM! Complete query logging coverage.All remaining query methods now have consistent structured logging. The logging pattern is well-established and provides good observability across all service operations.
Also applies to: 647-648, 671-672, 688-689, 707-708, 719-720, 731-732, 777-778
646-646: Function signature change looks correct.The
QueryProjectStatusesmethod now properly uses thectxparameter for logging, which aligns with the consistent pattern across all other methods in this service.
StarpTech
left a comment
StarpTech
left a comment
There was a problem hiding this comment.
As discussed.
- Plugin stack trace should be visible in the router logs
- Try to expose a Zap logger to unify logging with the router
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
router-plugin/plugin.go (2)
66-71: Service name option looks good.
No functional issues. Consider clarifying in the comment that this influences tracing identifiers and possibly logs.Apply:
-// WithServiceName sets the service name for the plugin. +// WithServiceName sets the service name used in tracing (and related metadata).
101-108: Document expectations for custom loggers (JSON + DisableTime).
Users supplying a custom logger must ensure it’s JSON-formatted and has timestamps disabled for router ingestion.Apply:
-// WithCustomLogger sets the logger to the provided logger. -// This is useful for when you want to use a custom logger. -// For example, when you want to use a custom logger for the plugin. +// WithCustomLogger sets the logger to the provided logger. +// NOTE: The router ingests plugin stdout and adds timestamps itself. Custom loggers +// should be JSON-formatted and set DisableTime: true to avoid duplicate timestamps. +// This is useful when integrating a preconfigured hclog logger with the plugin. func WithCustomLogger(logger hclog.Logger) PluginOption { return func(c *RouterPlugin) { c.serveConfig.Logger = logger } }I can add a lightweight runtime guard/log warning if a non-JSON or time-enabled logger is detected (best-effort via a marker field) if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
demo/pkg/subgraphs/projects/src/main.go(1 hunks)router-plugin/plugin.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T09:13:38.935Z
Learnt from: Noroth
PR: wundergraph/cosmo#2132
File: router-plugin/plugin.go:88-104
Timestamp: 2025-08-12T09:13:38.935Z
Learning: In the Cosmo router plugin system, plugin logs are written to stdout and incorporated by the router into its zap logger, which handles timestamping. Therefore, plugin loggers should use DisableTime: true to avoid redundant timestamps that could interfere with the router's log processing.
Applied to files:
router-plugin/plugin.go
🧬 Code Graph Analysis (2)
demo/pkg/subgraphs/projects/src/main.go (2)
demo/pkg/subgraphs/projects/src/service/service.go (1)
ProjectsService(35-39)router-plugin/plugin.go (2)
NewRouterPlugin(110-158)WithLogger(89-99)
router-plugin/plugin.go (3)
router/internal/expr/expr.go (1)
Trace(96-98)router-plugin/setup/init.go (2)
GrpcServer(26-75)GrpcServerInitOpts(20-24)router-plugin/config/settings.go (1)
StartupConfig(32-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
demo/pkg/subgraphs/projects/src/main.go (1)
6-6: Import is correct and required for the new logger option.
No issues; the hclog import is needed for WithLogger.router-plugin/plugin.go (5)
7-8: New import for os is appropriate.
Used for startup config env var; no issues.
12-12: hclog import is correct.
Required for new logger options and wiring; no issues.
73-78: Tracing error handler option is sound.
This provides useful customization for tracing pipelines.
80-85: Service version option looks good.
No issues; aligns with tracing setup usage.
150-150: Passing the resolved logger into gRPC setup is correct.
This ensures middleware receives the intended logger.
…733-grpc-plugin-provide-panic-interceptor
Noroth
deleted the
ludwig/eng-7733-grpc-plugin-provide-panic-interceptor
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Checklist