Conversation
|
Preview (prod backend + PR dashboard) → https://1147.ns-preview.trapti.tech/ |
There was a problem hiding this comment.
Pull request overview
This pull request migrates the logging system from logrus to slog (structured logging) to enable adding trace ID and user ID to all logs through context propagation. The PR also introduces OpenTelemetry trace ID generation (without trace exporting) and adds a sloglint linter to enforce proper slog usage.
Changes:
- Migrated all logging calls from logrus to slog with structured key-value pairs
- Implemented custom slog handler to automatically inject trace_id and user_id from context
- Added OpenTelemetry tracer provider for trace ID generation
- Updated CLI flags and initialization to support slog log levels
- Added sloglint linter configuration
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/slogutil/keys.go | Defines context keys for trace_id and user_id |
| pkg/util/slogutil/init.go | Provides logger initialization with custom handler |
| pkg/util/slogutil/handler.go | Custom handler that extracts trace_id and user_id from context |
| pkg/infrastructure/observability/trace.go | Initializes OpenTelemetry tracer for ID generation |
| pkg/infrastructure/grpc/log_interceptor.go | Adds trace_id and user_id to context for gRPC requests |
| pkg/util/cli/flag.go | Updates log level flag handling for slog |
| cmd/main.go | Initializes logger and tracer provider at startup |
| cmd/providers.go | Adds OpenTelemetry interceptor to gRPC server |
| .golangci.yml | Adds sloglint linter configuration |
| go.mod, go.sum | Updates dependencies for OpenTelemetry and removes logrus |
| (multiple files) | Converts log.Info/Error/etc to slog.InfoContext/ErrorContext/etc with structured attributes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err := s.cluster.Start(ctx) | ||
| if err != nil { | ||
| log.Errorf("failed to start cluster: %+v", err) | ||
| slog.Error("failed to start cluster", "error", err) |
There was a problem hiding this comment.
Context is not being passed to the slog call, but ctx is available in the closure scope. This should use ErrorContext(ctx, ...) instead to properly include trace_id and user_id in the logs.
| err := client.StreamBuildLog(context.Background(), buildID, send) | ||
| if err != nil { | ||
| log.Errorf("sending build log: %+v", err) | ||
| slog.Error("failed to send build log", "error", err) |
There was a problem hiding this comment.
Context is not being passed to the slog call, but context.Background() is being used in this goroutine. Consider passing the context as a parameter to enable proper trace_id and user_id logging. If context.Background() is intentional, the current usage is acceptable but loses tracing information.
| slog.Error("failed to detect build crash", "error", err) | ||
| } | ||
| log.Debugf("Build crash detection complete in %v", time.Since(start)) | ||
| slog.Debug("Build crash detection complete", "duration", time.Since(start)) |
There was a problem hiding this comment.
Context is not being passed to the slog calls, but ctx is available in the closure scope. These should use ErrorContext(ctx, ...) and DebugContext(ctx, ...) instead to properly include trace_id and user_id in the logs.
pirosiki197
force-pushed
the
feat/structure-logging
branch
from
df85058 to
60e2677
Compare
pirosiki197
force-pushed
the
feat/structure-logging
branch
from
4131911 to
38d4590
Compare
なぜやるか
ログにtrace idやuser idを付けたい。
そのためにはctxに値を含めて引き回す必要があり、slogの方がそのあたりに対応しやすい。また、構造化ログににしておくと検索などで扱いやすい。
やったこと
やらなかったこと
trace idはつけたが、traceのexportはしない
資料