Logger improvements (foundations) #1556
Conversation
This is the recommended approach and means we don't need custom plugs.
lib/nerves_hub/logger_formatter.ex
Outdated
| @@ -1,7 +1,7 @@ | |||
| defmodule NervesHub.LoggerFormatter do | |||
| @pattern Logger.Formatter.compile("$time [$level] $metadata$message\n") | |||
| def format(level, message, timestamp, metadata) do | |||
| metadata = Keyword.drop(metadata, [:line, :file, :domain, :application]) | |||
| metadata = Keyword.drop(metadata, [:line, :file, :domain, :application, :mfa, :pid]) | |||
There was a problem hiding this comment.
This is the only thing I'm unsure of. We might use the mfa for something, I'll check tomorrow
There was a problem hiding this comment.
Thanks for the feedback. From my experience, mfa is mostly noise as it always has the same output of which function and module the logging came from, which is always one of the reporters.
If SmartRent would like to keep it, I'll look into how we can make this a runtime config.
There was a problem hiding this comment.
It has been useful in the past to establish consistency for log identification and grouping, so I/SmartRent would like to retain the ability to have :mfa in the logs if its not too much trouble.
:pid can happily go 🙇
There was a problem hiding this comment.
Thanks for the feedback @rraub , I'll make this configurable as I believe its noise for the majority of people. It will mean that you will need to add an env var to enable it, is that alright with you? (I'll document the env var in the PR)
There was a problem hiding this comment.
@rraub I've updated the PR, as well as documenting in the PR description how to enable :mfa information. (I need to work on some more complete docs on all the env vars and how they are used)
This is based on how Phoenix and other libs use telemetry for logging requests, in fact, we also use telemetry for logging, so this is just following that best practice as well.
I have more code related to this, but I wanted to create a PR for these changes first so we can have any discussions around the approach.
The
:mfainformation is excluded from the log by default but can be added back in by usingLOG_INCLUDE_MFA=true