Skip to content
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

add: log all errors wrapper for Logger component #143

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gregfurman
Copy link
Collaborator

@gregfurman gregfurman commented Oct 29, 2024

Adds an optional wrapped logger that will promote any log with an error type to the ERROR level.

This does not include ading an error in a logger's With or WithField.

For example, if you set:

logger:
    log_all_errors: true

Then any subsequent logger calling Warn, Info,Debug, Trace, or with an error type in the params will promote the log to an Error type.

In addition, a field @log_all_errors=true is added to all logs generated from the strict logger:

level=error msg="Trace message root module: fail"  @log_all_errors=true @service=bento_service

Copy link
Contributor

@richardartoul richardartoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments

continue
}

switch arg.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you probably want something like this:

_, isErr := arg.(error)

instead to see if it implements the error interface, not if its literally an error no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wont the switch case also check if the value arg implements the error interface?

Or are you suggesting we remove the check for *error as well since that's technically not an error type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if your tests pass its fine

@@ -14,13 +14,15 @@ const (
fieldFilePath = "path"
fieldFileRotate = "rotate"
fieldFileRotateMaxAge = "rotate_max_age_days"
fieldStrictLogging = "strict"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log_all_errors IMO instead of "strict" since thats way more clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update all the variables / go code to match the name as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@gregfurman gregfurman changed the title add: strict mode for Logger component add: log all errors wrapper for Logger component Oct 29, 2024
internal/log/strict.go Outdated Show resolved Hide resolved
internal/log/strict_test.go Outdated Show resolved Hide resolved
internal/log/strict_test.go Outdated Show resolved Hide resolved
@jem-davies
Copy link
Collaborator

I acknowledge the request for review - will give it a good look soon 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants