-
Notifications
You must be signed in to change notification settings - Fork 456
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
Enable lager to use the OTP logger's event pipeline #524
base: master
Are you sure you want to change the base?
Conversation
Some things we need to do:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. I haven't investigated the failing tests yet but I suspect they're just due to the WIP nature of these changes.
src/lager.erl
Outdated
Formatter = proplists:get_value(formatter, Config, lager_default_formatter), | ||
FormatterConfig = proplists:get_value(formatter_config, Config, []), | ||
%% XXX I don't think disk_log is suitable as it is, we should provide a logger compatible version of | ||
%% lager's file_backend and try to patch disk_log upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
false -> | ||
%% set up the config, is safe even during relups | ||
lager_config:new(), | ||
%% TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: probably should delete or update this comment
do_format(Level, Msg, Metadata, Config) -> | ||
FormatModule = maps:get(formatter, Config, lager_default_formatter), | ||
Timestamp = maps:get(time, Metadata), | ||
MegaSecs = Timestamp div 1000000000000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these numbers macros to improve readability and maintenance sanity, please.
{application, Name} = lists:keyfind(application, 1, Report), | ||
Formatted = error_logger_lager_h:format_reason(Reason), | ||
{"Application ~w exited with reason: ~s", [Name, Formatted]} | ||
end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail on custom reports, ex.:
?LOG_INFO(#{http_reponse => 200, request_duration => Duration})
@@ -689,6 +690,64 @@ rotate_handler(Handler) -> | |||
rotate_handler(Handler, Sink) -> | |||
gen_event:call(Sink, Handler, rotate, ?ROTATE_TIMEOUT). | |||
|
|||
generate_logger_config() -> | |||
Handlers = application:get_env(lager, handlers, lager_app:default_handlers()), | |||
{Level, NewHandlers} = generate_logger_handlers(Handlers, {notice, []}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of "translating" Lager handlers (which will not support custom handlers anyway) to logger handlers, add new handler that will forward all messages from the logger to lager's gen_event
as a compatibility layer.
{lager_logger_formatter, #{report_cb => fun lager_logger_formatter:report_cb/1, | ||
formatter => Formatter, | ||
formatter_config => FormatterConfig}}}}, | ||
NewLevel = case lager_util:level_to_num(Level) > lager_util:level_to_num(CurrentLevel) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of that you can use logger:compare_levels/2
.
We should really get this finished so let's try to figure out what's missing.