Review: std.logger

[linkrope via Digitalmars-d]

Sorry, but at the first contact with the implementation (the one 
with 'genDocComment' and 'buildLogFunction' from months ago) I 
was scared away. (It's better now.)

I feared that if I criticize the 'mixin' sequences you would ask 
something like "suggestions?" ;-)
So, I made this experiment to provide an answer. Now, I can 
suggest: try something like this:

     alias trace = log!(LogLevel.trace);
     alias info = log!(LogLevel.info);
     alias warn = log!(LogLevel.warn);
     alias error = log!(LogLevel.error);
     alias fatal = log!(LogLevel.fatal);

(see https://github.com/linkrope/log/blob/master/src/log.d)

Controversial conditional logging
----------------------------
The only advantage of
     tracec(condition, "passed");
over
     if (condition) trace("passed");
would be, that a costly evaluation of the condition is omitted 
when there is no trace logger.
That's why the std.log proposal had 'when(lazy bool now)'.

First, I was puzzled about your argument that LOG_FIRST_N or 
LOG_EVERY_N would be no problem with the '...c' functions. But a 
look at the implementation confirmed that the std.logger has no 
lazy evaluation of the condition; discarding the only advantage.

Sets of log levels
--------------
No!
Of course, I can log (trace | info) to stdout, warn.orHigher to 
stderr, and for instance info.orHigher to some file.

Simplicity
--------
"The simplest way to achieve simplicity is through thoughtful 
reduction."

We started with tango.util.log (best described as log4j for D). 
We are happier now with a lot less functionality, but on the 
other hand with the simplest API that works.

On Monday, 21 July 2014 at 22:53:27 UTC, Robert burner Schadek 
wrote:
> On Sunday, 20 July 2014 at 16:15:53 UTC, linkrope wrote:
>> Pros
>> ----
>> The lighning talk about the std.logger proposal at DConf 2014 
>> had a positive impact.
>> We were able to change the "Current D Use" entry of our 
>> company from
>>    "Uses D2 / Phobos, Tango (log, xml)"
>> to
>>    "Uses D2 / Phobos, Tango (xml)".
>> (We got rid of tango.util.log; we still rely on the fast 
>> tango.text.xml.)
>
> I didn't expect to hear from you about this, after you did not 
> reply to my email about this topic.
>
> If xml is problem for you, where is the PR?
>
>>
>> Cons
>> ----
>> 1. I am not happy with the (reverse) hungarian-style naming
>>
>> At least in the code of our company, logging a formatted 
>> string is the basic use case.
>> The function for the basic use case should not require a 
>> suffix letter.
>> The consistency argument, that 'infof' is like 'writef', does 
>> not fully apply:
>> neither 'infoln' nor 'infofln' make sense.
>> (In my opinion, "half consistent" is inconsistent.)
>
> so we have gone full circle now ....
>
>>
>> Currently, suffix 'c' is used for conditional logging.
>> But, how will future extensions like glog's LOG_EVERY_N or 
>> LOG_FIRST_N be named?
>
> That's an easy one.
>
> ```
> auto a = LOG_FIRST_N(1337);
>
> logc(a, "Hello world");
>
> auto b = WHAT_EVERY_THE(....);
> logc(b, "Hello world again");
> ```
>
>> With suffix 'e'? Suffix 'f' is already assigned!
>
> what is 'e'?
>
>> The suffix letter sequence seems to be the road to confusion.
>>
>> I would prefer the explicit naming of the previous std.log 
>> proposal:
>>    log.when(condition)(...)
>> However, there is only a small advantage over
>>    if (condition) log(...)
>
> ...
>
>>
>> 2. No support for 24/7 (server) applications
>>
>> In my opinion, I really need logging for applications that 
>> possibly run forever.
>> With the FileLogger, the file will grow forever.
>> That's why most other frameworks provide something like a 
>> RollingFileLogger or some "logrotate-aware" FileLogger.
>>
>> By the way: Instead of what I really need, I get a NullLogger.
>> I have no clue, why I never ever missed such an oddity.
>
> That was a user request, through github. Where I asked you to 
> submit PRs and issues.
>
> Have you tried subclassing Logger? I asked for PRs in the email 
> I wrote to you at least twice.
>
>>
>> 3. Implementation is hidden behind 'mixin' expressions
>>
>> When I tried to look at the implementation, I found long 
>> sequences of lines like this:
>>    mixin(buildLogFunction(true, false, false, LogLevel.info));
>>
>> Nowadays, this changed into:
>>    mixin(freeLog.format(
>>        "info", "info", "info", "info",
>>        "info", "info", "info", "info",
>>        "info", "info", "info", "info",
>>        "info", "info", "info", "info"));
>>
>> This is much better, but I still think, it's a complicated 
>> solution for a simple problem.
>> And it would be a shame for D, if there is no simple solution.
>
> Yes please, suggestions?
>
>>
>> Small stuff
>> -----------
>> 4. FileLogger needs flush
>>
>> It's annoying when the events that caused a crash have been 
>> logged, but they never have been written to the file.
>
> I will fix that in the next session.
>
>>
>> 5. Suspect use of '__gshared'
>>
>> The FileLogger has a field
>>    private __gshared File file_;
>>
>> In this case, "__gshared is equivalent to static".
>> This means that all FileLogger instances share the same file!
>
> I missed that, thank you
>
>>
>> 6. Bad naming of "StdIOLogger"
>>
>> Like 'std.stdio.StdioException', the 'io' should be lower case.
>> If the 'StdIOLogger' logs to 'stdout', 'StdoutLogger' would be 
>> preferable.
>
> easy fix
>
>>
>> 7. No need for StdIOLogger
>>
>> 'stdout' (and 'stderr') are Files, so a FileLogger should be 
>> able to handle them.
>> A second constructor should do the trick.
>
> It is a special file, I wanted to have that clear. two 
> different classes does the trick for me.
>
>>
>> 8. Log levels
>>
>> Many frameworks mix the types "log level" and "set of log 
>> levels" (for filtering).
>> While 'trace', ..., 'fatal' are log levels, 'all' and 'off' 
>> (better: 'none'?) are sets of log levels.
>> (I have no idea about the type of 'unspecific'.)
>>
>> A clean separation would avoid confusion:
>> why is there 'info(...)' but not 'all(...)'?
>
> unspecific is about to be removed, all and off are pretty easy 
> to understand but than ....
>
>>
>> Also, it would be easier to log for example 'trace' and 'info' 
>> to 'stdout'.
>
> not if you want to have that logged somewhere else.
>
>>
>> 9. Bad naming of "std.logger"
>>
>> The focus of this proposal is on the log/logging API; the 
>> loggers are only examples.
>>
>> The recommended use should be
>>    import log = std.logger;
>
> you got that wrong, you can do it like that, nobody will force 
> you and properly people will do it different.
> You can also create a module wide global logger and use that
>
> Again, std.logger is not the solution that works for every 
> special case anybody comes up with out of the box. It is a set 
> of ideas that enable you to have the logging tailored to your 
> needs easily. On top of that, it allows you very fast access to 
> basic logging that can be extend later on easily and seamlessly.
>
>>
>> Then, the name "std.log" (of the previous proposal) would be 
>> more appropriate.
>>
>> Counter Proposal
>> ----------------
>> As a consequence of these issues, I once decided to spend a 
>> weekend to prepare a counter proposal:
>>
>> http://code.dlang.org/packages/log
>>
>> The design goal was simplicity. So:
>> - conditional logging is not supported
>> - no suffix letter sequences
>
> So you deleted code and functionality, hm?
>
>> - there is no NullLogger
>
> Same point
>
>> - there is no MultiLogger (this functionality is implicit)
>
> It is not, you can't remove Loggers individual and you can't 
> build trees.
>
>> - there is no need to provide a name for a logger
>
> Because, you have no MultiLogger
>
>>
>> I prefer 'alias' over 'mixin':
>> 'info' is just an alias for 'log(arg)' as well as for 
>> 'log(fmt, args)' at log level 'info'.
>>
>> Sets of log levels are implemented as (bit) sets of log levels.
>> A helper function lets you select the traditional >= filtering:
>>    LogLevel.info.orHigher
>
> So I need nine LogLevels, how do I add one between info and 
> warn?
>
>>
>> For convenience, 'stdoutLogger' and 'stderrLogger' are factory 
>> functions that create 'FileLogger' instances.
>
> Object.factory
>
>>
>> Of course, a RollingFileLogger as well a a 
>> "RotatingFileLogger" (that reopens the log file on SIGHUP) are 
>> provided.
>
> Subclassing Logger should get the job done in under 30 lines.
>
>>
>> By now, this simple solution is in use in tens of thousands 
>> lines of commercial code.
>> (Where it outperforms the previously used tango.util.log 
>> implementation.)
>
> So what are the numbers for std.logger and where is the 
> benchmark file to test it?
>
> From the source it looks like you kept parts of the design of 
> std.logger and pulled out everything you didn't agree with. 
> Also you only have global logging, to cite Brian: "Why?"
>
> You added the parts, that you described are missing in 
> std.logger. Of course your counter proposal will meet your 
> needs then. I mean what would be the point of coding it up 
> anyway else?