Early review of std.logger

[Robert Schadek]

On 10/14/2013 04:44 PM, Sönke Ludwig wrote:
> 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.
It is for ment for phobos not druntime. Anyway structs would mean all
templates and people will scream template bloat. And this would break
the design, which I find to be a valid use of classes and
polymorphisms.  The StdIOLogger can have its default constructor called IMO.
>
>>>  - 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.
Maybe I miscommunicated what I want to show by that example. The (d|D)
part is the rename to enum lower case.
The debug log level is given through the LogLevel.Debug, which will be
renamed to LogLevel.debug. I would call the developer the user of the
logger. Maybe log messages can be communicated to the user of the
applicaiton and the developer of the application through a MultiLogger
class.
>>>  - 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 had the case that a custom class made problems in some special case.
And IMO it was just prettier to have the condition if it should be
printed passed as first argument rather than adding a if branch in my
top level code.

log(cls.canBePrinted(), "%s", cls.toString());
is IMO prettier than

if(cls.canBePrinted()) {
    log("%s", cls.toString());
}

>
>>>  - 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.
At least jmdavis had a very strong argument against it. Something like
function should be verbs ... Maybe something like logCritical, logInfo
would be a compromise, but I think everybody here has a different idea
of what is correct.
>>>  - 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.
Properly yes, at least will that be my first try.
>
>>>  - 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.
Yes it is per thread. I concur that this needs to be documented well.
>>>  - 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.
Maybe, but I bet somebody else will bring up Allocator and Ref Counting
in that context.
>
> 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).
First, the default logger is assignable so maybe we should talk about
the StdIOLogger which prints to StdIO or maybe StdErr. IMHO { I hate
when that happens, I hate writing redirects to the cmd. } But this is
properly a task issue.
>
> Thanks for bringing this forward!

To whom is this directed?