feat(log): added support for contextual logging - #152
Conversation
Signed-off-by: Mohamed Tahar KEDJOUR <mkedjour@cisco.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the error handling and logging infrastructure by moving the log package from pkg/log to internal/pkg/log and removing the custom errutil.Err() wrapper in favor of standard Go error handling with errors.New() and fmt.Errorf().
- Migrated logging package from public to internal with enhanced context-aware logging capabilities
- Replaced custom error wrapper
errutil.Err()with standard Go error handling patterns - Added contextual logger interceptor for gRPC to enrich logs with request metadata
Reviewed Changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/log/log.go | Removed old public log package |
| internal/pkg/log/log.go | New internal log package with context enrichment support |
| internal/pkg/db/logger.go | New logrus logger adapter for GORM database operations |
| internal/pkg/db/context.go | Renamed type from context to dbContext and integrated new logger |
| internal/pkg/shutdown.go | New utility function for graceful shutdown with context |
| internal/pkg/errutil/error.go | Removed Err() function, keeping only ErrInfo() |
| pkg/oidc/provider.go | Updated to use standard error wrapping and internal log package |
| pkg/oidc/parser.go | Replaced errutil.Err() with errors.New() and fmt.Errorf() |
| internal/node/grpc/interceptors/contextual_logger.go | New interceptor to enrich context with gRPC metadata for logging |
| internal/node/grpc/interceptors/contextual_logger_test.go | Test coverage for contextual logger interceptor |
| internal/node/grpc/interceptors/errors.go | Updated to use context-aware logging |
| internal/node/vc_service.go | Replaced direct log calls with context-aware log.FromContext(ctx) |
| internal/node/id_service.go | Updated logging to use context-aware approach |
| internal/node/id_generator.go | Updated logging to use context-aware approach |
| internal/node/grpc/issuer.go | Updated logging to use context-aware approach |
| internal/core/issuer/verification/service.go | Updated to use context-aware logging and standard error handling |
| internal/core/vc/postgres/repository.go | Replaced errutil.Err() with fmt.Errorf() |
| internal/core/issuer/postgres/repository.go | Replaced errutil.Err() with fmt.Errorf() |
| internal/core/id/postgres/repository.go | Replaced errutil.Err() with fmt.Errorf() |
| internal/issuer/badge/service.go | Replaced custom error wrapper with standard Go error handling |
| internal/issuer/badge/mcp/discover.go | Updated error handling and fixed typo in comment |
| internal/pkg/httputil/data.go | Updated import path for log package |
| internal/pkg/grpcserver/server.go | Updated import path for pkg package |
| internal/pkg/convertutil/common.go | Updated import path for log package |
| internal/pkg/cmd/config.go | Updated to use log.WithError() instead of logrus.Fields |
| internal/pkg/cache/cache.go | Updated import path for log package |
| internal/issuer/vault/data/repository.go | Removed unnecessary import alias |
| internal/issuer/vault/data/filesystem/repository.go | Removed unnecessary import alias |
| cmd/node/main.go | Updated imports, improved variable naming, and added contextual logger interceptor |
| cmd/node/configuration.go | Added IsDev() helper method |
| cmd/issuer/commands/issuer/register.go | Fixed error message (removed invalid error wrapping) |
| cmd/issuer/commands/badge/issue/*.go | Removed unnecessary import aliases |
| .gitignore | Added /.run/ directory to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err, | ||
| "failed to discover mcp server", | ||
| ) | ||
| return nil, fmt.Errorf("failed to discover mcp tools: %w", err) |
There was a problem hiding this comment.
Inconsistent capitalization in error messages. Line 58 uses "failed to discover mcp tools" while line 66 uses "failed to discover MCP resources". Both should use consistent capitalization for "MCP" (uppercase acronym).
| return nil, fmt.Errorf("failed to discover mcp tools: %w", err) | |
| return nil, fmt.Errorf("failed to discover MCP tools: %w", err) |
| defer func(mcpClient *client.Client) { | ||
| _ = mcpClient.Close() | ||
| }(mcpClient) |
There was a problem hiding this comment.
The defer function unnecessarily captures mcpClient as a parameter when it's already in scope. The simpler form defer mcpClient.Close() would work, or if error handling is needed, use defer func() { _ = mcpClient.Close() }() without the parameter.
| defer func(mcpClient *client.Client) { | |
| _ = mcpClient.Close() | |
| }(mcpClient) | |
| defer mcpClient.Close() |
| // Info : Configure log level | ||
| func Info(args ...any) { | ||
| logrus.Info(args...) | ||
| } | ||
|
|
||
| // Debug : Configure log level | ||
| func Debug(args ...any) { | ||
| logrus.Debug(args...) | ||
| } | ||
|
|
||
| // Fatal : Configure log level | ||
| func Fatal(args ...any) { | ||
| logrus.Fatal(args...) | ||
| } | ||
|
|
||
| // Error : Configure log level | ||
| func Error(args ...any) { | ||
| logrus.Error(args...) | ||
| } | ||
|
|
||
| func Warn(args ...any) { | ||
| logrus.Warn(args...) |
There was a problem hiding this comment.
The function comments "Configure log level" are incorrect for these logging functions. They should describe what each function does (e.g., "Info logs an informational message", "Debug logs a debug message", etc.)
| } | ||
|
|
||
| func (l *logrusLogger) Warn(ctx context.Context, msg string, data ...any) { | ||
| log.FromContext(ctx).Infof(msg, data...) |
There was a problem hiding this comment.
The Warn method is calling log.FromContext(ctx).Infof() instead of log.FromContext(ctx).Warnf(). This should log at the warning level, not info level.
| log.FromContext(ctx).Infof(msg, data...) | |
| log.FromContext(ctx).Warnf(msg, data...) |
| nCtx := ctx | ||
|
|
||
| if stored, ok := ctx.Value(contextLogFieldsKey{}).(logrus.Fields); ok { | ||
| maps.Copy(stored, fields) |
There was a problem hiding this comment.
The maps.Copy function copies from the second argument to the first argument, so this should be maps.Copy(fields, stored) to preserve existing context fields and add new ones. Currently, this overwrites the new fields with the old ones, which is the opposite of the intended behavior.
Signed-off-by: Mohamed Tahar KEDJOUR <mkedjour@cisco.com>
Description
errutil.Err()and replaced with withfmt.Errorf()pkgtointernal/pkg(log,db,grpc, ...)Type of Change
Checklist