Conversation
📝 WalkthroughWalkthroughThis pull request implements per-component log level configuration by introducing a logging factory pattern. A new Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cce50f9 to
33e99fc
Compare
2e388a3 to
60e28d1
Compare
33e99fc to
8c5edbb
Compare
60e28d1 to
4de1dc1
Compare
|
Worked for me when tested, just had to add a missing comma here compared to the PR description.
|
8c5edbb to
62341c7
Compare
Adds the ability to configure per-component log levels to make it easier to see, for example, debug logs for a single component. PLAT-417
4de1dc1 to
d902a69
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/internal/config/config.go (1)
42-48: Consider validatingComponentLevelsentries inLogging.validate().The
validate()method checks thatLevelis a valid zerolog level, butComponentLevelsentries are not validated here. WhileNewFactorydoes catch invalid levels at construction time, adding validation here would surface configuration errors earlier duringConfig.Validate().♻️ Proposed enhancement
func (l Logging) validate() []error { var errs []error if _, err := zerolog.ParseLevel(l.Level); err != nil { errs = append(errs, fmt.Errorf("log_level: invalid log level %q: %w", l.Level, err)) } + for component, level := range l.ComponentLevels { + if _, err := zerolog.ParseLevel(level); err != nil { + errs = append(errs, fmt.Errorf("component_levels.%s: invalid log level %q: %w", component, level, err)) + } + } return errs }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/config/config.go` around lines 42 - 48, Logging.validate currently only checks l.Level and ignores entries in l.ComponentLevels; update Logging.validate to iterate over the l.ComponentLevels map and validate each value with zerolog.ParseLevel, appending formatted errors (including the component key and invalid value) to the errs slice so Config.Validate surfaces these issues early (this complements the checks done in NewFactory).server/internal/migrate/runner.go (1)
43-52: Promote the component key to a shared constant.
"migration_runner"is now part of thelogging.component_levelscontract. Keeping it inline makes future renames easy to miss in config docs or tests; a package-level const (or shared component-name list) would make that surface safer as more components adopt the factory.♻️ Suggested shape
+const componentMigrationRunner = "migration_runner" + func NewRunner( hostID string, store *Store, injector *do.Injector, loggerFactory *logging.Factory, migrations []Migration, candidate *election.Candidate, ) *Runner { return &Runner{ hostID: hostID, store: store, injector: injector, - logger: loggerFactory.Logger("migration_runner"), + logger: loggerFactory.Logger(componentMigrationRunner), migrations: migrations, candidate: candidate, errCh: make(chan error, 1), doneCh: make(chan struct{}), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/migrate/runner.go` around lines 43 - 52, The literal component name "migration_runner" used in the Runner constructor (loggerFactory.Logger("migration_runner")) should be promoted to a package-level constant to avoid drifting config/tests; add a const (e.g., const componentMigrationRunner = "migration_runner") near the top of the package and replace the inline literal in the Runner creation and any other usages (Runner, loggerFactory.Logger) with that constant so renames are centralized and less error-prone.server/internal/etcd/provide.go (1)
55-79: Use a component logger for the etcd reconfiguration path.Now that this provider already resolves
*logging.Factory, the debug block at Lines 66-71 is the main etcd startup path still bypassinglogging.component_levels. If those messages are part of etcd troubleshooting, derive this logger fromloggerFactorytoo so a per-component debug override actually surfaces them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/etcd/provide.go` around lines 55 - 79, The debug messages in the etcd reconfiguration path currently use the package-level `logger` and bypass per-component levels; resolve and use a component logger from the already-invoked `loggerFactory` instead: obtain a component logger (via `loggerFactory.Component` or equivalent) before the debug block and replace uses of `logger.Debug()` in this path with that component logger, and ensure `newEtcdForMode(newMode, cfg, loggerFactory)` (or its call-site) receives or uses this component logger so all etcd startup/reconfiguration logs respect `logging.component_levels`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/config/config.go`:
- Around line 42-48: Logging.validate currently only checks l.Level and ignores
entries in l.ComponentLevels; update Logging.validate to iterate over the
l.ComponentLevels map and validate each value with zerolog.ParseLevel, appending
formatted errors (including the component key and invalid value) to the errs
slice so Config.Validate surfaces these issues early (this complements the
checks done in NewFactory).
In `@server/internal/etcd/provide.go`:
- Around line 55-79: The debug messages in the etcd reconfiguration path
currently use the package-level `logger` and bypass per-component levels;
resolve and use a component logger from the already-invoked `loggerFactory`
instead: obtain a component logger (via `loggerFactory.Component` or equivalent)
before the debug block and replace uses of `logger.Debug()` in this path with
that component logger, and ensure `newEtcdForMode(newMode, cfg, loggerFactory)`
(or its call-site) receives or uses this component logger so all etcd
startup/reconfiguration logs respect `logging.component_levels`.
In `@server/internal/migrate/runner.go`:
- Around line 43-52: The literal component name "migration_runner" used in the
Runner constructor (loggerFactory.Logger("migration_runner")) should be promoted
to a package-level constant to avoid drifting config/tests; add a const (e.g.,
const componentMigrationRunner = "migration_runner") near the top of the package
and replace the inline literal in the Runner creation and any other usages
(Runner, loggerFactory.Logger) with that constant so renames are centralized and
less error-prone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba433b3e-b75d-4eec-8e0a-818d988dd9d2
📒 Files selected for processing (25)
changes/unreleased/Added-20260303-092813.yamldocs/installation/configuration.mdserver/internal/api/provide.goserver/internal/api/server.goserver/internal/config/config.goserver/internal/election/candidate.goserver/internal/election/candidate_test.goserver/internal/election/provide.goserver/internal/election/service.goserver/internal/etcd/embedded.goserver/internal/etcd/embedded_test.goserver/internal/etcd/provide.goserver/internal/etcd/remote.goserver/internal/etcd/remote_test.goserver/internal/logging/factory.goserver/internal/logging/provide.goserver/internal/migrate/migrations/add_task_scope.goserver/internal/migrate/provide.goserver/internal/migrate/runner.goserver/internal/migrate/runner_test.goserver/internal/scheduler/provide.goserver/internal/scheduler/service.goserver/internal/testutils/logger.goserver/internal/workflows/provide.goserver/internal/workflows/worker.go
Summary
Adds the ability to configure per-component log levels to make it easier to see, for example, debug logs for a single component.
Testing
Try setting a more verbose log level for a particular component in
docker/control-plane-dev/config.json. For example:{ "profiling_enabled": true "logging": { "component_levels": { "election_candidate": "debug" } } }PLAT-417