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

Logging macro incompatibility (OTP >= v21) #136

Open
bunnylushington opened this issue Jan 8, 2021 · 2 comments
Open

Logging macro incompatibility (OTP >= v21) #136

bunnylushington opened this issue Jan 8, 2021 · 2 comments

Comments

@bunnylushington
Copy link
Contributor

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.

@choptastic
Copy link
Member

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.

@codebykat
Copy link
Contributor

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.

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

No branches or pull requests

3 participants