[Logging] Add support for component scoped log-levels#8477
[Logging] Add support for component scoped log-levels#8477peterargue wants to merge 27 commits intomasterfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…gger Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…vel string earlier
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds a LogRegistry-based per-component logging system with parsing/validation utilities, component-level level-filtering writers, admin commands to set/get/reset levels, CLI/node integration, documentation, tests/benchmarks, and a .gitignore entry for Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
utils/logging/component_writer.go (1)
22-24: Fail fast on nil constructor inputs.A nil
levelordelegatewill panic later on log writes. Consider validating at construction time for clearer failure mode.Proposed change
func NewComponentLevelWriter(level *atomic.Int32, delegate zerolog.LevelWriter) zerolog.LevelWriter { + if level == nil { + panic("logging.NewComponentLevelWriter: level must not be nil") + } + if delegate == nil { + panic("logging.NewComponentLevelWriter: delegate must not be nil") + } return &componentLevelWriter{level: level, delegate: delegate} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/logging/component_writer.go` around lines 22 - 24, The constructor NewComponentLevelWriter should validate its inputs to fail fast: check that the level (*atomic.Int32) and delegate (zerolog.LevelWriter) are not nil and return an explicit error or panic with a clear message; update NewComponentLevelWriter to perform these nil checks and either return an error (or document/choose to panic) before constructing &componentLevelWriter{level: level, delegate: delegate} so later writes (methods on componentLevelWriter) cannot panic due to nil fields.utils/logging/registry.go (1)
165-173: Harden registry API boundaries with internal pattern validation.
SetLevelandResetcurrently trust callers entirely; malformed patterns can be silently persisted or ignored. Adding internal validation keeps invariants intact even when these methods are called outside admin validators.Proposed hardening diff
func (r *LogRegistry) SetLevel(pattern string, level zerolog.Level) { r.mu.Lock() defer r.mu.Unlock() pattern = NormalizePattern(pattern) + if pattern == "*" { + panic(`log registry: "*" is not valid for SetLevel; use SetDefaultLevel`) + } + if err := ValidatePattern(pattern); err != nil { + panic(fmt.Sprintf("log registry: %s", err)) + } r.overrides[pattern] = level r.applyToMatching(pattern) r.updateGlobalLevel() } @@ func (r *LogRegistry) Reset(patterns ...string) { r.mu.Lock() defer r.mu.Unlock() for _, pattern := range patterns { pattern = NormalizePattern(pattern) if pattern == "*" { r.overrides = make(map[string]zerolog.Level) for id, al := range r.registered { al.Store(int32(r.resolve(id))) } break } + if err := ValidatePattern(pattern); err != nil { + panic(fmt.Sprintf("log registry: %s", err)) + } delete(r.overrides, pattern) r.applyToMatching(pattern) } r.updateGlobalLevel() }As per coding guidelines, "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented."
Also applies to: 180-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/logging/registry.go` around lines 165 - 173, Add internal pattern validation to harden LogRegistry API: implement a small validator (e.g., validatePattern) and call it from SetLevel and Reset before normalizing/persisting, so malformed inputs are never written to r.overrides or used in r.applyToMatching; if validation fails, do not persist or call applyToMatching—log the failure via the registry logger (or return an error if you can change the signature) and return early. Update SetLevel (which currently calls NormalizePattern, r.overrides[pattern] = level, r.applyToMatching(pattern), r.updateGlobalLevel()) to validate the raw input first, and do the same in Reset (the method around lines 180-197 that removes patterns from the overrides map) so only validated patterns are removed/applied. Ensure the validator enforces the same rules used by NormalizePattern and document its use within LogRegistry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/commands/common/component_log_level_reset.go`:
- Around line 52-57: The handler drops the reset-all pattern ("*") because the
code 'if p == "*" { if len(raw) > 1 { return ... } continue }' skips adding "*"
to the validator data so Reset() is called with no patterns; change the logic so
when p == "*" and len(raw) == 1 you add the "*" pattern into the ValidatorData
(or otherwise ensure the validator receives the "*" pattern) instead of
continuing/omitting it; apply the same change to the other identical block
referenced around the Reset() call so Reset() receives the "*" pattern and
performs a global reset.
In `@admin/commands/common/component_log_level_set.go`:
- Around line 67-72: The branch that checks for pattern == "*" is unreachable
because logging.ValidatePattern(pattern) returns an error first; update the
order in the function handling the component log level set so the wildcard check
runs before calling logging.ValidatePattern. Specifically, in the code that
inspects the variable pattern, move the check pattern == "*" ahead of the call
to logging.ValidatePattern(pattern) and return the tailored
admin.NewInvalidAdminReqErrorf("global wildcard \"*\" is not a valid when
setting component level logging. use set-log-level instead") there; keep the
ValidatePattern call afterwards to validate other patterns.
In `@utils/logging/component_writer_test.go`:
- Around line 59-60: The test currently discards the error from
w.WriteLevel(zerolog.DebugLevel, []byte(`{"level":"debug"}`)), which can hide
failures; change the call to capture the returned error (e.g., err :=
w.WriteLevel(...)) and assert it is nil (use require.NoError(t, err) or
assert.NoError(t, err)) before checking buf.Len() so the first-write error in
the AtomicUpdate test is surfaced; references: w.WriteLevel, buf.Len(), and the
AtomicUpdate test function.
In `@utils/logging/registry_test.go`:
- Around line 286-294: The test asserts the buffer is empty after calling
r.Reset("hotstuff") but the buffer still contains the earlier "debug visible"
message; fix by clearing the test buffer between the two checks (e.g., call
buf.Reset() or reinitialize the buffer) after verifying the debug message from
logger.Debug().Msg and before calling r.Reset("hotstuff") and the subsequent
logger.Debug() call so that buf.String() reflects only the post-reset output;
reference r.SetLevel, r.Reset, logger.Debug and buf.String in the change.
---
Nitpick comments:
In `@utils/logging/component_writer.go`:
- Around line 22-24: The constructor NewComponentLevelWriter should validate its
inputs to fail fast: check that the level (*atomic.Int32) and delegate
(zerolog.LevelWriter) are not nil and return an explicit error or panic with a
clear message; update NewComponentLevelWriter to perform these nil checks and
either return an error (or document/choose to panic) before constructing
&componentLevelWriter{level: level, delegate: delegate} so later writes (methods
on componentLevelWriter) cannot panic due to nil fields.
In `@utils/logging/registry.go`:
- Around line 165-173: Add internal pattern validation to harden LogRegistry
API: implement a small validator (e.g., validatePattern) and call it from
SetLevel and Reset before normalizing/persisting, so malformed inputs are never
written to r.overrides or used in r.applyToMatching; if validation fails, do not
persist or call applyToMatching—log the failure via the registry logger (or
return an error if you can change the signature) and return early. Update
SetLevel (which currently calls NormalizePattern, r.overrides[pattern] = level,
r.applyToMatching(pattern), r.updateGlobalLevel()) to validate the raw input
first, and do the same in Reset (the method around lines 180-197 that removes
patterns from the overrides map) so only validated patterns are removed/applied.
Ensure the validator enforces the same rules used by NormalizePattern and
document its use within LogRegistry.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.gitignoreadmin/commands/common/component_log_level_reset.goadmin/commands/common/component_log_level_set.goadmin/commands/common/component_log_levels_get.goadmin/commands/common/config_get.goadmin/commands/common/config_set.goadmin/commands/common/configs_list.goadmin/commands/common/golog_level_set.goadmin/commands/common/identity_get.goadmin/commands/common/log_level_set.gocmd/node_builder.gocmd/scaffold.goutils/logging/README.mdutils/logging/component_writer.goutils/logging/component_writer_test.goutils/logging/export_test.goutils/logging/parse.goutils/logging/parse_test.goutils/logging/registry.goutils/logging/registry_bench_test.goutils/logging/registry_test.goutils/logging/validate.goutils/logging/validate_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/logging/registry.go (1)
71-83: Fail fast whenbaseWriteris nil.If
baseWriteris nil,toLevelWritercan wrap a nil delegate and defer failure to runtime writes. Add a constructor guard to make misconfiguration explicit.Proposed hardening
func NewLogRegistry( baseWriter io.Writer, globalDefault zerolog.Level, staticConfig map[string]zerolog.Level, ) *LogRegistry { + if baseWriter == nil { + panic("log registry: baseWriter must not be nil") + } + static := make(map[string]zerolog.Level, len(staticConfig)) maps.Copy(static, staticConfig)Also applies to: 305-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/logging/registry.go` around lines 71 - 83, NewLogRegistry currently calls toLevelWriter(baseWriter) without validating baseWriter; add a fail-fast guard at the top of NewLogRegistry that checks if baseWriter == nil and panics (or returns an explicit error if you prefer) with a clear message (e.g., "nil baseWriter passed to NewLogRegistry") before calling toLevelWriter, so misconfiguration surfaces immediately; apply the same nil-check to the other constructor in this file that also calls toLevelWriter to avoid deferring failures to runtime writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/logging/registry.go`:
- Around line 163-171: The SetLevel method (and similarly any methods that
mutate r.overrides such as the sibling block around lines 176-193) currently
normalizes and stores any pattern blindly; modify SetLevel to validate the
normalized pattern first (e.g., via a new validateRuntimePattern or
ValidatePattern helper) and return an error to callers if the pattern is
malformed, only proceeding to mutate r.overrides, call
r.applyToMatching(pattern) and r.updateGlobalLevel() when validation succeeds;
ensure the same validation + error return is applied to the other
override-setting function(s) referenced in the diff so invalid selectors are
rejected rather than silently accepted.
---
Nitpick comments:
In `@utils/logging/registry.go`:
- Around line 71-83: NewLogRegistry currently calls toLevelWriter(baseWriter)
without validating baseWriter; add a fail-fast guard at the top of
NewLogRegistry that checks if baseWriter == nil and panics (or returns an
explicit error if you prefer) with a clear message (e.g., "nil baseWriter passed
to NewLogRegistry") before calling toLevelWriter, so misconfiguration surfaces
immediately; apply the same nil-check to the other constructor in this file that
also calls toLevelWriter to avoid deferring failures to runtime writes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
utils/logging/parse.goutils/logging/registry.go
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/logging/parse.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
utils/logging/registry_test.go (1)
21-23: Useio.DiscardintestRegistryto keep test output quiet.This helper does not assert emitted output, so writing to stderr is unnecessary noise.
Suggested cleanup
import ( "bytes" - "os" + "io" "testing" @@ - r := logging.NewLogRegistry(os.Stderr, defaultLevel, static) - return r, zerolog.New(os.Stderr).Level(zerolog.TraceLevel) + r := logging.NewLogRegistry(io.Discard, defaultLevel, static) + return r, zerolog.New(io.Discard).Level(zerolog.TraceLevel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/logging/registry_test.go` around lines 21 - 23, Replace the use of os.Stderr in the test helper with io.Discard so tests don't emit noise: update the call to logging.NewLogRegistry(...) and the zerolog.New(...) invocation in the test helper (the function that returns r and zerolog.New(...).Level(...), referencing logging.NewLogRegistry, defaultLevel, static and zerolog.New) to pass io.Discard instead of os.Stderr.utils/logging/registry.go (1)
180-191: Avoid mutating caller-providedpatternsinReset.Normalizing in place mutates the variadic backing array from callers that pass a slice, which is a surprising side effect for a public API.
Suggested refactor
func (r *LogRegistry) Reset(patterns ...string) error { - for i, pattern := range patterns { + normalizedPatterns := make([]string, 0, len(patterns)) + for i, pattern := range patterns { normalized := NormalizePattern(pattern) if normalized == "*" { if len(patterns) > 1 { return fmt.Errorf("\"*\" must be the only pattern when resetting all components") } } else if err := ValidatePattern(normalized); err != nil { return fmt.Errorf("pattern %d is invalid: %w", i, err) } - patterns[i] = normalized + normalizedPatterns = append(normalizedPatterns, normalized) } @@ - for _, pattern := range patterns { + for _, pattern := range normalizedPatterns {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/logging/registry.go` around lines 180 - 191, In Reset, avoid mutating the caller-provided patterns slice; instead create a new local slice (e.g., normalizedPatterns := make([]string, 0, len(patterns))) and loop over the input patterns, compute normalizedPattern := NormalizePattern(pattern), perform the "*" check using the original len(patterns) and call ValidatePattern(normalizedPattern) as before, append normalizedPattern to normalizedPatterns, and then use normalizedPatterns for the rest of the function; this preserves the original slice and removes the surprising side effect in LogRegistry.Reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/commands/common/component_log_level_set.go`:
- Around line 53-72: The handler currently validates patterns and levels while
iterating the input map (building parsedComponentLevel) which can apply partial
updates nondeterministically; instead add comprehensive validation and
normalization into the request Validator (validate every pattern: type/string,
zerolog.ParseLevel, maxPatternLength, disallow "*" wildcard) and produce a
deterministic, ordered slice of parsedComponentLevel (e.g., sorted by pattern)
that the Handler can trust; ensure Validator returns an error for any invalid
entry so the Handler only ever applies an all-or-nothing update via
LogRegistry.SetLevel using the normalized slice.
In `@cmd/scaffold.go`:
- Around line 919-923: The root logger is currently assigned directly to
fnb.Logger at Trace level and bypasses component filtering; change the
assignment so fnb.Logger is obtained from the registry (use
fnb.LogRegistry.Logger()) after creating fnb.LogRegistry (created via
logging.NewLogRegistry) and remove direct use of the unscoped `log` for
fnb.Logger so all root/unscoped logs are filtered by LogRegistry; update
references where fnb.Logger is set to ensure you call LogRegistry.Logger()
instead of assigning `log` directly.
---
Nitpick comments:
In `@utils/logging/registry_test.go`:
- Around line 21-23: Replace the use of os.Stderr in the test helper with
io.Discard so tests don't emit noise: update the call to
logging.NewLogRegistry(...) and the zerolog.New(...) invocation in the test
helper (the function that returns r and zerolog.New(...).Level(...), referencing
logging.NewLogRegistry, defaultLevel, static and zerolog.New) to pass io.Discard
instead of os.Stderr.
In `@utils/logging/registry.go`:
- Around line 180-191: In Reset, avoid mutating the caller-provided patterns
slice; instead create a new local slice (e.g., normalizedPatterns :=
make([]string, 0, len(patterns))) and loop over the input patterns, compute
normalizedPattern := NormalizePattern(pattern), perform the "*" check using the
original len(patterns) and call ValidatePattern(normalizedPattern) as before,
append normalizedPattern to normalizedPatterns, and then use normalizedPatterns
for the rest of the function; this preserves the original slice and removes the
surprising side effect in LogRegistry.Reset.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
admin/commands/common/component_log_level_reset.goadmin/commands/common/component_log_level_set.gocmd/node_builder.gocmd/scaffold.goutils/logging/component_writer_test.goutils/logging/registry.goutils/logging/registry_test.goutils/logging/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/logging/component_writer_test.go
- cmd/node_builder.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
admin/commands/common/component_log_level_set.go (1)
55-76:⚠️ Potential issue | 🟠 MajorReject duplicate normalized patterns to keep updates deterministic.
Two raw keys can normalize to the same pattern (e.g., casing/whitespace variants). In that case, the winning level depends on map iteration order, making runtime overrides non-deterministic.
Proposed fix
func (s *SetComponentLogLevelCommand) Validator(req *admin.CommandRequest) error { @@ - parsed := make([]parsedComponentLevel, 0, len(raw)) + parsed := make([]parsedComponentLevel, 0, len(raw)) + seen := make(map[string]struct{}, len(raw)) for pattern, val := range raw { @@ - if len(pattern) > maxPatternLength { - return admin.NewInvalidAdminReqErrorf("pattern %q is too long (max %d characters)", pattern, maxPatternLength) - } pattern = logging.NormalizePattern(strings.TrimSpace(pattern)) + if len(pattern) > maxPatternLength { + return admin.NewInvalidAdminReqErrorf("pattern %q is too long (max %d characters)", pattern, maxPatternLength) + } + if _, exists := seen[pattern]; exists { + return admin.NewInvalidAdminReqErrorf("duplicate pattern %q after normalization", pattern) + } + seen[pattern] = struct{}{} if pattern == "*" { return admin.NewInvalidAdminReqErrorf("global wildcard \"*\" is not a valid when setting component level logging. use set-log-level instead") } - if err := logging.ValidatePattern(logging.NormalizePattern(pattern)); err != nil { + if err := logging.ValidatePattern(pattern); err != nil { return admin.NewInvalidAdminReqErrorf("invalid pattern %q: %w", pattern, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/commands/common/component_log_level_set.go` around lines 55 - 76, Normalize each raw key once (e.g., norm := logging.NormalizePattern(strings.TrimSpace(pattern))) before further checks, then track seen normalized patterns with a map[string]struct{} and if a normalized pattern is already present return admin.NewInvalidAdminReqErrorf("duplicate normalized pattern %q", norm); use norm for length checks, logging.ValidatePattern(norm) and when appending parsedComponentLevel{pattern: norm, level: level} so duplicate normalized inputs are rejected and updates remain deterministic.
🧹 Nitpick comments (1)
admin/commands/common/component_log_level_set.go (1)
86-87: Defensively guardValidatorDatabefore type assertion.The direct assertion can panic if handler execution is reached with malformed/internal call flow.
Proposed hardening
func (s *SetComponentLogLevelCommand) Handler(_ context.Context, req *admin.CommandRequest) (interface{}, error) { - entries := req.ValidatorData.([]parsedComponentLevel) + entries, ok := req.ValidatorData.([]parsedComponentLevel) + if !ok { + return nil, admin.NewInvalidAdminReqFormatError("validated component-level payload is missing") + } + if s.registry == nil { + return nil, fmt.Errorf("log registry is not initialized") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/commands/common/component_log_level_set.go` around lines 86 - 87, Guard req.ValidatorData before asserting to []parsedComponentLevel by first checking for nil and using the comma-ok form (e.g., v, ok := req.ValidatorData.([]parsedComponentLevel)); if !ok or v == nil, return a clear error or handle the invalid input path instead of proceeding. Replace the direct assertion that assigns to entries with this safe check, and ensure any error path uses the same handler error/response mechanism as the surrounding code so callers don’t hit a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@admin/commands/common/component_log_level_set.go`:
- Around line 55-76: Normalize each raw key once (e.g., norm :=
logging.NormalizePattern(strings.TrimSpace(pattern))) before further checks,
then track seen normalized patterns with a map[string]struct{} and if a
normalized pattern is already present return
admin.NewInvalidAdminReqErrorf("duplicate normalized pattern %q", norm); use
norm for length checks, logging.ValidatePattern(norm) and when appending
parsedComponentLevel{pattern: norm, level: level} so duplicate normalized inputs
are rejected and updates remain deterministic.
---
Nitpick comments:
In `@admin/commands/common/component_log_level_set.go`:
- Around line 86-87: Guard req.ValidatorData before asserting to
[]parsedComponentLevel by first checking for nil and using the comma-ok form
(e.g., v, ok := req.ValidatorData.([]parsedComponentLevel)); if !ok or v == nil,
return a clear error or handle the invalid input path instead of proceeding.
Replace the direct assertion that assigns to entries with this safe check, and
ensure any error path uses the same handler error/response mechanism as the
surrounding code so callers don’t hit a panic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5c5d50f-977c-4356-95d0-ab0a0a2140d8
📒 Files selected for processing (5)
admin/commands/common/component_log_level_reset.goadmin/commands/common/component_log_level_set.gocmd/node_builder.gocmd/scaffold.goutils/logging/registry.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/node_builder.go
- admin/commands/common/component_log_level_reset.go
Problem
Flow nodes had a single global log level with no way to change it per component at runtime. Debugging required restarting the node with a different level, and verbose debug logging from one component would flood logs from all others.
Solution
Introduces
LogRegistryinutils/logging— a component-scoped log level controller that sits in zerolog's writer chain and filters events per-component, with no overhead on suppressed events.Key design points:
registry.Logger(parent, id), receiving azerolog.Loggerbacked by a per-component writer. Level changes take effect immediately on all loggers derived from that component (including.With()children).zerolog.GlobalLevelis kept at the minimum of all component levels to preserve zerolog's pre-event-creation short-circuit (zero allocation on suppressed events)."hotstuff","hotstuff.voter"). Wildcard patterns ("hotstuff.*") match all descendants.New admin commands (via existing admin HTTP server):
get-component-log-levels— list all registered components and their current effective levelset-component-log-level— set a level for an exact ID or wildcard patternreset-component-log-level— remove a runtime override, falling back to static config or the global defaultStatic config via
--component-log-levelsflag (e.g.hotstuff=debug,network.*=warn) allows per-component levels to be set at startup, with runtime admin overrides on top.Benchmark Results
Benchmarks compare
baseline(plain zerolog) vsregistry(zerolog routed throughLogRegistry).Run on Apple M4 Pro,
goos: darwin,goarch: arm64.Suppressed events have zero overhead — zerolog's global-level short-circuit fires before any registry code runs. Active event overhead is within noise (~1–2 ns).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores