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 structured logging via logrus #15

Merged
merged 3 commits into from
Mar 13, 2021
Merged

Add structured logging via logrus #15

merged 3 commits into from
Mar 13, 2021

Conversation

JonathonReinhart
Copy link
Collaborator

This adds structured logging using logrus.

This PR is a result of discussions on #13 and is based loosely on grafana/smtprelay#10.

This adds two new config options to the INI file:

  • log_format which can be one of:
    • default -- The default logrus format, but with full timestamps, similar to the previous output:
      INFO[2021-03-13T00:54:21-05:00] listening on address                          address=":2525"
      
    • plain -- The default logrus format, but with no timestamps (useful for running under Systemd)
      INFO listening on address                          address=":2525"
      
    • json -- JSON formatting with RFC3339Nano timestamp format:
      {"address":":2525","level":"info","msg":"listening on address","time":"2021-03-13T00:56:40.251607698-05:00"}
      
  • log_level -- The logging level; defaults to "info" (can be panic, fatal, error, warn, info, debug, trace)

@JonathonReinhart
Copy link
Collaborator Author

@dannykopping Please take a look and let me know if this doesn't work for you guys. Aside from obvious log location or message changes due to the various refactorings, this differs from what you have in the following ways:

  • I set the default log file to stderr rather than stdout, because I think that's more common, and also the logrus default.
  • I set the default log format to "default" (plain text), to more closely match the previous behavior.
  • I set the default log level to "info" based on my and @decke's preference.
  • Errors are recorded to the error key, not err as before. If that's a big problem, we can set `logrus.ErrorKey = "err", but I thought it made sense to leave it at the default.

I think you should be able to run with the nearly equivalent functionality as before with this INI snipppet:

logfile = "/dev/stdout"
log_format = "json"
log_level = "debug"

config.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
var smtpError smtpd.Error

switch err.(type) {
case *textproto.Error:
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch! In which kind of error situation does SendMail return this?

Copy link
Collaborator Author

@JonathonReinhart JonathonReinhart Mar 13, 2021

Choose a reason for hiding this comment

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

I can't take credit for this; it was taken straight from #13. But in my testing I have seen:

ERRO[2021-03-13T02:47:27-05:00] delivery failed                               err_code=530 err_msg="5.7.0 Authentication Required. Learn more at\n5.7.0  https://support.google.com/mail/?p=WantAuthError h75sm6304163qke.80 - gsmtp" from=jreinhart@jrr-vaio host="smtp.gmail.com:587" peer=127.0.0.1 to="[jonathon.reinhart@gmail.com]" uuid=ada212ce-ca9d-4a2b-923a-830ce6f19e37

Copy link
Owner

Choose a reason for hiding this comment

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

@dannykopping do you know in which situation SendMail returns this error? I can check myself later but need a bigger screen.

Copy link

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Looks pretty good @JonathonReinhart - thanks for taking the time to do this!
Left a couple thoughts but overall happy with it, and kind of you to add this for us to maintain our current behaviour:

I think you should be able to run with the nearly equivalent functionality as before with this INI snipppet:

config.go Show resolved Hide resolved
main.go Show resolved Hide resolved
This was based loosely on an earlier implementation by
Danny Kopping <danny.kopping@grafana.com>
The causes logrus to write to stderr.
@JonathonReinhart
Copy link
Collaborator Author

Okay this should be ready to go now if @decke is happy with my updates.

@decke decke merged commit d1933a2 into master Mar 13, 2021
@decke
Copy link
Owner

decke commented Mar 13, 2021

Thanks a lot guys! It was a pleasure to see how opensource can benefit us all.

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.

None yet

3 participants