Dt/logging

[doubt72]

Dep locking + logging changes

Long overdue description:

Rebar is touchy about having a parse transform in the erl_opts in rebar.config; because of that, the parse transform had to be pulled out of the rebar.config and be replace with -compile directive in any modules that need to call a module that needs it (i.e., lager).

Since lager is called from (seemingly) every module (not actually true, but there are a lot), that left me with two choices: add "-compile([{parse_transform, lager_tranform}])." to every module that calls lager, or pull the lager calls into another module that wraps the lager calls, and call that module from elsewhere.

Given that this would allow future refactoring of logging to be a bit easier, I went with the latter option. I'm willing to be convinced that going the other way may have been better (and changing things isn't that hard), but overall the refactor seemed a better option, so that's what I did.

[langloisjp]

LGTM!

I'm not seeing why you needed to refactor though. The commit message says 'so that locked deps actually works' but can you elaborate? Comments in the code about it would be great!

[doubt72]

@langloisjp ...Added comments about why. Discussed this briefly at standup this morning, but didn't get back here to document it better until now.

[langloisjp]

Somehow not seeing the comment you refer too... Where should I look?

[doubt72]

PR description -- do you need to refresh the page, maybe?

[langloisjp]

LOL I was looking in the code :) You definitely did the right thing. Would be great to add these comments to pushy_logger.erl right before the -compile.

[christophermaier]

I believe one consequence of delegating to another module for logging is that all the contextual call site information for a log statement (e.g., the line number, module name, etc) is all going to refer to that delegated module, rather than the "true" call site of the log statement.

This could hamper debugging efforts, since all messages are going to be coming from pushy_logger (provided, of course, that the lager output formatter is configured to print this information, that is). In Bifrost, we don't output this information, since we currently only log from one place (the webmachine logging module), so that information is noise anyway. Here, though, it seems like it might provide legitimate value.

However, if all the messages are distinct enough, and call site information doesn't really provide much, it would be good to not output the call site info to streamline the log files (comments in the code to this effect would be good, too :smiley:)

[langloisjp]

Thanks for the explanation! Makes sense now. I was wondering why lager needed the parse transform... Seems like we should try to fix this at the rebar level...

[doubt72]

Okay, after some discussion with CM, I've decided to roll back the new module and do per-module compiles instead; it does preserve slightly more information which we may or may not use when debugging, etc. It's possible to duplicate that behavior and/or suppress it if we didn't actually want it, but as it's of possible slight use... It might be possible to get rid of the parse_transforms altogether, but that would require additional investigation, and the point of this exercise in the first place was only to lock down the rebar dependencies and this is clearly scope creep.

So, going to do the simplest thing that preserves old behavior here, and we can revisit this later, especially if we decide to overhaul logging for some reason.

Anyway, still open for any additional comments on this.

[jkeiser]

I am not seeing further problems, and am super happy we are locking :) :+1: