-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: update log level handling #141
Merged
Merged
Conversation
This file contains 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
Signed-off-by: catsby <[email protected]>
Signed-off-by: catsby <[email protected]>
Signed-off-by: catsby <[email protected]>
…ately Signed-off-by: catsby <[email protected]>
Signed-off-by: catsby <[email protected]>
5 tasks
Signed-off-by: catsby <[email protected]>
Racer159
requested changes
Sep 18, 2024
Signed-off-by: catsby <[email protected]>
Signed-off-by: catsby <[email protected]>
@Racer159 thanks again for the review. I applied your feedback and added an |
Signed-off-by: catsby <[email protected]>
Signed-off-by: catsby <[email protected]>
* add-e2e-env-var: remove code comment restore old e2e make step and use 'go list' to filter unit tests
ericwyles
previously approved these changes
Sep 19, 2024
* main: chore: use go-list to filter out e2e tests in test-unit step (#140)
Racer159
requested changes
Sep 19, 2024
Signed-off-by: catsby <[email protected]>
* main: feat: add templated conditionals to tasks (#139)
Racer159
approved these changes
Sep 19, 2024
21 tasks
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.
Short Description
This PR updates Maru's logging to match the standard library's
log/slog
package, and appropriately filters log messages according to levels. Tests included and feedback very welcome.Long Description
The current implementation of log level handling is misleading because our custom handler doesn't filter by level. The code comment mentions logging functions already being aware if they are allowed to be called, but in practice I have not found that to be true. This is further complicated by Maru's log levels not matching what
log/slog
uses both in values and in ordering.Example: slog uses these levels/ordering
(See here for source and long explaination to the numbers used: https://pkg.go.dev/log/slog#Level)
Where as Maru uses these levels/ordering:
(source: src/message/logging.go#L16-L29)
In theory, implementing the
Handler
interface would have us ignore messages with a lower level than the one set. Maru sets it's default log level toInfoLevel
(from Maru's constants, not slog's) which results in a value of1
. However because ourInfoLevel
is1
and slog'sLevelInfo
is0
, then in theory Maru's logger should actually ignore it because it's a lower level. A call tomessage.SLog.Info()
(ex. in utils.go) invokes slog'sInfo()
method, which calls an internal methodlog()
with slog'sLevelInfo
level which is a value of0
. Thelog()
method calls the handlersEnabled()
method to see if it should log the message, which for maru always returnstrue
, and then passes it on to the handlersHandle()
method (Maru'sHandle()
method here) and we print the message, even though we shouldn't because it's a lower value than our default.The net effect is setting the log level with
--log-level
has no effect, except for when setting totrace
, in which case we output pterm debugging information with lines for errors/traces and debug calls. Otherwise Maru will always output all of the logs fromDebug()
,Info()
,Warn()
orError()
regardless of the level set.This PR updates Maru's log level constants both in values and ordering to match the standard library
log/slog
package, and implements theEnabled()
method in the handler to filter correctly based on level. The net effect of this PR should be benign in that we aren't changing any usage details, but going forward we'll correctly output logs according to the set level.NOTE: This PR is based off of my other PR (#140) for changing the
Makefile
with regards totest-unit
andtest-e2e
tests.Test output:
➜ go test ./src/message/... -v -count=1 === RUN Test_LogLevel_Diff === RUN Test_LogLevel_Diff/DebugLevel === RUN Test_LogLevel_Diff/InfoLevel === RUN Test_LogLevel_Diff/InfoWarnLevel === RUN Test_LogLevel_Diff/WarnInfoLevel === RUN Test_LogLevel_Diff/InfoTraceLevel === RUN Test_LogLevel_Diff/TraceInfoLevel === RUN Test_LogLevel_Diff/TraceLevel --- PASS: Test_LogLevel_Diff (0.00s) --- PASS: Test_LogLevel_Diff/DebugLevel (0.00s) --- PASS: Test_LogLevel_Diff/InfoLevel (0.00s) --- PASS: Test_LogLevel_Diff/InfoWarnLevel (0.00s) --- PASS: Test_LogLevel_Diff/WarnInfoLevel (0.00s) --- PASS: Test_LogLevel_Diff/InfoTraceLevel (0.00s) --- PASS: Test_LogLevel_Diff/TraceInfoLevel (0.00s) --- PASS: Test_LogLevel_Diff/TraceLevel (0.00s) PASS ok github.com/defenseunicorns/maru-runner/src/message 0.235s
Related
uds run
uds-cli#916Type of change
Checklist before merging