fix: config warnings honor logging.format/level (#113)#119
Merged
Conversation
parse() emitted deprecation and unknown-tier warnings through the default text handler before setupLogging() installed the operator-configured one, so they ignored logging.format/level and broke JSON log parsers. Buffer log records during config load and replay them through the real handler once it is installed, filtering by the configured level. Keeps the config package untouched; startup ordering is fixed in main. Fixes #113
Contributor
Reviewer's GuideStartup logging is refactored so config-time warnings are first buffered via a temporary slog.Handler and then replayed through the operator-configured logger, ensuring they honor logging.format/logging.level; tests validate buffering and flushing behavior, and a changelog fragment documents the fix. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The bufferHandler is explicitly single-threaded; if there’s any chance config-loading logs could be emitted from multiple goroutines in the future, consider adding a mutex now to avoid subtle data races around the records slice.
- bufferHandler.WithAttrs and WithGroup intentionally no-op, which could be surprising if future config-time logging starts using structured logging; consider tracking attrs/groups so the behavior matches other handlers even during buffering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The bufferHandler is explicitly single-threaded; if there’s any chance config-loading logs could be emitted from multiple goroutines in the future, consider adding a mutex now to avoid subtle data races around the records slice.
- bufferHandler.WithAttrs and WithGroup intentionally no-op, which could be surprising if future config-time logging starts using structured logging; consider tracking attrs/groups so the behavior matches other handlers even during buffering.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Config-time log warnings emitted during
config.parse()now honor the operator-configuredlogging.format/logging.levelinstead of always going out through the default text handler before logging was set up.Related Issue
Fixes #113
Changes
cmd/devcloud/buffer.go: a bufferingslog.Handlerthat captures records and replays them through a destination handler, gated by that handler's level filter (flushTo).cmd/devcloud/main.go: install the buffer as the default handler before config load, then replay buffered records through the real handler aftersetupLogging(). The load-error path falls back to the default handler and flushes so the error and any buffered warnings are still surfaced.internal/configis left untouched — the fix is purely a startup-ordering change inmain.Fixedchangelog fragment.Test Plan
cmd/devcloud/buffer_test.go: buffered Info+Warn records flushed to a JSON handler atlevel=warn— asserts the Warn record (with structured attrs) is replayed as JSON and the Info record is dropped; a second flush is a no-op. Also asserts the buffer accepts all levels.go vet ./cmd/devcloud/,go test ./cmd/devcloud/,go build ./...all pass.golangci-lint run ./cmd/devcloud/→ 0 issues.DEVCLOUD_SERVICES=tier9 devcloud --config <yaml with logging.format=json, deprecated dashboard key>→ both config warnings now emit as JSON with structured attrs ("token":"tier9").Checklist
golangci-lint run)changie new, see docs/release.md)