Draft: CSPL-4347 #1662
Open
Draft: CSPL-4347 #1662
+1,184
−1
Conversation
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
added 3 commits
January 16, 2026 14:42
Collaborator
Pull Request Test Coverage Report for Build 21068664710Details
💛 - Coveralls |
pkg/logging/slog_test.go
Outdated
| @@ -0,0 +1,947 @@ | |||
| // Copyright (c) 2018-2022 Splunk Inc. All rights reserved. | |||
Collaborator
There was a problem hiding this comment.
2026 instead of 2022?
kasiakoziol
reviewed
Jan 21, 2026
Igor-splunk
changed the base branch from
develop
to
feature/slog-as-default-logger
kasiakoziol
approved these changes
Jan 21, 2026
kubabuczak
reviewed
Jan 21, 2026
Comment on lines
+104
to
+106
| flag.StringVar(&slogLevel, "slog-level", "", "slog log level: debug, info, warn, error (overrides LOG_LEVEL env var)") | ||
| flag.StringVar(&slogFormat, "slog-format", "", "slog output format: json, text (overrides LOG_FORMAT env var)") | ||
| flag.BoolVar(&slogAddSource, "slog-add-source", false, "add source file:line to slog output (overrides LOG_ADD_SOURCE env var)") |
Collaborator
There was a problem hiding this comment.
Maybe those flags should be implementation agnostic? So just log-level, log-format and log-add-source
|
|
||
| // LoadConfig loads logging configuration from environment variables | ||
| // Environment variables take precedence over defaults | ||
| func LoadConfig() Config { |
Collaborator
There was a problem hiding this comment.
Do we use it outside of logging package? Maybe it shouldn't be exported?
| AddSource: false, | ||
| } | ||
|
|
||
| if level := os.Getenv(EnvLogLevel); level != "" { |
Collaborator
There was a problem hiding this comment.
[not in this PR] We should have one place where we define configuration. Having those system calls all over codebase is messy.
Comment on lines
+136
to
+158
| // NewProductionHandler creates a handler optimized for production | ||
| // JSON format, INFO level, no source location, sensitive data redaction | ||
| func NewProductionHandler() slog.Handler { | ||
| opts := &slog.HandlerOptions{ | ||
| Level: slog.LevelInfo, | ||
| AddSource: false, | ||
| ReplaceAttr: redactSensitiveData, | ||
| } | ||
|
|
||
| return slog.NewJSONHandler(os.Stdout, opts) | ||
| } | ||
|
|
||
| // NewDevelopmentHandler creates a handler optimized for development | ||
| // Text format, DEBUG level, source location enabled | ||
| func NewDevelopmentHandler() slog.Handler { | ||
| opts := &slog.HandlerOptions{ | ||
| Level: slog.LevelDebug, | ||
| AddSource: true, | ||
| ReplaceAttr: redactSensitiveData, | ||
| } | ||
|
|
||
| return slog.NewTextHandler(os.Stdout, opts) | ||
| } |
Collaborator
There was a problem hiding this comment.
Shouldn't we set up those opts via config? What peace of code is expected to call those functions?
Comment on lines
+180
to
+188
| func SetupLogger(cfg Config) *slog.Logger { | ||
| handler := NewHandler(cfg) | ||
| logger := slog.New(handler) | ||
| slog.SetDefault(logger) | ||
| return logger | ||
| } | ||
|
|
||
| // SetupLoggerWithAttrs initializes the global slog logger with common attributes | ||
| func SetupLoggerWithAttrs(cfg Config, component, version, build string) *slog.Logger { |
Collaborator
There was a problem hiding this comment.
Maybe we should use such function here:
SetupLogger(cfg Config, attrs ...slog.Attr)
Comment on lines
+201
to
+203
| func LevelFromString(s string) slog.Level { | ||
| return parseLevel(s) | ||
| } |
Collaborator
There was a problem hiding this comment.
[nit] Maybe we should just use LevelFromString and move parseLevel code here
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package logging |
Collaborator
There was a problem hiding this comment.
Why not black box testing?
Aren't we using build tags?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description
What does this PR have in it?
Key Changes
Highlight the updates in specific files
Testing and Verification
How did you test these changes? What automated tests are added?
Related Issues
Jira tickets, GitHub issues, Support tickets...
PR Checklist