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

Lager dependency unfriendly #57

Open
eproxus opened this issue Jan 7, 2016 · 32 comments
Open

Lager dependency unfriendly #57

eproxus opened this issue Jan 7, 2016 · 32 comments
Assignees

Comments

@eproxus
Copy link

eproxus commented Jan 7, 2016

In my opinion, including Lager is outside the scope of Exometer. We're trying to include Exometer in an existing Elixir application which uses the Elixir's Logger. Getting Lager included just because of Exometer adds a lot of overhead and makes configuring logging much more complex (we either have to disable Lager or make it somehow send messages to the Elixir logging framework).

I see three possible solutions:

  1. Exclude logging completely from Exometer
  2. Use error_logger (which is picked up by Lager and Elixir's Logger anyway)
  3. Make logging/events inside Exometer be handled by a user defined callback module
@tolbrino
Copy link
Contributor

tolbrino commented Jan 7, 2016

I ran into the same situation with an Elixir project. Obviously this is an issue for any Erlang/Elixir library.

Since exometer doesn't use much of lager anyway and historally has only done so since it was integrated in Riak early on, I'd be fine with generalizing logging. @uwiger what do you think?

@uwiger
Copy link
Member

uwiger commented Jan 7, 2016

@tolbrino I'm fine with it, but I'd also like to hear from Basho. After all, Exometer is their product and this is a fairly significant change.

@tolbrino
Copy link
Contributor

tolbrino commented Jan 7, 2016

@uwiger Good point.

@jonmeredith
Copy link

@eproxus Ulf pinged me and we had a quick conversation. I thought your error_logger idea sounded best, until Ulf pointed out the lack of debug logging support in SASL....

I suppose there's a 4th option of trying to make it a compile time choice and switch between lager calls or the Elixir Logger.

I still haven't played with Elixir much - how are other libraries handling this?

@tolbrino
Copy link
Contributor

tolbrino commented Jan 8, 2016

@jonmeredith Exometer is more involved than a normal library, an application really. Lots of Elixir libraries simply default to the Elixir logger. Since exometer already includes the log.hrl extending that to support switching the logging library would be easy.

Alternatively, exometer doesn't use lager debug a lot anyway. In my use cases I always found that I wanted to debug at the interfaces to exometer from my applications, which is fine since an umbrella application can always choose to use lager. This way one can also easily trace inside of exometer if needed.

Also we've got the lager reporter which could be used to pipe whatever data is needed into lager on a debug level easily.

@eproxus
Copy link
Author

eproxus commented Jan 8, 2016

In my opinion, a library that is not application specific, should never force me into a certain logging paradigm.

If you really need the introspection into Exometer (that the error_logger cannot provide), I would then suggest the 3rd option, possible even together with a bundled Lager event callback module (where the dependency on Lager would be removed still). Then people who want to use Lager can just configure that callback module to be used.

@uwiger
Copy link
Member

uwiger commented Jan 8, 2016

In the locks application, instead of using a logger for debugging, I simply use an ?event() macro (example), which ends up calling an empty function. When I want to activate debugging, I use TTB to trace on the event functions (see locks_ttb). The place where I use this the most is in the more complicated test suite (example)

The following screenshot shows what the pretty-printed trace log from a multi-node trace looks like in emacs:
screenshot 2016-01-08 12 27 00

@uwiger
Copy link
Member

uwiger commented Jan 8, 2016

One of the nice things about lager:debug() is that it won't process the input arguments unless logging is enabled at that level. This means that one can massage the debug output without incurring runtime overhead with debugging disabled.

It's not an issue in exometer in the sense that it doesn't currently massage debug output arguments. However, it does pretty-print, and using the ?event-style debugging as above tends to produce slightly messier output.

FWIW, in the latest push to locks, I introduced a ?costly_event macro, which only calls the event function if that function is being traced. I think this increases the usefulness of the technique somewhat.

@eproxus
Copy link
Author

eproxus commented Jan 11, 2016

Well, if a couple of calls to an empty callback module (which would be the case for option 3) is too much overhead, I would instead suggest a debug macro in Exometer that will compile in all the Lager logging code. That way, we can remove the dependency on Lager, and people who use this macro would have to make sure Lager is present when running and compiling.

@uwiger
Copy link
Member

uwiger commented Jan 11, 2016

To be clear, 'disabled' lager debug macros are not zero overhead either.

Specifically, the generated check for a lager:debug() is

case {whereis(lager_event), whereis(lager_event), lager_config:get({lager_event,loglevel}, {0,[]})} of
    {undefined,undefined,_} -> fun() -> {error,lager_not_running} end();
    {undefined,_,_} ->               fun() -> {error,{sink_not_configured,lager_event}} end();
    {__Pidexometer_report1160, _, {__Levelexometer_report1160, __Tracesexometer_report1160}}
        when __Levelexometer_report1160 band 128 /= 0 orelse  __Tracesexometer_report1160 /= [] ->
        lager:do_log(debug, [{application,exometer_core} | ...], ...);
    _ ->
        ok
end,

where lager_config:get/2 performs an ets:lookup/2.

@eproxus
Copy link
Author

eproxus commented Jan 11, 2016

Oh, then a callback module with a no-op function should be much faster? So win-win?

@jonmeredith
Copy link

The problem with a callback module is you would lose the line number reporting that you get from the parse transform - all error reports would come from exometer_log:info:1234 rather than the original call site.

Converting all the lager code to compile time macros seems like the best option (and checking it doesn't have any similar problems - I'm fairly certain the preprocessor gets to run before the parse transform).

@eproxus
Copy link
Author

eproxus commented Jan 11, 2016

Good point with the line numbers.

The only problem I see is that when Lager is not a dependency of the project, you might have problems including the Lager header file when that compile time macro is enabled.

@jonmeredith
Copy link

Hmmm, maybe make rebar.config.script add a preprocessor define around which
code files it can see? Check for lager in the path and define LOG_LAGER,
check for the Elixir logger and define something else?

On Mon, Jan 11, 2016 at 7:52 AM Adam Lindberg notifications@github.com
wrote:

Good point with the line numbers.

The only problem I see is that when Lager is not a dependency of the
project, you might have problems including the Lager header file when that
compile time macro is enabled.


Reply to this email directly or view it on GitHub
#57 (comment)
.

@tolbrino
Copy link
Contributor

Could you guys take a look at https://github.com/tolbrino/hut and let me know what you think? I tested it locally with exometer_core and it works fine.

@eproxus
Copy link
Author

eproxus commented Jan 18, 2016

For me, Hut looks good. How would an optional dependency be specified in a project using it (e.g. Exometer and Lager)?

@tolbrino
Copy link
Contributor

Exometer would include Hut as its dependency. Your umbrella project would include Exometer and Lager as its dependency. Then, depending on your build tool, you'd ensure that Exometer gets compiled with the macro HUT_LAGER.

@eproxus
Copy link
Author

eproxus commented Jan 18, 2016

Sounds reasonable to me. I think that no-op or SASL logging should be the default though. Having ton's of io:format's everywhere would not be nice.

@tolbrino
Copy link
Contributor

I thought of switching to SASL for the default too.

@jonmeredith
Copy link

Hey Tino - @mrallen1 took a look at hut and thinks that it will work fine integrating with Riak, so feel free to go ahead and convert if you would like.

I'd also vote for SASL as it's part of the standard runtime and should always be present - that seems like a much better compromise between nothing at all and a spew of io:formats.

@tolbrino
Copy link
Contributor

@jonmeredith Thanks for the feedback. I'll do the conversion then and adapt the default.

@tolbrino tolbrino self-assigned this Jan 23, 2016
@eproxus
Copy link
Author

eproxus commented Jan 25, 2016

@tolbrino Looking forward to trying it out. :-) Thanks for putting in the effort.

