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

Remove lager dependency, use native OPT21+ logger. #50

Closed
wants to merge 1 commit into from

Conversation

csrl
Copy link
Contributor

@csrl csrl commented Oct 15, 2021

Erlcass as a library is consumed by applications and does not stand on its own. As such it should not force using additional logging facilities that the application does not use. The lager dependency brings in multiple sub dependencies as well. OTP 21+ provide reasonable logging facilities sufficient for the use in Erlcass.

This PR removes lager and it's dependencies. The code change is a matter of swapping lager for logger in include/erlcass_internals.hrl

@silviucpp
Copy link
Owner

Hello,

Unfortunately we are using this library in OTP versions older than 21.

Once we will update our systems we will take this into account. But please note that something is broken with your PR the builds are failing because of a syntax error in rebar.lock I think.

Silviu

@csrl
Copy link
Contributor Author

csrl commented Oct 15, 2021

Thanks for looking.

I assumed it was a good time to introduce this change since the recent "Fix OTP 24 build" commit removed otp 19/20/21 targets from travis. Should those be restored then?

In any case I've fixed the syntax error the rebar.lock file. I suppose rebar.lock could just be removed as well since there are no longer any dependencies.

Feel free to close the PR if it won't be pulled in.

@silviucpp
Copy link
Owner

silviucpp commented Oct 16, 2021

Hello,

We removed the OTP 19/20/21 from travis because are not longer supported there out of the box. Idea is that internally we are using this library in OTP 19 as well and also the main issue is the fact that we need to be able to send the logs in graylog. And we are using a module for lager which is doing this. Lager seems is not able to catch the logs from OTP21 logger ..

Other guys asked for another library we have as well to get rid of lager silviucpp/erlkaf#9 (comment)

@csrl
Copy link
Contributor Author

csrl commented Oct 16, 2021

Would it work for you if the PR was changed to use a configuration parameter for picking 'logger' vs 'lager'? And would it be ok if lager was still removed from the dependencies such that the top level application must set it as a dependency instead when configured for using lager?

@silviucpp
Copy link
Owner

@csrl Are you talking about an ENV variable or can you elaborate on how you want to achieve this ? If it's an env variable I think you can also use rebar3 dynamic config to also add lagger as dependency in case it's used.

@csrl
Copy link
Contributor Author

csrl commented Oct 16, 2021

I was thinking as an config field, for example:

{erlcass, [{log_facility, <<"lager">>}, {log_level, 3}, {cluster_options, [...]}]}

@csrl
Copy link
Contributor Author

csrl commented Oct 16, 2021

To be clear - that approach would require making erlcass_log.erl a named process and update the minimal use of ERROR/INFO_MSG macros in erlcass.erl to send the log messages to erlcass_log.

@silviucpp
Copy link
Owner

A better idea might be to use a custom define like:

-ifdef(USE_LAGER).
  %% Log macros will use lager
-else.
  %% Log macros will use logger
-endif.

And somehow to define that macro from rebar3 . maybe different profiles to build with logger or lager ?

@silviucpp
Copy link
Owner

I implemented this in master and I will publish a package today.

@silviucpp silviucpp closed this Jan 18, 2023
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

Successfully merging this pull request may close these issues.

2 participants