-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(instrumentation): adopt slog #6907
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to ed2012c in 1 minute and 40 seconds
More details
- Looked at
908
lines of code in16
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. pkg/errors/errors.go:35
- Draft comment:
Consider usingslog.Strings
instead ofslog.Any
for theadditional
field if it always contains strings. - Reason this comment was not posted:
Confidence changes required:50%
TheLogValue
method in thebase
struct usesslog.Any
for theadditional
field, which is an array of strings. This is correct as it allows for flexibility in logging different types of data, but it's worth noting that if the array is always strings,slog.Strings
could be used for clarity.
2. pkg/factory/setting.go:45
- Draft comment:
Ensure that theLogger
method returns a non-nil logger to avoid potential nil pointer dereferences. - Reason this comment was not posted:
Confidence changes required:30%
TheLogger
method in thescoped
struct returns a pointer toslog.Logger
, which is consistent with the rest of the codebase. No issues here.
3. pkg/instrumentation/logger.go:19
- Draft comment:
slog.SetLogLoggerLevel
is not a standard method in theslog
package. Ensure this is a valid method or replace it with the correct one. - Reason this comment was not posted:
Comment did not seem useful.
4. pkg/instrumentation/instrumentationtest/instrumentation.go:24
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_5Fns5CMUjL0Pna6u
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments
Summary
feat(instrumentation): adopt slog
Related Issues / PR's
Important
Adopt
slog
for logging across the codebase, replacingzap
, updating configurations, and adding structured logging with trace correlation.zap
withslog
for logging inpkg/errors/errors.go
,pkg/factory/setting.go
,pkg/instrumentation/instrumentation.go
,pkg/sqlmigrator/migrator.go
, andpkg/sqlstore/sqlitesqlstore/provider.go
.LogValue()
method tobase
struct inpkg/errors/errors.go
for structured logging.NewLogger()
inpkg/instrumentation/logger.go
to createslog
logger with JSON handler.correlation
log handler inpkg/instrumentation/loghandler/correlation.go
to include trace and span IDs in logs.LogsConfig
inpkg/instrumentation/config.go
to useslog.Level
.example.yaml
to removeenabled
field from logs and add comments for clarity.go.mod
andgo.sum
to includeslog
and update related dependencies.TestNewWithEnvProvider
inpkg/instrumentation/config_test.go
to validate configuration loading from environment variables.TestCorrelation
inpkg/instrumentation/loghandler/correlation_test.go
to test log correlation with trace and span IDs.This description was created by for ed2012c. It will automatically update as commits are pushed.