Allow log_root on crash_log

[paulo-ferraz-oliveira]

Since crash.log is assumed defined, by default (otherwise it has to be defined as false or undefined explicity - something I don't usually do), if I define log_root and not crash_log, I get crash_log as "<myfolder>/log/crash.log".

I'd prefer to have crash.log directly in <myfolder>. The proposed solution aims for backward compatibility.

Edit: I guess the same could be applied to the DEFAULT_HANDLER_CONF, though not setting handlers is probably less common than not setting crash_log.

[lukebakken]

Is this related to #535 at all?

[paulo-ferraz-oliveira]

I ended up extending this tweak to the default logs: 5a9bebb.

[paulo-ferraz-oliveira]

Is this related to #535 at all?

Good question. I don't know, let me check.

[paulo-ferraz-oliveira]

~Hm... yeah, it seems to be related, and #535 seems more elegant.~ Related, but not solving the same issue.

[lukebakken]

Thank you for checking. If you can confirm #535 accomplishes what you'd like, we would appreciate it!

[paulo-ferraz-oliveira]

I tested and it doesn't solve my use case :(.

If my sys.config is

[
    {lager, [{log_root, "logs"}]}
].

I still get logs/log/crash.log when I define {log_root, "logs"}.

[paulo-ferraz-oliveira]

This seems to happen because log is hardcoded in three places (not made optional), so only works when both file and log root used the same base folder, I think.

[yidayoung]

Seems those change could make some compatibility problem, befor this change with env

{lager, [
    {log_root, "/all_system_log"}
]}

crash.log should in /all_system_log/log/crash.log

with this change crash.log will in /all_system_log/crash.log same with error.log and console.log if just define log_root before

this example is not usual, but if some one use like this, and add some script to contol log, this update may cause his script error

as you said, just overwite crash_log with "crash.log" and define handlers also could solve your problem

535 just to resovle problem when some on call c:cd change current path, log file path will also change if log_root is relative path or not defined, indeed can't resolve your problem

[paulo-ferraz-oliveira]

Hey, @yidayoung, thank you for your participation. I'll try to further reason on my proposed changes.

Seems those change could make some compatibility problem,

I understand software evolves. Even if this could be a "breaking" change (you state "this example is not usual" and I agree), there's ways to signal it (e.g. a migration guide, a major version change, etc.). I think that semantics, and "hidden" elements, play an important role, here.

For semantics, I would expect the log_root to actually be the log root (a folder), which it apparently is. For "hidden" elements, I wouldn't expect the crash log to exist inside assumed folder log, which is why I mostly prefer to have no default options in my Erlang projects (i.e. I didn't set a crash log option, so no crash log should exist in a folder I didn't ask for - if this was the case this pull request wouldn't be required).

In any case, I opened the pull request expecting further discussion, more reflexion on the subject, the participation of the maintainers, ..., and a possible outcome: we either close it or move forward with it.

if some one use like this, and add some script to contol log, this update may cause his script error

Sure, but I don't think it's up to lager (or its maintainers) to try and account for all these flows (is it?). There is a way to move forward, which would be to adapt this pull request, and there is a way to keep behaviour (which is to tweak error.log, crash.log and console.log - the same way you state I could tweak my existing deployment, I believe people who upgrade to the new lager version, if this pull request gets accepted, could tweak their deployment).

I find it weird (though possible) that somebody controlling logs with scripts would not care to set either crash.log, console.log, error.log and/or log_root, since in that case it's strange to assume changes won't break your existing deployment (and if that's the case, accepting this pull request should be innocuous).

[yidayoung]

e.g. a migration guide, a major version change, etc. is good enough . my point is this too, not disagree your pr

log file should be in log_root dir directly, i have been confused by this before too, after saw app.src then got should overwrite crash_log, I'm sure we're not the only ones.

a little unnecessary code here https://github.com/paulo-ferraz-oliveira/lager/blob/5a9bebba8c383271d7ac95cb65d8f3b7f80924cc/src/lager_sup.erl#L82-L85

because we have default log_root now and crash log file real path will expend here https://github.com/erlang-lager/lager/blob/91575d4479c897451edf138c10fa86c3125ec52e/src/lager_crash_log.erl#L75-L76

just advise

---

our pr dosn't conflict, just code what you want

[paulo-ferraz-oliveira]

a little unnecessary code here

You mean I can take it out completely? I'll do that, thanks.

Edit: 3811baa.

[paulo-ferraz-oliveira]

Ping. @mrallen1, does this have the potential to move forward?

[jadeallenx]

Definitely - I have just been off work for several weeks dealing with a lot of personal issues. I'll try to take a look at this some time today. Sorry for the delays.

[paulo-ferraz-oliveira]

No worries, @mrallen1. I just though this had gone "under the radar". Wait I shall.

[lukebakken]

It's on my list of things to review and test soon as well.

[paulo-ferraz-oliveira]

Thanks, all.