nitrogen_core icon indicating copy to clipboard operation
nitrogen_core copied to clipboard

Logging macro incompatibility (OTP >= v21)

Open bunnylushington opened this issue 4 years ago • 2 comments

An issue arises if the new(-ish) logging macros (kernel/include/logger.hrl) are included alongside wf.hrl, specifically, ?LOG is redefined. I cannot see a 100% backwards compatible solution to this problem as logger:?LOG accepts different arguments than wf:?LOG. (It's entirely possible my understanding of how includes works is faulty or incomplete.)

If this is something that should be addressed in Nitrogen, I'd propose a solution like

%%% LOGGING %%%
-ifndef(debug_print).
-define(debug_print, true).

-ifndef(LOGGER_HRL).
-define(PRINT(Var), error_logger:info_msg("DEBUG: ~p~n~p:~p~n~p~n  ~p~n", [self(), ?MODULE, ?LINE, ??Var, Var])).
-define(LOG(Msg, Args), error_logger:info_msg(Msg, Args)).
-define(DEBUG, error_logger:info_msg("DEBUG: ~p:~p~n", [?MODULE, ?LINE])).

-else.
%% logger.hrl has been included
%% ?LOG has already been defined
-define(WFLOG(Msg, Args), ?LOG_INFO(Msg, Args)).
-define(PRINT(Var), ?LOG_INFO("DEBUG: ~p: ~p~n", [??Var, Var])).
-define(DEBUG, ?LOG_INFO("DEBUG: ~p:~p~n", [?MODULE, ?LINE])).
-endif.

-endif.

which allows for both hrl files to be included, in this order:

-include_lib("kernel/include/logger.hrl").
-include_lib("nitrogen_core/include/wf.hrl").

This is an incomplete (and not well tested) solution! wf:info/1,2; wf:warning/1,2; and wf:error/1,2 will continue to use the legacy logging infrastructure. Further, the alternate macros do not take advantage of the enhanced metadata the new logging facility provides (which would require, I think, a custom log formatter).

I'm happy to submit a PR addressing this issue, either along these lines or a better solution arising from discussion. Before continuing however, I think it's important to acknowledge the compatibility issue that will arise and determine how it should be managed. It seems to be worth noting that the incompatibility is triggered by the developer opting to include the new macros; the default logging should just work in the vanilla case.

bunnylushington avatar Jan 08 '21 16:01 bunnylushington

Hey @bunnylushington, your proposed solution is good.

For the sake of consistency, I'd go with ?WF_LOG only because the other ?WF macro (?WF_IF) uses ?WF_ as the prefix instead of ?WF (without the underscore).

In this situation, I'm not super concerned with backwards compatibility being a problem, since it's a pretty easy s/LOG/WF_LOG kind of solution.

choptastic avatar Feb 04 '21 02:02 choptastic

Hola @bunnylushington and @choptastic, is anyone working on this one? We are migrating some production pages to a new system built on Erlang and Nitrogen and just discovered this conflict. It would be really helpful to be able to use the OTP logger macros without having to refactor all the Nitrogen stuff into a separate module. I'd be happy to try my hand at a PR if that would help get the ball rolling, though I'm admittedly not sure how to address the deeper questions about wf:info et al using the legacy logging setup.

codebykat avatar Aug 12 '21 21:08 codebykat