erlyaws / yaws

Yaws webserver
https://erlyaws.github.io
BSD 3-Clause "New" or "Revised" License
1.28k stars 267 forks source link

Yaws assumes `error_logger` process always exists and it is a gen_event #442

Closed seriyps closed 2 years ago

seriyps commented 2 years ago

I noticed that in yaws_log it is assumed that there is always a process registered as error_logger and this process is gen_event:

https://github.com/erlyaws/yaws/blob/866714d3de230ba73ab7455aa9a9d70079429a87/src/yaws_log.erl#L363-L368

https://github.com/erlyaws/yaws/blob/866714d3de230ba73ab7455aa9a9d70079429a87/src/yaws_log.erl#L398

But with OTP-21 logger one can have just a custom OTP logger handler registered as error_logger which would capture all the error_logger:*_msg error_logger:*_report logs without ever starting any processes with error_logger name.

I think it might make sense to, maybe, add some config that tells Yaws to not try to manage the error_logger?

vinoski commented 2 years ago

In general, Yaws logging needs work, as it's never been converted over to logger. With the recent Yaws 2.1.0 release, the oldest Erlang/OTP release we now support is 21.0, which is when logger was introduced, so Yaws should now just drop older logging support and convert fully to logger.

seriyps commented 2 years ago

Thanks. I can try to have a look and maybe send some pull-request to migrate to OTP logger.

What degree of backwards-compatibility should we have? May you point me if there is some documentation describing public logging-related APIs which we should not break?

seriyps commented 2 years ago

I guess might be nice to keep yaws_logger behaviour, but that may require a special logger_handler for "virtual servers" which requested a custom logger_mod.

Maybe install individual logger handler for each enabled auth_log and access_log.

Some docs I found: https://github.com/erlyaws/yaws/blob/866714d3de230ba73ab7455aa9a9d70079429a87/doc/yaws.tex#L2005-L2023

http://yaws.hyber.org/logger_mod.yaws

https://github.com/erlyaws/yaws/blob/866714d3de230ba73ab7455aa9a9d70079429a87/doc/yaws.tex#L2801-L2873

seriyps commented 2 years ago

Actually, it seems the better plan would be:

vinoski commented 2 years ago

Yep, that sounds right.

seriyps commented 2 years ago

Ok, pushed a bit different implementation from what I planned, see #443. Still, fully backwards-compatible.

seriyps commented 2 years ago

Fixed by #445