Review: std.logger

[Andrei Alexandrescu via Digitalmars-d]

On 7/11/14, 7:36 AM, Dicebot wrote:
> Round of a formal review before proceeding to voting. Subject for Phobos
> inclusion : http://wiki.dlang.org/Review/std.logger authored by Robert
> Schadek.

Preface: (1) We already use the std.logger candidate at Facebook; (2) I 
really want to get this in, it's been past time already.

Overall: I think std.logger has a passable overall design having 
simplicity and no-nonsense as compelling advantages. However, there's 
some serious tactical work needed.

0. There's no way to set the minimal logging level statically, except 
for an on/off switch. There must be a way to define e.g. 
-version=logLevel=trace that sets the minimum logging level actually 
performed. Any logging below that level is a no-op. Which segues into 
the next problem:

1. There's a bunch of code still generated even with logging statically 
disabled (per http://d.godbolt.org). I could not find a way to 
completely eliminate generated code. Unused lazy parameters MUST 
generate no code at the front end level. This is a performance bug in 
the D front end that blocks acceptance of this proposal.

2. The three one-letter suffixes don't seem to scale well. They optimize 
for brevity but at the cost of clarity. Also they establish a precedent 
- are we sure we recommend future D code in the standard library and 
beyond to mangle names using one-letter conventions? I think we must 
find a way to solve things via overloads.

Currently we have: log, logc, logf, logl, logcf, loglf, loglc, loglcf. 
One would almost expect logwtf to be somewhere in there.

I think an overloading-based solution would be a better choice here. 
First, I think the logging level must guide all overloads. True, we'd 
sometimes like to log the logging level itself, but that's easily done 
by using a formatting function (as shown below). So:

log(A...)(lazy A stuff); // just log whatevs
log(A...)(LogLevel lvl, lazy A stuff); // log at specified level
log(A...)(bool c, LogLevel lvl, lazy A stuff); // conditionally log at 
specified level

Then three more logf with similar signatures, using the precedent set by 
write/writef. And we're done.

To log the logging level itself, just use logf(someLevel, "%s", 
someLevel) and be done with it.

There's no conditional logging without specifying a level, which should 
be fine seeing as conditional logging is not that frequent to start with.

There should be some shortcuts for logging levels such that one can 
write log(info, "crap") instead of log(LogLevel.info, "crap").

3. I'm not sure I like the design using defaultLogger in conjunction 
with free functions using it. It clearly makes for comfortable casual 
logging by just calling log(whatevs) and it uses a precedent set by 
stdout. But I wonder if it would be cleaner to just give it a shorter 
name "log" and then have it have a "write" method:

log("crap");
-> becomes ->
log.write("crap");

Also there'd be log.writef("%s", "crap") etc.

4. Conditional logging must be justified. In my mind the justification 
is that statically setting the log level makes the code disappear 
without the condition being ever evaluated, but the current design 
doesn't allow setting the log level!

5. There was some nice stuff in the previous std.logger work by me and 
later Jose (I think), which allowed logging every n times/milliseconds 
so as to allow nice throttling. That's nice to omit/defer for 
simplification purposes, but I noticed that log noise is a serious matter.

6. The current backend design requires use of classes and references, 
i.e. garbage collection. Amid the current tendency to make std work 
without requiring GC, I think a design based on RefCounted would be 
recommended here.

7. The current backend design fills a struct with data then passes it to 
the implementation. But if the implementation doesn't use e.g. the 
timestamp then that work has been wasted. Maybe offer the fields as 
properties instead, with caching upon first use?

8. Documentation needs work as it has disfluencies and typos.

9. I've also posted a bunch of comments to the code at 
https://github.com/D-Programming-Language/phobos/pull/1500/files


Andrei