dnsimple / erldns

DNS server, in Erlang.
MIT License
398 stars 98 forks source link

Implemented structured logging support using OTP21+ logger #101

Closed m0rcq closed 3 years ago

m0rcq commented 4 years ago
aeden commented 4 years ago

@m0rcq could you please provide a comment here explaining how to verify this? Does the erlang logger need to be configured to have the log entries output somewhere?

m0rcq commented 4 years ago
{kernel,[
  {logger_level, debug},
  {logger, [
    {handler, structured, logger_std_h,
       #{formatter => {flatlog, #{
         map_depth => 3,
         term_depth => 50,
         colored => true
        }}}
    }
  ]}
 ]}

Full configuration details available here: http://erlang.org/doc/apps/kernel/logger_chapter.html#configuration-examples

flatlog custom formatter documentation is available here: https://github.com/ferd/flatlog

aeden commented 4 years ago

I don't think it makes sense for us to force a dependency on flatlog in erldns. Can you please provide a change to https://github.com/dnsimple/erldns/blob/master/erldns.config.example as part of this PR that allows the logging to function without the dependency on flatlog.

m0rcq commented 4 years ago

I don't think it makes sense for us to force a dependency on flatlog in erldns. Can you please provide a change to https://github.com/dnsimple/erldns/blob/master/erldns.config.example as part of this PR that allows the logging to function without the dependency on flatlog.

@aeden - agreed, formatter should be picked at will, erldns.config.example amended with https://github.com/dnsimple/erldns/commit/2560808719c9a1c1e8111ac87d3e80616bde1f45

m0rcq commented 4 years ago

I suggest switching to direct logger:function_name calls rather than using the macro. That removes the need to include the lib and frankly I don't see us switching to another logger once we move to the OTP provided logger.

@aeden - roger that, will amend as required.

m0rcq commented 4 years ago

I suggest switching to direct logger:function_name calls rather than using the macro. That removes the need to include the lib and frankly I don't see us switching to another logger once we move to the OTP provided logger.

@aeden - on the subject: just reviewed the OTP logger documentation in regard to the log macros vs log functions - there's small side effect related to the metadata when using log macros vs. log event fun:

https://erlang.org/doc/man/logger.html#type-metadata

for the former, Logger is inserting the following information in metadata:

mfa => {?MODULE, ?FUNCTION_NAME, ?FUNCTION_ARITY}
file => ?FILE
line => ?LINE

which is not available in the latter. Using set_process_metadata/1 will allow to build custom metadata if we need the above information on log calls, but the macros are probably slightly more convenient.

Please let me know what do think on this.

aeden commented 4 years ago

If the logger library can automatically generate the metadata, I think we should take advantage of that fact to simplify what we have to include inline.

m0rcq commented 4 years ago

If the logger library can automatically generate the metadata, I think we should take advantage of that fact to simplify what we have to include inline.

Agreed, keeping the macros for the time being as it will streamline the metadata generation on fly.