Early review of std.logger

[Sönke Ludwig]

Am 14.10.2013 15:12, schrieb Robert Schadek:
> On 10/14/2013 02:39 PM, Sönke Ludwig wrote:
>>  - The static methods in LogManager should be made global and the class
>>    be removed. It's not for objects so it shouldn't be a class.
> LogManager also stores the global log level. Sure I can make another
> static global function storing this log level, but I would like to keep
> them together as they belong together IMO.

The same could be said about the global "log" functions, which are
tightly coupled to that state. I think this is already nicely grouped
together by the logger module itself, since there is not much else in it.

Basically, I just wouldn't consider this style to be particularly
idiomatic D code, but of course that's just personal
perception/preference (there is also some precedence using "struct"
instead of "class" in Druntime). However, if it ends up like this in the
final version, it should get a "@disable this();" to prevent misuse.

>>  - For me this logger is completely worthless without any debug log
>>    levels. The last std.log entry had at least anonymous verbosity
>>    levels, but I'd prefer something like I did in vibe.d [1], where
>>    each level has a defined role. This should especially improve the
>>    situation when multiple libraries are involved.
> Logger.log(LogLevel.(d|D)ebug, "Your message");

That would be my idea. Having at least two (diagnostic output for the
user and debug output for the developer), but better all four debug
levels can be very useful, though.

>>  - Similarly, I've never felt the need for conditional logging, but
>>    without it being lazy evaluated what's the use for it, actually?
> The conditional logging part is totally transparent.

But is there a compelling use case? It's transparent, but still
increases the size/complexity of the API, not much, but there should be
a reason for that IMO.

>>  - I really think there should be shortcuts for the different log
>>    levels. Typing out "LogLevel.xxx" each time looks tedious for
>>    something that is used in so many places.
> One could argue that writting logger.logDebug("...") is more tedious
> than writing,
> logger.logLevel = LogLevel.xxx;
> logger.log("...");
> logger.log("...");
> ...
> 
> This has been argued in the last logger discussion to some extend and it
> looked to me like this is the mostly preferred version.

The last discussion resulted (if I remember right) in something like
"log.info(...)". This would be fine, too, although I think just
"logInfo" is slightly preferable due to its length and the principle of
least surprise.

>>  - There should be some kind of MultiLogger so that multiple log
>>    destinations can be configured (e.g. console + file). Or, instead of
>>    a single default logger, there could be a list of loggers.
> there is one default logger. I will create a MultiLogger, good point.
> I'm currently sure how to store the multi logger (LL or Array or ... )

I'd say a dynamic array is fine because the list of loggers will rarely
change and this would be most efficient for iteration.

>>  - On the same topic, if I'm right and the default logger is stored as
>>    __gshared, it should be documented that Loggers need to be
>>    thread-safe.
> It is not stored __gshared, but If, you're right.

So the defaultLogger is per-thread? That may result in unexpected log
output if you just do simple code like "spawn({ log("Hello, World!");
});" and have a custom logger set up during application initialization.
Anyway, let's just say the threading behavior in general should be
clearly documented here as this is critical for both log users and
Logger implementors.

>>  - GC allocations for each log message _must_ be avoided at all costs.
>>    Using formattedWrite() instead of format() on either a temporary
>>    buffer that is reused, or, better, on some kind of output range
>>    interface of the Logger would solve this.
> This was proposed in the last thread. A fixed size buffer would scream
> bufferoverflow, a dynamic buffer not but both would raise the question
> of thread safety. 

Something like a thread local buffer that grows when needed, or small
fixed buffer + a scoped heap allocation for large messages should be
fine. Using an output range interface could of course avoid buffering
each message altogether. That would just open up the question of a nice
API design for such.

One last thing just occurred to me, in the default logger in vibe.d I've
made it so (thanks to Jordi Sayol for requesting this) that the "info"
level gets output to stdout and all other levels to stderr. Either that
or always outputting to stderr would be conforming to the idea of
stdout/stderr and would avoid that some library with logging calls
interferes with the output of an application (when piping process output
in a shell script).

Thanks for bringing this forward!