@tolbrino
Copy link
Contributor

@eproxus The PR replacing lager has been merged. I feel like we can close this discussion.

@surik
Copy link
Contributor

surik commented Feb 26, 2016

I really don't understand how I can use exometer_core with hut in Elixir application.
For example I have an application which uses exometer_core from master:

defmodule App.Mixfile do
  use Mix.Project

  def project do
    [app: :app,
     version: "1.0.0",
     deps: deps(Mix.env)]
  end

  def application do
    [applications: [:exometer_core],
     mod: {App, []}]
  end

  defp deps(_) do
    [{:exometer_core, github: "Fuerlabs/exometer_core"}]
  end
end

How I can build it with using Logger?
I have seen on example in hut but it looks redundantly to create Makefiles instead using mix or rebar.

@tolbrino
Copy link
Contributor

What exactly is your intention? Obviously you can use Elixir.Logger in your app as you please. If you are talking about using it in exometer_core, that is not supported by hut as of now. Feel free to post a PR. However, Elixir.Logger already intercepts SASL logging, so even the standard config for hut should be ok. Btw, there is a type in Feuerlabs in your deps.

@surik
Copy link
Contributor

surik commented Feb 26, 2016

@tolbrino ok, I see that Elixir.Logger is not supported right now.

My question is not about Logger. My question is how I can change default logging system for exometer_core in my Elixir application?
Lets imagine I want to use lager. How I can say to mix to compile exometer_core with -DHUT_LAGER option? In your example I see you use Makefile. Is it preferable way?

@tolbrino
Copy link
Contributor

When using rebar you can set these preprocessor macros using the erl_opts property, see https://github.com/rebar/rebar/blob/master/rebar.config.sample

@surik
Copy link
Contributor

surik commented Feb 26, 2016

@tolbrino I'm not sure but looks erl_opts will be used only for current application and not for dependencies. That means exometer_core will use default logging system.

@tolbrino
Copy link
Contributor

Ok, now I get your point. I'll look into and dig up an example.

@surik
Copy link
Contributor

surik commented Jul 13, 2016

Hi @tolbrino, any results?

@mootpointer
Copy link

Wouldn't a seperate hut_lager package which integrates hut with lager make more sense than using preprocessor macros?

@surik
Copy link
Contributor

surik commented Aug 6, 2016

@mootpointer how separated hut_lager can help me build exometer_core with lager?

Maybe better to use sasl directly without additional dependency which is difficult to use?

ldr added a commit to ldr/netlink that referenced this issue Jul 1, 2017
Made same changes as were done for feuerlabs/exometer_core

See related discussions:

* Feuerlabs/exometer_core#57
* Feuerlabs/exometer_core#66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants