Convert from log to slog #154
Conversation
|
Sorry wrote a better git commit, missed that is it wans't a good enough one Not sure if I set the correct log levels in the applications, i.e. if some logs should be moved to |
|
@xonvanetta I will try to take a look on it tommorow |
cmd/exporter/main.go
Outdated
| warn := godotenv.Load() | ||
| if warn != nil { | ||
| log.Println(warn) | ||
| err := godotenv.Load() |
There was a problem hiding this comment.
since this error is not a critical error, leading to panic i would like you to leave name of the variable as "warn"
There was a problem hiding this comment.
I do not agree on this, since the returned interface is an error I would like to use the variable name err, even if it's a trivial error. The problem lies at first glance, I didn't think this was an error type.
But in the end this is just my opinion, will of course change it to warn again :)
There was a problem hiding this comment.
You can change it to something like erorr and criticalError
But I would like to distingiush these two, as godotenv load can safely fail and logger should crash the application in my opinion
| } | ||
|
|
||
| logger, err := config.SetupSlog() | ||
| if err != nil { |
There was a problem hiding this comment.
I think in this case we should just crash the application
| handler = slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{ | ||
| Level: level, | ||
| }) | ||
| case "json": |
|
I like where it is going, feel free to convert rest of the log calls to slog calls |
kuskoman
added
enhancement
|
I have fixed the comments and found no more log calls that can be moved to slog |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #154 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 13 +1
Lines 570 589 +19
=========================================
+ Hits 570 589 +19
☔ View full report in Codecov by Sentry. |
collectors/collector_manager.go
Outdated
| executionStatus = "error" | ||
| } else { | ||
| log.Printf("executor %s succeeded after %s", name, executionDuration) | ||
| slog.Info("executor succeeded", "name", name, "duration", executionDuration) |
Default log level is `info` and default log format is `text`. Added json or text handler for slog. Added log level handler. Changed all places from log to slog with what I thought was the correct level. Added test to verify error handling in setuping up slog is working. Resolves: kuskoman#151
|
I will leave it for a moment on the master branch before releasing it. You should be able to grab the master version from github actions output |
|
I grabbed |
Converted from log to slog
Default log level is
infoand default log format istext.Added json or text handler for slog.
Added log level handler.
Changed all places from log to slog with what I thought was the correct level.
Added test to verify error handling in setuping up slog is working.
Resolves: #151