www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - std.experimental.logger formal review round 3

reply "Dicebot" <public dicebot.lv> writes:
Previous review thread : 
http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh forum.dlang.org

Previous voting thread (also contains discussion in the end) : 
http://forum.dlang.org/post/vbotavcclttrgvzcjjia forum.dlang.org

Code : https://github.com/D-Programming-Language/phobos/pull/1500

Important changes since last review:
- new approach for compile-time log level filtering
- thread-safe API (by Marco Leise, commits 3b32618..e71f317)
- documentation enhancements all over the place
- more  nogc annotations
- "raw" log overload that expects pre-formatted message and all 
metadata (file/line/function) as run-time function arguments

(anything I have missed Robert?)

Usual process : 2 weeks for checking out if there are any 
critical issues that are likely to prevent successful voting, 
write a comment if you need more time for review, focus on API 
issues.
Sep 28 2014
next sibling parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Sunday, 28 September 2014 at 12:24:23 UTC, Dicebot wrote:

 Previous review thread : 
 http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh forum.dlang.org

 Previous voting thread (also contains discussion in the end) : 
 http://forum.dlang.org/post/vbotavcclttrgvzcjjia forum.dlang.org

 Code : 
 https://github.com/D-Programming-Language/phobos/pull/1500

 Important changes since last review:
 - new approach for compile-time log level filtering
 - thread-safe API (by Marco Leise, commits 3b32618..e71f317)
 - documentation enhancements all over the place
 - more  nogc annotations
 - "raw" log overload that expects pre-formatted message and all 
 metadata (file/line/function) as run-time function arguments

 (anything I have missed Robert?)


I added more unittests, but unfortunately didn't find any bugs.
The "raw" log overload is actually a template function that has 
one template parameter, the value you want to log.

That's all, I think
Sep 28 2014
next sibling parent reply Marco Leise <Marco.Leise gmx.de> writes:
This module is going to be the go-to logging in D.
Every other D logging library will extend on it, and libraries
will pass their messages through std.logger's `stdlog`.

It is very important that all of you who plan on using logging
in D or know about specific logger implementations take their
time and see if what they have in mind can be implemented on
the API foundations that std.logger provides. Our community
lacks a real logging guru and needs the collective experience.

If you don't have time to experiment, but believe that some
use case must be covered just describe it in a few words.
(Especially effects on API design, performance or threading.)
Sep 29 2014
parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Monday, 29 September 2014 at 10:30:21 UTC, Marco Leise wrote:

 If you don't have time to experiment, but believe that some
 use case must be covered just describe it in a few words.


Two things:

1. Timestamps with millisecond precision or better?

2. Creating one log file per day? I.e. built-in logrotate. If 
it's not built-in, would it be difficult to add on top?
Sep 29 2014
next sibling parent "Robert burner Schadek" <rburners gmail.com> writes:
On Monday, 29 September 2014 at 10:41:06 UTC, Vladimir Panteleev 
wrote:

 On Monday, 29 September 2014 at 10:30:21 UTC, Marco Leise wrote:

 If you don't have time to experiment, but believe that some
 use case must be covered just describe it in a few words.


 Two things:

 1. Timestamps with millisecond precision or better?


You have all that is in Datetime.


 2. Creating one log file per day? I.e. built-in logrotate. If 
 it's not built-in, would it be difficult to add on top?


No, as said in the last review thread there are to many solution 
on different platforms. But you can build the one you need easily.
Sep 29 2014
prev sibling next sibling parent Marco Leise <Marco.Leise gmx.de> writes:
Am Mon, 29 Sep 2014 10:41:05 +0000
schrieb "Vladimir Panteleev" <vladimir thecybershadow.net>:


 Two things:
 
 1. Timestamps with millisecond precision or better?
 
 2. Creating one log file per day? I.e. built-in logrotate. If 
 it's not built-in, would it be difficult to add on top?


Basically your logger gets passed all the basic information
(file, line, function, module, log level, Tid, timestamp as
`SysTime` (up to hnsecs precision), message and the original
logger). You decide how to handle it, whether you pass it on
to another logger, open a file or keep it in memory.

-- 
Marco
Sep 29 2014
prev sibling parent "Dicebot" <public dicebot.lv> writes:
On Monday, 29 September 2014 at 10:41:06 UTC, Vladimir Panteleev 
wrote:

 2. Creating one log file per day? I.e. built-in logrotate. If 
 it's not built-in, would it be difficult to add on top?


I see logger customization explained in top-level docs of 
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d
Do you feel something needs to be added there?
Sep 29 2014
prev sibling next sibling parent Marco Leise <Marco.Leise gmx.de> writes:
The thread-safety changes in short: Every Logger is now taking
a lock when a public method is invoked and then calls the user
overridable methods with the lock taken.

* When you implement a new logger, thread-safety is already
  taken care of.
* Taking a lock in a single-threaded context is practically
  free compared to the actual logging. (So don't worry.)
* It is not possible to enter a Logger with multiple threads
  and provide more fine-grained thread synchronization. In
  particular only one thread at a time can call a global log
  function like `debug(...)` or `error(...)`.

Note: This is different from the original intention to have
Loggers implement thread safety if they need it and a stopgap
measure looking for use cases that need modifications.

--
Marco
Sep 29 2014
prev sibling parent reply "Kevin Lamonte" <kevindotlamnodotonte gmail.com> writes:
I haven't tested it yet, but have two questions anyway:

1. I did not see any reference to the use of Clock.currTime(), 
which on the last round accounted for about 90% of the total time 
spent in a log call.  Reference: 
https://issues.dlang.org/show_bug.cgi?id=13433 .  (This is the 
difference between logging-and-filtering ~100k logs/sec and ~1M 
logs/sec for loggers that use criteria other than logLevel for 
filtering messages.)  Same question for this cycle:  Does 
std.logger API need a method for clients or subclasses to 
change/defer/omit the call to Clock.currTime?  Or defer for a 
change in std.datetime?

2. We have Tid in the API.  What about Fiber and Thread?  If we 
can only pick one, I would vote for Thread rather than Tid, as 
Tid's currently have no way to be uniquely identified in a 
logging message.  Reference: 
https://issues.dlang.org/show_bug.cgi?id=6989

General comment: very nice to see continued progress!
Oct 01 2014
next sibling parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Wednesday, 1 October 2014 at 10:50:54 UTC, Kevin Lamonte wrote:

 I haven't tested it yet, but have two questions anyway:

 1. I did not see any reference to the use of Clock.currTime(), 
 which on the last round accounted for about 90% of the total 
 time spent in a log call.  Reference: 
 https://issues.dlang.org/show_bug.cgi?id=13433 .  (This is the 
 difference between logging-and-filtering ~100k logs/sec and ~1M 
 logs/sec for loggers that use criteria other than logLevel for 
 filtering messages.)  Same question for this cycle:  Does 
 std.logger API need a method for clients or subclasses to 
 change/defer/omit the call to Clock.currTime?  Or defer for a 
 change in std.datetime?


maybe I should add a disableGetSysTime switch


 2. We have Tid in the API.  What about Fiber and Thread?  If we 
 can only pick one, I would vote for Thread rather than Tid, as 
 Tid's currently have no way to be uniquely identified in a 
 logging message.  Reference: 
 https://issues.dlang.org/show_bug.cgi?id=6989

 General comment: very nice to see continued progress!


I'm gone take a closer look
Oct 01 2014
next sibling parent reply Marco Leise <Marco.Leise gmx.de> writes:
Am Wed, 01 Oct 2014 12:49:29 +0000
schrieb "Robert burner Schadek" <rburners gmail.com>:


 maybe I should add a disableGetSysTime switch


CLOCK_REALTIME_COARSE / CLOCK_REALTIME_FAST should be explored.
On Linux you can't expect finer resolution than the kernel Hz,
for FreeBSD I only found mention of 10ms resolution.
If you format log messages with 2 digits time precision
anyway, you don't need the precise version.

If you disable time completely, what would the LogEntry
contain as the time stamp? SysTime.init?

-- 
Marco
Oct 01 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Wednesday, 1 October 2014 at 14:24:52 UTC, Marco Leise wrote:

 Am Wed, 01 Oct 2014 12:49:29 +0000
 schrieb "Robert burner Schadek" <rburners gmail.com>:


 maybe I should add a disableGetSysTime switch


 CLOCK_REALTIME_COARSE / CLOCK_REALTIME_FAST should be explored.


good pointer, but what about Win and Mac


 If you disable time completely, what would the LogEntry
 contain as the time stamp? SysTime.init?


That was my first idea.
Oct 01 2014
next sibling parent reply Marco Leise <Marco.Leise gmx.de> writes:
Am Wed, 01 Oct 2014 15:05:53 +0000
schrieb "Robert burner Schadek" <rburners gmail.com>:


 On Wednesday, 1 October 2014 at 14:24:52 UTC, Marco Leise wrote:

 Am Wed, 01 Oct 2014 12:49:29 +0000
 schrieb "Robert burner Schadek" <rburners gmail.com>:


 maybe I should add a disableGetSysTime switch


 CLOCK_REALTIME_COARSE / CLOCK_REALTIME_FAST should be explored.


 
 good pointer, but what about Win and Mac


Windows 2000 had some function that returns 4ms accurate time,
I hope it is implemented like CLOCK_REALTIME_COARSE.
OS X ... oh well. Don't know. Just declare the "fast timer" a
hint, I guess. Like when you ask for anti-aliasing in OpenGL
and the implementation is free to decide if it can or want's
to deliver. So it turns into: "I need a sub-second
timestamp, but make it as fast as possible on the target OS".
Maybe some day Apple will copy CLOCK_REALTIME_FAST from
FreeBSD.


 If you disable time completely, what would the LogEntry
 contain as the time stamp? SysTime.init?


 
 That was my first idea.




-- 
Marco
Oct 01 2014
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/1/14 11:53 AM, Marco Leise wrote:

 Am Wed, 01 Oct 2014 15:05:53 +0000
 schrieb "Robert burner Schadek" <rburners gmail.com>:


 On Wednesday, 1 October 2014 at 14:24:52 UTC, Marco Leise wrote:

 Am Wed, 01 Oct 2014 12:49:29 +0000
 schrieb "Robert burner Schadek" <rburners gmail.com>:


 maybe I should add a disableGetSysTime switch


 CLOCK_REALTIME_COARSE / CLOCK_REALTIME_FAST should be explored.


 good pointer, but what about Win and Mac


 Windows 2000 had some function that returns 4ms accurate time,
 I hope it is implemented like CLOCK_REALTIME_COARSE.


I think it wouldn't be prudent to explore Windows options until we prove 
the windows currTime is slow like the Linux version.

On Mac, gettimeofday is used. I don't know that it's necessarily slow, 
but I don't know of a better way to get the wall clock time.

-Steve
Oct 01 2014
parent "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= writes:
On Wednesday, 1 October 2014 at 16:10:41 UTC, Steven 
Schveighoffer wrote:

 I think it wouldn't be prudent to explore Windows options until 
 we prove the windows currTime is slow like the Linux version.

 On Mac, gettimeofday is used. I don't know that it's 
 necessarily slow, but I don't know of a better way to get the 
 wall clock time.


x86 has a builtin time stamp counter, executes faster than the 
fastest system call.

http://en.wikipedia.org/wiki/Time_Stamp_Counter

http://en.wikipedia.org/wiki/High_Precision_Event_Timer
Oct 02 2014
prev sibling parent =?UTF-8?B?U8O2bmtlIEx1ZHdpZw==?= <sludwig rejectedsoftware.com> writes:
Am 01.10.2014 17:05, schrieb Robert burner Schadek:

 On Wednesday, 1 October 2014 at 14:24:52 UTC, Marco Leise wrote:

 Am Wed, 01 Oct 2014 12:49:29 +0000
 schrieb "Robert burner Schadek" <rburners gmail.com>:

 If you disable time completely, what would the LogEntry
 contain as the time stamp? SysTime.init?


 That was my first idea.


It should be noted that SysTime.init.toString() currently crashes (at 
least some other methods, too).
Oct 02 2014
prev sibling parent =?UTF-8?B?U8O2bmtlIEx1ZHdpZw==?= <sludwig rejectedsoftware.com> writes:
Am 01.10.2014 14:49, schrieb Robert burner Schadek:

 On Wednesday, 1 October 2014 at 10:50:54 UTC, Kevin Lamonte wrote:

 I haven't tested it yet, but have two questions anyway:

 1. I did not see any reference to the use of Clock.currTime(), which
 on the last round accounted for about 90% of the total time spent in a
 log call.  Reference: https://issues.dlang.org/show_bug.cgi?id=13433
 .  (This is the difference between logging-and-filtering ~100k
 logs/sec and ~1M logs/sec for loggers that use criteria other than
 logLevel for filtering messages.)  Same question for this cycle:  Does
 std.logger API need a method for clients or subclasses to
 change/defer/omit the call to Clock.currTime?  Or defer for a change
 in std.datetime?


 maybe I should add a disableGetSysTime switch


Don't know if this is still the case, but calling Clock.currTime() used 
to allocate on each call, whereas Clock.currTime(UTC()) didn't and thus 
was orders of magnitudes faster.
Oct 02 2014
prev sibling parent reply "Dicebot" <public dicebot.lv> writes:
On Wednesday, 1 October 2014 at 10:50:54 UTC, Kevin Lamonte wrote:

 2. We have Tid in the API.  What about Fiber and Thread?  If we 
 can only pick one, I would vote for Thread rather than Tid, as 
 Tid's currently have no way to be uniquely identified in a 
 logging message.  Reference: 
 https://issues.dlang.org/show_bug.cgi?id=6989


In my opinion only solution that scales is to provide same ID as 
one used by std.concurrency - it can be thread, fiber, process or 
pretty much anything. There are many possible threading 
abstractions and all can't be easily supported, makes sense to 
stick to one considered standard.
Oct 01 2014
parent reply "Kevin Lamonte" <kevindotlamnodotonte gmail.com> writes:
On Wednesday, 1 October 2014 at 13:37:43 UTC, Dicebot wrote:

 On Wednesday, 1 October 2014 at 10:50:54 UTC, Kevin Lamonte 
 wrote:

 2. We have Tid in the API.  What about Fiber and Thread?  If 
 we can only pick one, I would vote for Thread rather than Tid, 
 as Tid's currently have no way to be uniquely identified in a 
 logging message.  Reference: 
 https://issues.dlang.org/show_bug.cgi?id=6989


 In my opinion only solution that scales is to provide same ID 
 as one used by std.concurrency - it can be thread, fiber, 
 process or pretty much anything. There are many possible 
 threading abstractions and all can't be easily supported, makes 
 sense to stick to one considered standard.


Long-term this sounds very reasonable.  But the problem at the 
moment is that std.concurrency provides nothing that can be used 
to distinguish threads in a log file.  Tid cannot be converted to 
a unique string or vice versa, nor can one Tid.join() to ensure 
that a child Thread has completed before closing the log file(s) 
and exiting main().  For boring end-user applications Threads are 
today's most common denominator.

Would PR 
https://github.com/D-Programming-Language/phobos/pull/1910 
provide a way given a Tid to determine: a) What underlying 
concurrency model it is using (Thread, Fiber, process, future)? 
b) Uniquely identify that structure (Thread ID string, Fiber 
address string, process ID, something else)?  c) Be capable of 
using that identifying immutable (because it needs to be 
send()able to another Tid writing to network/file/etc) 
string-representable thing to find the original Tid again?  A+B 
is necessary for using std.logger to debug concurrent 
applications, C is a very nice-to-have that comes up periodically.
Oct 02 2014
parent reply "Sean Kelly" <sean invisibleduck.org> writes:
On Thursday, 2 October 2014 at 10:37:02 UTC, Kevin Lamonte wrote:

 Would PR 
 https://github.com/D-Programming-Language/phobos/pull/1910 
 provide a way given a Tid to determine: a) What underlying 
 concurrency model it is using (Thread, Fiber, process, future)? 
 b) Uniquely identify that structure (Thread ID string, Fiber 
 address string, process ID, something else)?  c) Be capable of 
 using that identifying immutable (because it needs to be 
 send()able to another Tid writing to network/file/etc) 
 string-representable thing to find the original Tid again?  A+B 
 is necessary for using std.logger to debug concurrent 
 applications, C is a very nice-to-have that comes up 
 periodically.


register() is meant to provide a means of referring to a thread.  
But the relevant thing there is finding a thread by role, not by 
instance.  So if a thread doing a known job terminates, a new one 
can spawn and register under the same name so proper operation 
can continue.  Having an identifier for logging is a bit 
different.  Would using the MessageBox address be sufficient?  
I'd be happy to add a Tid.id property that returns a value like 
this.  I'd rather not try to generate a globally unique 
identifier though (that would probably mean a UUID, which is long 
and expensive to generate).
Oct 05 2014
parent reply "Kevin Lamonte" <kevindotlamnodotonte gmail.com> writes:
On Sunday, 5 October 2014 at 17:06:06 UTC, Sean Kelly wrote:


 Having an identifier for logging is a bit different.  Would 
 using the MessageBox address be sufficient?  I'd be happy to 
 add a Tid.id property that returns a value like this.  I'd 
 rather not try to generate a globally unique identifier though 
 (that would probably mean a UUID, which is long and expensive 
 to generate).


I think Tid.id returning the MessageBox address would be fine for 
logging purposes.  The main value is being able to distinguish 
messages coming in at the same time from multiple threads.  Even 
if a MessageBox address was re-used by a new receive()er after a 
previous one exited I doubt it would confuse users very much.  I 
thing that a doc on Tid.id like "this value will not be the same 
as any other Tid currently existing, but might be the same as a 
Tid that has exited previously" would be sufficient.
Oct 05 2014
parent "Marc =?UTF-8?B?U2Now7x0eiI=?= <schuetzm gmx.net> writes:
On Sunday, 5 October 2014 at 23:40:32 UTC, Kevin Lamonte wrote:

 On Sunday, 5 October 2014 at 17:06:06 UTC, Sean Kelly wrote:


 Having an identifier for logging is a bit different.  Would 
 using the MessageBox address be sufficient?  I'd be happy to 
 add a Tid.id property that returns a value like this.  I'd 
 rather not try to generate a globally unique identifier though 
 (that would probably mean a UUID, which is long and expensive 
 to generate).


 I think Tid.id returning the MessageBox address would be fine 
 for logging purposes.  The main value is being able to 
 distinguish messages coming in at the same time from multiple 
 threads.  Even if a MessageBox address was re-used by a new 
 receive()er after a previous one exited I doubt it would 
 confuse users very much.  I thing that a doc on Tid.id like 
 "this value will not be the same as any other Tid currently 
 existing, but might be the same as a Tid that has exited 
 previously" would be sufficient.


A moving GC can affect the address, too, but for your purpose it 
would still be fine, you just mustn't put too much significance 
into the actual value.
Oct 07 2014
prev sibling next sibling parent reply "deadalnix" <deadalnix gmail.com> writes:
On Sunday, 28 September 2014 at 12:24:23 UTC, Dicebot wrote:

 Previous review thread : 
 http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh forum.dlang.org

 Previous voting thread (also contains discussion in the end) : 
 http://forum.dlang.org/post/vbotavcclttrgvzcjjia forum.dlang.org

 Code : 
 https://github.com/D-Programming-Language/phobos/pull/1500

 Important changes since last review:
 - new approach for compile-time log level filtering
 - thread-safe API (by Marco Leise, commits 3b32618..e71f317)
 - documentation enhancements all over the place
 - more  nogc annotations
 - "raw" log overload that expects pre-formatted message and all 
 metadata (file/line/function) as run-time function arguments

 (anything I have missed Robert?)

 Usual process : 2 weeks for checking out if there are any 
 critical issues that are likely to prevent successful voting, 
 write a comment if you need more time for review, focus on API 
 issues.


It has been mentioned in the comment already, but log4j like
approach seems better. Also, it is sad that output range are not
leveraged to format/sample/filter/select output of the logger.
Sep 29 2014
parent "Robert burner Schadek" <rburners gmail.com> writes:
On Tuesday, 30 September 2014 at 00:19:00 UTC, deadalnix wrote:

 It has been mentioned in the comment already, but log4j like
 approach seems better. Also, it is sad that output range are not
 leveraged to format/sample/filter/select output of the logger.


yeah log4j is totally awesome and shows IMO how pretty something 
can be build on top of std.logger. Even though it uses the 
non-range LogEntry from std.logger.
Sep 30 2014
prev sibling next sibling parent "ponce" <contact gam3sfrommars.fr> writes:
On Sunday, 28 September 2014 at 12:24:23 UTC, Dicebot wrote:

 Previous review thread : 
 http://forum.dlang.org/post/zhvmkbahrqtgkptdlcvh forum.dlang.org

 Previous voting thread (also contains discussion in the end) : 
 http://forum.dlang.org/post/vbotavcclttrgvzcjjia forum.dlang.org

 Code : 
 https://github.com/D-Programming-Language/phobos/pull/1500

 Important changes since last review:
 - new approach for compile-time log level filtering
 - thread-safe API (by Marco Leise, commits 3b32618..e71f317)
 - documentation enhancements all over the place
 - more  nogc annotations
 - "raw" log overload that expects pre-formatted message and all 
 metadata (file/line/function) as run-time function arguments

 (anything I have missed Robert?)

 Usual process : 2 weeks for checking out if there are any 
 critical issues that are likely to prevent successful voting, 
 write a comment if you need more time for review, focus on API 
 issues.


Upgraded my logger to 0.3.0. I like that I don't have to make 
them thread-safe myself.
I vote 'yes' again.
Sep 29 2014
prev sibling next sibling parent reply Marco Leise <Marco.Leise gmx.de> writes:
How would I typically log an exception?
I thought of one line per exception in the chain plus a full
trace for the last exception in the chain.

So the messages would be like:
  Initialization failed.
  Could not load configuration.
  File "~/.config/app/settings.ini" no found.
  Stack trace (outer to inner):
    _Dmain_yadda_yadda+120
    Config.__ctor(=E2=80=A6)+312
    =E2=80=A6
    File.__ctor(=E2=80=A6)+12

So far so good, but I'm stuck at this line of code (where `e`
is a Throwable):

  error(e.msg, e.line, e.file, funcName, prettyFuncName, moduleName);

I don't know how to get at the function and module name where
the exception was thrown. I know this stuff is part of the
symbolic debug information, but I would think it is a common
use case of a logger to log exceptions.

Is it? And if so, what do we want to do about it?

--=20
Marco
Oct 02 2014
next sibling parent "Kevin Lamonte" <kevindotlamnodotonte gmail.com> writes:
On Thursday, 2 October 2014 at 18:11:26 UTC, Marco Leise wrote:

 How would I typically log an exception?


We could add a Throwable reference in LogEntry and some 
overrides.  But how exception stack traces appear in the output 
(multiple lines, all-on-one line, e.msg only, following the 
chain, etc.) should not be firmly set here:  we can provide a 
reasonable baseline in FileLogger, but other Logger subclasses 
will definitely want to do different things.

Another issue is that in practice one will often want to log at 
debug an exception they are about to throw (libraries), but log 
at error anything they actually catch (applications).  Should we 
include those use cases too?  This skirts very close to the 
"policy, not mechanism" line, but might be worth it.

Proposal:

    LogEntry {
        ...
        Throwable throwable; /// Optional Throwable
        bool caught;         /// If true, then throwable was 
logged via caught/caughtf
    }

    void throwing(Throwable t, string msg) {...}
    void throwingf(Throwable t, string formatMsg, A... args) {...}
    void caught(Throwable t, string msg) {...}
    void caughtf(Throwable t, string formatMsg, A... args) {...}

Thoughts?
Oct 03 2014
prev sibling parent "Robert burner Schadek" <rburners gmail.com> writes:
On Thursday, 2 October 2014 at 18:11:26 UTC, Marco Leise wrote:

 How would I typically log an exception?
 I thought of one line per exception in the chain plus a full
 trace for the last exception in the chain.

 So the messages would be like:
   Initialization failed.
   Could not load configuration.
   File "~/.config/app/settings.ini" no found.
   Stack trace (outer to inner):
     _Dmain_yadda_yadda+120
     Config.__ctor(…)+312
     …
     File.__ctor(…)+12

 So far so good, but I'm stuck at this line of code (where `e`
 is a Throwable):

   error(e.msg, e.line, e.file, funcName, prettyFuncName, 
 moduleName);

 I don't know how to get at the function and module name where
 the exception was thrown. I know this stuff is part of the
 symbolic debug information, but I would think it is a common
 use case of a logger to log exceptions.


I guess that should be part of the Exception. I have no idea how 
to get __FUCTION__ of and Exception from inside another function.


 Is it? And if so, what do we want to do about it?
Oct 05 2014
prev sibling next sibling parent reply =?UTF-8?B?U8O2bmtlIEx1ZHdpZw==?= <sludwig rejectedsoftware.com> writes:
I still think that there should be the two predefined log levels "debug" 
(for developer related diagnostics) and "diagnostic" (for end user 
related diagnostics) between "trace" and "info". This is important for 
interoperability of different libraries, so that they have predictable 
debug output.

But independent of that, there should really be a function for safely 
generating the user defined intermediate log levels, maybe like this:

     LogLevel raiseLevel(LogLevel base_level, ubyte increment)
     {
         assert(base_level is a valid base level);
         assert(base_level + increment smaller than the next base level);
         return cast(LogLevel)(base_level + increment);
     }

     // ok
     enum notice = raiseLevel(LogLevel.info, 16);

     // error, overlaps with the next base level
     enum suspicious = raiseLevel(LogLevel.info, 32);

Casting to an enum type is a pretty blunt operation, so it should at 
least be made as safe as possible.
Oct 02 2014
parent "Robert burner Schadek" <rburners gmail.com> writes:
On Thursday, 2 October 2014 at 20:15:32 UTC, Sönke Ludwig wrote:

 I still think that there should be the two predefined log 
 levels "debug" (for developer related diagnostics) and 
 "diagnostic" (for end user related diagnostics) between "trace" 
 and "info". This is important for interoperability of different 
 libraries, so that they have predictable debug output.

 But independent of that, there should really be a function for 
 safely generating the user defined intermediate log levels, 
 maybe like this:

     LogLevel raiseLevel(LogLevel base_level, ubyte increment)
     {
         assert(base_level is a valid base level);
         assert(base_level + increment smaller than the next 
 base level);
         return cast(LogLevel)(base_level + increment);
     }

     // ok
     enum notice = raiseLevel(LogLevel.info, 16);

     // error, overlaps with the next base level
     enum suspicious = raiseLevel(LogLevel.info, 32);

 Casting to an enum type is a pretty blunt operation, so it 
 should at least be made as safe as possible.


from log4d
/// Aliases for debugX and fineX functions
alias debug1  = defaultLogFunction!(Log4DLogger.LOG_LEVEL_DEBUG1);
alias debug2  = defaultLogFunction!(Log4DLogger.LOG_LEVEL_DEBUG2);
alias debug3  = defaultLogFunction!(Log4DLogger.LOG_LEVEL_DEBUG3);

but adding a raiseLevel function should be no problem
Oct 05 2014
prev sibling next sibling parent reply "Dicebot" <public dicebot.lv> writes:
I don't see critical objections so far and this will move to 
voting stage this weekend. Please hurry up if you want to say 
something bad :)
Oct 10 2014
next sibling parent reply "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Saturday, 11 October 2014 at 03:41:08 UTC, Dicebot wrote:

 I don't see critical objections so far and this will move to 
 voting stage this weekend. Please hurry up if you want to say 
 something bad :)


Attributes need to be applied thoroughly. Even if most uses will 
be through the base class `Logger`, it's still useful to have 
stronger guarantees through a derived class reference. This is 
particularly important because it's an important design decision 
to choose which attributes to apply to `Logger`'s methods.

 trusted is used everywhere instead of properly using  safe and 
minimized  trusted. I think this is the third time I point this 
out...

The multiloggers are a complete mess. There's both `ArrayLogger` 
and `MultiLogger`, and while `ArrayLogger` has simple O(n) 
operations, `MultiLogger` is a disaster: insertion iterates all 
elements twice and sorts the entire collection on every call, and 
removal iterates all elements once, then does binary search 
twice. Once using `SortedRange`'s search, and once using its own 
binary search algorithm. It also contains debug code that writes 
to stdout. Neither type adheres to the Phobos container concept, 
instead the underlying array is exposed as a public, undocumented 
field. `string` is used instead of `const(char)[]` for search and 
removal operations.

The implementation of `Logger` has several performance problems. 
`Logger` provides default behaviour that allocates GC memory 
multiple times for even the simplest log messages through the 
`Appender`. I don't think this behaviour should be encouraged by 
putting it in the root logger class, and besides, it can be made 
much more intelligent than just using a new appender for each 
message.

Another issue is that the way it's written currently, 
`writeLogPart` is called a lot more often than necessary, without 
any opportunity for optimization within `formattedWrite`, thus 
`FileLogger` is doomed to write to the underlying file 
character-by-character in easily reproducible circumstances (e.g. 
log a range of characters); this issue probably doesn't affect 
the API though.

`Logger` has a bunch of public and documented `*Impl` functions...

Some other line comments I posted a while ago have not been 
addressed.
Oct 10 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Saturday, 11 October 2014 at 04:31:17 UTC, Jakob Ovrum wrote:

 On Saturday, 11 October 2014 at 03:41:08 UTC, Dicebot wrote:

 I don't see critical objections so far and this will move to 
 voting stage this weekend. Please hurry up if you want to say 
 something bad :)


 Attributes need to be applied thoroughly. Even if most uses 
 will be through the base class `Logger`, it's still useful to 
 have stronger guarantees through a derived class reference. 
 This is particularly important because it's an important design 
 decision to choose which attributes to apply to `Logger`'s 
 methods.

  trusted is used everywhere instead of properly using  safe and 
 minimized  trusted. I think this is the third time I point this 
 out...

 The multiloggers are a complete mess. There's both 
 `ArrayLogger` and `MultiLogger`, and while `ArrayLogger` has 
 simple O(n) operations, `MultiLogger` is a disaster: insertion 
 iterates all elements twice and sorts the entire collection on 
 every call, and removal iterates all elements once, then does 
 binary search twice. Once using `SortedRange`'s search, and 
 once using its own binary search algorithm. It also contains 
 debug code that writes to stdout. Neither type adheres to the 
 Phobos container concept, instead the underlying array is 
 exposed as a public, undocumented field. `string` is used 
 instead of `const(char)[]` for search and removal operations.


The latest std.container.Array broke the code anyway, so it is 
due for a rewrite anyway.


 The implementation of `Logger` has several performance 
 problems. `Logger` provides default behaviour that allocates GC 
 memory multiple times for even the simplest log messages 
 through the `Appender`. I don't think this behaviour should be 
 encouraged by putting it in the root logger class, and besides, 
 it can be made much more intelligent than just using a new 
 appender for each message.


Well, to have ultra simple thread-safe sub classing (which is an 
important part of the design), this was the price. This being 
said. Doing it nogc yourself if you know the output is very easy 
as shown in FileLogger.


 Another issue is that the way it's written currently, 
 `writeLogPart` is called a lot more often than necessary, 
 without any opportunity for optimization within 
 `formattedWrite`, thus `FileLogger` is doomed to write to the 
 underlying file character-by-character in easily reproducible 
 circumstances (e.g. log a range of characters); this issue 
 probably doesn't affect the API though.


Again, by design. To allow user created structured logging, this 
is necessary.

 `Logger` has a bunch of public and documented `*Impl` 
 functions...


see my other post


 Some other line comments I posted a while ago have not been 
 addressed.


I will recheck github
Oct 11 2014
parent reply Marco Leise <Marco.Leise gmx.de> writes:
Am Sat, 11 Oct 2014 12:23:10 +0000
schrieb "Robert burner Schadek" <rburners gmail.com>:


 On Saturday, 11 October 2014 at 04:31:17 UTC, Jakob Ovrum wrote:

 The implementation of `Logger` has several performance=20
 problems. `Logger` provides default behaviour that allocates GC=20
 memory multiple times for even the simplest log messages=20
 through the `Appender`. I don't think this behaviour should be=20
 encouraged by putting it in the root logger class, and besides,=20
 it can be made much more intelligent than just using a new=20
 appender for each message.


=20
 Well, to have ultra simple thread-safe sub classing (which is an=20
 important part of the design), this was the price. This being=20
 said. Doing it nogc yourself if you know the output is very easy=20
 as shown in FileLogger.


I had the same feeling as Jakob about an `Appender` already
in the base class and would have expected a bare bones
abstract class + a batteries included version using `Appender`.
(A bit like Java's =E2=80=A6Listener and =E2=80=A6Adapter classes.)
That seems more clean to me in a representational fashion.
Technically we can just ignore the extra field...
It also seems legit to reduce pressure on the GC, by resetting
the `Appender` instead of nulling it.

--=20
Marco
Oct 11 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Saturday, 11 October 2014 at 23:37:42 UTC, Marco Leise wrote:

 I had the same feeling as Jakob about an `Appender` already
 in the base class and would have expected a bare bones
 abstract class + a batteries included version using `Appender`.
 (A bit like Java's …Listener and …Adapter classes.)
 That seems more clean to me in a representational fashion.
 Technically we can just ignore the extra field...
 It also seems legit to reduce pressure on the GC, by resetting
 the `Appender` instead of nulling it.


What if a Logger down the chain keeps the string around and you 
overwrite it?
Oct 12 2014
parent Marco Leise <Marco.Leise gmx.de> writes:
Am Sun, 12 Oct 2014 12:07:55 +0000
schrieb "Robert burner Schadek" <rburners gmail.com>:


 On Saturday, 11 October 2014 at 23:37:42 UTC, Marco Leise wrote:

 I had the same feeling as Jakob about an `Appender` already
 in the base class and would have expected a bare bones
 abstract class + a batteries included version using `Appender`.
 (A bit like Java's =E2=80=A6Listener and =E2=80=A6Adapter classes.)
 That seems more clean to me in a representational fashion.
 Technically we can just ignore the extra field...
 It also seems legit to reduce pressure on the GC, by resetting
 the `Appender` instead of nulling it.


=20
 What if a Logger down the chain keeps the string around and you=20
 overwrite it?


That would be bad.

--=20
Marco
Oct 14 2014
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2014-10-11 05:41, Dicebot wrote:

 I don't see critical objections so far and this will move to voting
 stage this weekend. Please hurry up if you want to say something bad :)


I think it's unacceptable that the documentation of "defaultLogFunction" 
and "trace", "info" and so on is merged. Same thing with 
"memLogFunctions". Do these even need to be exposed?

-- 
/Jacob Carlborg
Oct 11 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Saturday, 11 October 2014 at 10:48:00 UTC, Jacob Carlborg 
wrote:

 On 2014-10-11 05:41, Dicebot wrote:

 I don't see critical objections so far and this will move to 
 voting
 stage this weekend. Please hurry up if you want to say 
 something bad :)


 I think it's unacceptable that the documentation of 
 "defaultLogFunction" and "trace", "info" and so on is merged. 
 Same thing with "memLogFunctions". Do these even need to be 
 exposed?


this has been used make user defined LogLevel log functions, like

trace1(...), trace2(...)
Oct 11 2014
parent Jacob Carlborg <doob me.com> writes:
On 2014-10-11 14:08, Robert burner Schadek wrote:


 this has been used make user defined LogLevel log functions, like

 trace1(...), trace2(...)


I still don't like the merge of the documentation.

-- 
/Jacob Carlborg
Oct 11 2014
prev sibling next sibling parent "Robert burner Schadek" <rburners gmail.com> writes:
On Saturday, 11 October 2014 at 03:41:08 UTC, Dicebot wrote:

 I don't see critical objections so far and this will move to 
 voting stage this weekend. Please hurry up if you want to say 
 something bad :)


MultiLogger got a new simpler impl.
Oct 11 2014
prev sibling next sibling parent "Ola Fosheim Grostad" <ola.fosheim.grostad+dlang gmail.com> writes:
On Saturday, 11 October 2014 at 03:41:08 UTC, Dicebot wrote:

 I don't see critical objections so far and this will move to 
 voting stage this weekend. Please hurry up if you want to say 
 something bad :)


I don't think I will use it for the reasons I stated in the 
previous thread, so no point in saying anything bad.
Oct 11 2014
prev sibling parent reply "Dicebot" <public dicebot.lv> writes:
As there was quite some last moment feedback I am giving some 
more time for me to research issues a bit and Robert to address 
them :)
Oct 14 2014
next sibling parent reply "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Wednesday, 15 October 2014 at 02:54:27 UTC, Dicebot wrote:

 As there was quite some last moment feedback I am giving some 
 more time for me to research issues a bit and Robert to address 
 them :)


The Pareto Principle could be worth mentioning here. We were 80% 
of the way to a quality interface a long time ago, but the last 
20% is taking a disproportionate amount of time to iron out. I 
think all this criticism is indicative that we're holding this 
module to a high standard rather than the code being bad, which I 
think is a very good thing. Thankfully Marco stepped up and 
provided a solution to the threading problem, so I don't think 
it's that far off.

Apropos threading though, I'm not sure how to consolidate the 
fact that we're using shared memory without using `shared`. It 
seems like a failure to have such an intricately designed memory 
model, then as soon as we do threading in Phobos, we ignore it.

I still intend to go through all the documentation and fix things 
I can spot as soon as the interface is finalized.
Oct 14 2014
parent "Paolo Invernizzi" <paolo.invernizzi no.address> writes:
On Wednesday, 15 October 2014 at 03:45:12 UTC, Jakob Ovrum wrote:

 On Wednesday, 15 October 2014 at 02:54:27 UTC, Dicebot wrote:

 Apropos threading though, I'm not sure how to consolidate the 
 fact that we're using shared memory without using `shared`. It 
 seems like a failure to have such an intricately designed 
 memory model, then as soon as we do threading in Phobos, we 
 ignore it.


I've written several times that having the TDPL Concurrency 
chapter as a free link in the main page it's some sort of 
horrible masochism, granted the current state of the 
implementation of what it's described there...

But well ;-/
---
/Paolo
Oct 14 2014
prev sibling parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Wednesday, 15 October 2014 at 02:54:27 UTC, Dicebot wrote:

 As there was quite some last moment feedback I am giving some 
 more time for me to research issues a bit and Robert to address 
 them :)


No need, I fixed the MultiLogger last weekend.
Oct 15 2014
parent reply "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Wednesday, 15 October 2014 at 09:25:07 UTC, Robert burner 
Schadek wrote:

 On Wednesday, 15 October 2014 at 02:54:27 UTC, Dicebot wrote:

 As there was quite some last moment feedback I am giving some 
 more time for me to research issues a bit and Robert to 
 address them :)


 No need, I fixed the MultiLogger last weekend.


"Fixed" by simply removing the attempt at logarithmic time 
operations, and still not conforming to the Phobos container 
interface...
Oct 15 2014
parent "Robert burner Schadek" <rburners gmail.com> writes:
On Wednesday, 15 October 2014 at 10:12:56 UTC, Jakob Ovrum wrote:

 On Wednesday, 15 October 2014 at 09:25:07 UTC, Robert burner 
 Schadek wrote:

 On Wednesday, 15 October 2014 at 02:54:27 UTC, Dicebot wrote:

 As there was quite some last moment feedback I am giving some 
 more time for me to research issues a bit and Robert to 
 address them :)


 No need, I fixed the MultiLogger last weekend.


 "Fixed" by simply removing the attempt at logarithmic time 
 operations, and still not conforming to the Phobos container 
 interface...


Yes, because other people already complained that nobody will 
every need a logarithmic time MulitLogger and I always thought 
that MultiLogger was to complex anyway. I could make the Logger[] 
array alias this and it would be comforming to Phobos container, 
but that doesn't really solve the issue does it.
Oct 15 2014
prev sibling next sibling parent reply Martin Nowak <code+news.digitalmars dawg.eu> writes:
On 09/28/2014 02:24 PM, Dicebot wrote:

 Important changes since last review:
 - new approach for compile-time log level filtering


What's new here? It still relies on version identifiers to do so.
As I said in some earlier review, I think it's a bad idea for a library 
to rely on version identifiers that are defined in client code.
I will only work for templated code and makes it much harder for build 
tools. In a way it's the equivalent of
     #define LOG_LEVEL 2
     #include <logger.h>

I even proposed an alternative that uses type tags instead.
http://dpaste.dzfl.pl/95fb6a4e086d


 Usual process : 2 weeks for checking out if there are any critical
 issues that are likely to prevent successful voting, write a comment if
 you need more time for review, focus on API issues.


- Documentation is out of sync.
- ArrayLogger seems to do about the same as MultiLogger
- Why do loggers have to be classes?
   - Define a concept (isLogger template) with the requirements.
   - Add a Logger interface and a loggerObject to wrap structures when 
polymorphic loggers are needed.
Oct 11 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Saturday, 11 October 2014 at 13:16:18 UTC, Martin Nowak wrote:

 On 09/28/2014 02:24 PM, Dicebot wrote:

 Important changes since last review:
 - new approach for compile-time log level filtering


 What's new here? It still relies on version identifiers to do 
 so.
 As I said in some earlier review, I think it's a bad idea for a 
 library to rely on version identifiers that are defined in 
 client code.
 I will only work for templated code and makes it much harder 
 for build tools. In a way it's the equivalent of
     #define LOG_LEVEL 2
     #include <logger.h>


All that code is contained in 30 line template, That is by far 
the best working option anybody could come up with


 I even proposed an alternative that uses type tags instead.
 http://dpaste.dzfl.pl/95fb6a4e086d


And I showed that it did not work.


 - Documentation is out of sync.


gh-page is yes, give me 15min


 - ArrayLogger seems to do about the same as MultiLogger


have you read my reply to Jacob

 - Why do loggers have to be classes?


As answered multiply times before, to build log hierarchies.
Oct 11 2014
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2014-10-11 15:34, Robert burner Schadek wrote:


 - Why do loggers have to be classes?


 As answered multiply times before, to build log hierarchies.


What's stopping an interface or class to implement a logging concept?

-- 
/Jacob Carlborg
Oct 12 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Sunday, 12 October 2014 at 09:07:37 UTC, Jacob Carlborg wrote:

 On 2014-10-11 15:34, Robert burner Schadek wrote:


 - Why do loggers have to be classes?


 As answered multiply times before, to build log hierarchies.


 What's stopping an interface or class to implement a logging 
 concept?


Same as last time:
Logger[], Logger without a LogLevel not real useful IMO, (new) no 
thread safety by default
Oct 12 2014
parent reply "Martin Nowak" <code dawg.eu> writes:
On Sunday, 12 October 2014 at 12:06:44 UTC, Robert burner Schadek 
wrote:

 What's stopping an interface or class to implement a logging 
 concept?


 Same as last time:
 Logger[], Logger without a LogLevel not real useful IMO, (new) 
 no thread safety by default


I don't understand your answer. Do you have a link to your last 
response.
Oct 24 2014
parent reply "Robert burner Schadek" <rburners gmail.com> writes:
On Friday, 24 October 2014 at 11:01:40 UTC, Martin Nowak wrote:

 On Sunday, 12 October 2014 at 12:06:44 UTC, Robert burner 
 Schadek wrote:

 What's stopping an interface or class to implement a logging 
 concept?


 Same as last time:
 Logger[], Logger without a LogLevel not real useful IMO, (new) 
 no thread safety by default


 I don't understand your answer. Do you have a link to your last 
 response.


You can not tell if the Logger will log a message, because you 
can't know its LogLevel. It is not thread safe because the 
interface can't have an implementation. Therefore the default 
implementation is not thread safe.
Oct 24 2014
parent Martin Nowak <code+news.digitalmars dawg.eu> writes:
On 10/24/2014 02:16 PM, Robert burner Schadek wrote:

 On Friday, 24 October 2014 at 11:01:40 UTC, Martin Nowak wrote:

 On Sunday, 12 October 2014 at 12:06:44 UTC, Robert burner Schadek wrote:

 What's stopping an interface or class to implement a logging concept?


 Same as last time:
 Logger[], Logger without a LogLevel not real useful IMO, (new) no
 thread safety by default


 I don't understand your answer. Do you have a link to your last response.


 You can not tell if the Logger will log a message, because you can't
 know its LogLevel. It is not thread safe because the interface can't
 have an implementation. Therefore the default implementation is not
 thread safe.


Well then the concept is that a Logger has some method that returns the 
LogLevel (might be a runtime value). Where is the problem? We use this 
for empty in ranges for example.
Oct 26 2014
prev sibling next sibling parent reply "Martin Nowak" <code dawg.eu> writes:
On Saturday, 11 October 2014 at 13:34:29 UTC, Robert burner 
Schadek wrote:

 All that code is contained in 30 line template, That is by far 
 the best working option anybody could come up with

 I even proposed an alternative that uses type tags instead.
 http://dpaste.dzfl.pl/95fb6a4e086d


 And I showed that it did not work.


I never got a reply on this. Let's please try to solve this 
problem.
Depending on the leakage of version identifiers from client to 
library code (only templates) has severe issues. For example a 
template isn't reinstantiated, if it was already instatiated in 
another module (allinst switch). Also version identifiers work 
globally so you'll probably turn on logging in all libraries you 
use.
http://forum.dlang.org/post/lrf362$tkn$1 digitalmars.com


 - Documentation is out of sync.


 gh-page is yes, give me 15min


Thanks
Oct 24 2014
parent "Martin Nowak" <code dawg.eu> writes:
On Friday, 24 October 2014 at 10:59:43 UTC, Martin Nowak wrote:

 And I showed that it did not work.




Found it 
http://forum.dlang.org/post/hmzfcxlafwlgoovuweak forum.dlang.org
Oct 24 2014
prev sibling next sibling parent reply Martin Nowak <code+news.digitalmars dawg.eu> writes:
On 10/11/2014 03:34 PM, Robert burner Schadek wrote:

 - Why do loggers have to be classes?


 As answered multiply times before, to build log hierarchies.


You can easily create classes from structs, that's what I said 2 lines 
below.


 Add a Logger interface and a loggerObject to wrap structures when 


polymorphic loggers are needed.
Oct 26 2014
parent Martin Nowak <code+news.digitalmars dawg.eu> writes:
On 10/26/2014 11:14 PM, Martin Nowak wrote:


 As answered multiply times before, to build log hierarchies.


 You can easily create classes from structs, that's what I said 2 lines
 below.


And you could also log into a tuple of logger structs without polymorphism.



 Oct 26 2014



prev sibling  parent  Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/11/2014 03:34 PM, Robert burner Schadek wrote:


 As answered multiply times before, to build log hierarchies.


You don't need classes for hierarchies. You need them for runtime 
polymorphism. We're already building complex hierarchies with Ranges, 
same can be done for loggers, see 
http://dpaste.dzfl.pl/2538c3b5d287#line-140.



 Oct 26 2014



prev sibling next sibling parent reply "Dicebot" <public dicebot.lv>  writes:


Will start review round in ~2 days.

I am very sorry for delay :(



 Oct 24 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Friday, 24 October 2014 at 09:53:57 UTC, Dicebot wrote:

 Will start review round in ~2 days.

 I am very sorry for delay :(


No problem the PR has been open since Aug. 2013, one or two weeks 
more or less don't really matter anymore ;)



 Oct 24 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


Jut for the reference, my position on current state of things as 
review manager is this:

I agree with some of mentioned concerns and would like to see 
those fixed. However, I don't think any of those are truly 
critical and this proposal has been hanging there for just too 
long. In my opinion it will be more efficient to resolve any 
remainining issues in follow-up pull requests on case by case 
basis then forcing Robert to do it himself - there will be some 
time left before next release to break a thing or two before 
public statement is made.

Because of that I am going to start voting despite some arguments 
being still in process. I hope that won't cause any tension.



 Oct 25 2014



 next sibling parent  Jacob Carlborg <doob me.com>  writes:


On 2014-10-25 18:43, Dicebot wrote:

 Jut for the reference, my position on current state of things as review
 manager is this:

 I agree with some of mentioned concerns and would like to see those
 fixed. However, I don't think any of those are truly critical and this
 proposal has been hanging there for just too long. In my opinion it will
 be more efficient to resolve any remainining issues in follow-up pull
 requests on case by case basis then forcing Robert to do it himself -
 there will be some time left before next release to break a thing or two
 before public statement is made.

 Because of that I am going to start voting despite some arguments being
 still in process. I hope that won't cause any tension.


As long as the changes don't require a complete redesign. There was some 
talk about using templates instead of classes.

-- 
/Jacob Carlborg



 Oct 26 2014



prev sibling next sibling parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/25/2014 06:43 PM, Dicebot wrote:

 Because of that I am going to start voting despite some arguments being
 still in process. I hope that won't cause any tension.


The dependency on external version identifiers in phobos is still a 
complete bummer and there are still many implementation issues.
One reason why this is taking so long is that there were many issues 
some of which still need to be addressed.



 Oct 26 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 26 October 2014 at 22:12:20 UTC, Martin Nowak wrote:

 On 10/25/2014 06:43 PM, Dicebot wrote:

 Because of that I am going to start voting despite some 
 arguments being
 still in process. I hope that won't cause any tension.


 The dependency on external version identifiers in phobos is 
 still a complete bummer and there are still many implementation 
 issues.
 One reason why this is taking so long is that there were many 
 issues some of which still need to be addressed.


it is not really a dependency as the one template that uses the 
version identifier uses them optionally.

please name the issues that are not because of the gc.



 Oct 26 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 26 October 2014 at 22:27:55 UTC, Robert burner Schadek 
wrote:

 The dependency on external version identifiers in phobos is 
 still a complete bummer


 it is not really a dependency as the one template that uses the 
 version identifier uses them optionally.


And I forgot to add, no better solution presented itself in one 
year.



 Oct 26 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/26/2014 11:29 PM, Robert burner Schadek wrote:

 And I forgot to add, no better solution presented itself in one year.


Well I showed one solution, but reduce it to its essence.
If you allow to define a Logger with a LogLevel know at compile time and 
you statically pass the LogLevel of your message to the logging function 
you can elide that call. For anything else you need a runtime check.

http://dpaste.dzfl.pl/2538c3b5d287



 Oct 26 2014



 next sibling parent  Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 12:45 AM, Martin Nowak wrote:

 but reduce it to its essence.


but let's reduce



 Oct 26 2014



prev sibling next sibling parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 12:45 AM, Martin Nowak wrote:

 If you allow to define a Logger with a LogLevel know at compile time and
 you statically pass the LogLevel of your message to the logging function
 you can elide that call. For anything else you need a runtime check.


You are trying to globally define a LogLevel through the version 
identifier but that collides with D's separate compilation.
So you cannot enable logging in a library that was compiled with 
StdLoggerDisableLogging. And vice versa you cannot statically disable 
logging in a library compiled without StdLoggerDisableLogging.

Now if you use StdLoggerDisableLogging in your program the effect on the 
library will depend on whether or not you're calling a templated 
function or if the compiler inlined certain library function into your 
program.



 Oct 26 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 26 October 2014 at 23:58:14 UTC, Martin Nowak wrote:

 On 10/27/2014 12:45 AM, Martin Nowak wrote:

 If you allow to define a Logger with a LogLevel know at 
 compile time and
 you statically pass the LogLevel of your message to the 
 logging function
 you can elide that call. For anything else you need a runtime 
 check.


 You are trying to globally define a LogLevel through the 
 version identifier but that collides with D's separate 
 compilation.
 So you cannot enable logging in a library that was compiled 
 with StdLoggerDisableLogging. And vice versa you cannot 
 statically disable logging in a library compiled without 
 StdLoggerDisableLogging.

 Now if you use StdLoggerDisableLogging in your program the 
 effect on the library will depend on whether or not you're 
 calling a templated function or if the compiler inlined certain 
 library function into your program.


It is a good think then that the *DisableLogging versions are 
only used inside a template that is used inside a templates. 
Though version statements attached to a phobos compilation should 
only have impact on the unittest of phobos.

Secondly, why would phobos be shipped with certain LogLevel 
disabled.



 Oct 27 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 09:22 AM, Robert burner Schadek wrote:

 It is a good think then that the *DisableLogging versions are only used
 inside a template that is used inside a templates. Though version
 statements attached to a phobos compilation should only have impact on
 the unittest of phobos.

 Secondly, why would phobos be shipped with certain LogLevel disabled.


Well the question is, how can you avoid paying the overhead for logging 
when you don't need it.

Say I want to add tracing/logging to 
[`parseJson`](http://dlang.org/library/std/json/parseJSON.html) or 

have no way to get rid of the logLevel overhead with the version 
identifier and abstract class approach.

When the logLevel is know at CT this overhead can be completely eliminated.
auto json = parseJSON(data, stdoutLogger!(LogLevel.trace)());
auto json = parseJSON(data, stdoutLogger!(LogLevel.warning)());
auto json = parseJSON(data, NullLogger());



 Oct 27 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Monday, 27 October 2014 at 20:42:10 UTC, Martin Nowak wrote:

 Say I want to add tracing/logging to 
 [`parseJson`](http://dlang.org/library/std/json/parseJSON.html) 
 or 



This is exactly what is wrong :) Using std.logger inside Phobos 
itself is a big no. Actually even finding yourself in position 
where you may want to do it indicates some Phobos design issue.



 Oct 27 2014



 next sibling parent  Brad Roberts via Digitalmars-d <digitalmars-d puremagic.com>  writes:


On 10/27/14, 1:49 PM, Dicebot via Digitalmars-d wrote:

 On Monday, 27 October 2014 at 20:42:10 UTC, Martin Nowak wrote:

 Say I want to add tracing/logging to
 [`parseJson`](http://dlang.org/library/std/json/parseJSON.html) or



 This is exactly what is wrong :) Using std.logger inside Phobos itself is a
big no. Actually even
 finding yourself in position where you may want to do it indicates some Phobos
design issue.


Why?  I agree that large parts of phobos have little need for logging, but I
question the general 
statement.  What's so special about phobos that suggests that no part of it
should ever log?  How is 
it different from many other third party libraries that might be similar in
nature?

Consider the std.net.curl.  I find it entirely reasonable to consider that it
would have logging 
added to it.  It could be argued that that part of phobos could/should be
removed, but ignore that 
for now and pretend its a full re-implementation of an http client instead if
that helps.

Consider the scheduler work that Sean is doing.  I'll bet he's got printf's in
there at some 
strategic points for debugging right now.  Most of those I can easily see
translating into trace 
level logging.

The same holds for any higher level tasks that have any sort of complexities
and need a way for 
developers to witness how those subsystems are executing after the fact.

My key point, phobos isn't and shouldn't be considered special here.  If it
shouldn't be doing 
logging, then why and why doesn't that same logic apply to almost every other
library that 
developers are going to use?

I suspect there's some philosophical differences at play.

My 2 cents,
Brad



 Oct 27 2014



prev sibling next sibling parent  Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 09:49 PM, Dicebot wrote:

 This is exactly what is wrong :) Using std.logger inside Phobos itself
 is a big no. Actually even finding yourself in position where you may
 want to do it indicates some Phobos design issue.


Well the same problem also applies to any other library that I obtain as 
binary release.



 Oct 27 2014



prev sibling  parent  "Robert burner Schadek" <rburners gmail.com>  writes:


On Monday, 27 October 2014 at 20:49:35 UTC, Dicebot wrote:

 On Monday, 27 October 2014 at 20:42:10 UTC, Martin Nowak wrote:

 Say I want to add tracing/logging to 
 [`parseJson`](http://dlang.org/library/std/json/parseJSON.html) 
 or 



 This is exactly what is wrong :) Using std.logger inside Phobos 
 itself is a big no. Actually even finding yourself in position 
 where you may want to do it indicates some Phobos design issue.


Yes and No. His Logger can came from the user aka. outside of 
phobos. That might just be valid. But the problem with the design 
is that he needs to accept every possible Logger. And that either 
means template of abstract class Logger.

The problem with a template is:

``` library code that is given in binary form
auto fun(Logger l) {
    return parseJson(getData(), l);
}
```

their is no choice but to pass a class. Meaning you have to wrap 
the struct Logger with a class proxy. And this will properly 
develop into a common theme.

Allowing struct has one design problem IMO:
Either we force to callee to accept Logger as template or
we force the caller to wrap his Logger struct with Logger proxy 
classes.
This is because an abstract class is the lowest common 
denominator in this case.

Anyway, I'm pretty sure that Martin and I will never see eye to 
eye in this discussion. IMO disabling a single Logger through its 
LogLevel at compile (plus all the extra litter and possible 
needed wrapping) is no better than creating NullLogger. He thinks 
the opposite.

The problem for me now is, if I add a struct UFCS overload Martin 
will be happy but somebody else will stream WAT "The struct stuff 
must go"

So please everybody reading this, please give a comment about 
this.



 Oct 27 2014



prev sibling  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 26 October 2014 at 23:45:56 UTC, Martin Nowak wrote:

 On 10/26/2014 11:29 PM, Robert burner Schadek wrote:

 And I forgot to add, no better solution presented itself in 
 one year.


 Well I showed one solution, but reduce it to its essence.
 If you allow to define a Logger with a LogLevel know at compile 
 time and you statically pass the LogLevel of your message to 
 the logging function you can elide that call. For anything else 
 you need a runtime check.

 http://dpaste.dzfl.pl/2538c3b5d287


And again I'm saying fixing the LogLevel at CT is not good 
enough. And the other part of the solution uses class just like 
std.logger.
And the hierarchy you're building is also at CT, which is just 
not gone work, if you don't have ultimate control of all sources.



 Oct 27 2014



  parent  Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 09:08 AM, Robert burner Schadek wrote:

 And again I'm saying fixing the LogLevel at CT is not good enough. And
 the other part of the solution uses class just like std.logger.


Right, a CT LogLevel only works for certain use-cases and I never 
claimed that it works for everything (that's why there is the 
loggerObject adapter). This not an argument to go solely with classes 
though. And you loose an important optimization by not making use of CT 
LogLevels.
Introducing the concepts and providing the loggerObject adapter (structs 
to interface) is a very flexible solution that proved itself very 
successful for ranges.


 And the hierarchy you're building is also at CT, which is just not gone
 work, if you don't have ultimate control of all sources.


What's the problem here, a counterexample would be helpful.

Logger logger;
if (config.alsoLogToFile)
     logger = loggerObject(multiLogger(SysLogger(), FileLogger()));
else
     logger = loggerObject(SysLogger());



 Oct 27 2014



prev sibling  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/26/2014 11:27 PM, Robert burner Schadek wrote:

 it is not really a dependency as the one template that uses the version
 identifier uses them optionally.


It simply doesn't work, e.g. you could not statically disable logging in 
phobos without recompiling phobos.

cat > lib.d << CODE
version (StdLoggerDisableLogging)
     enum isLoggingActive = false;
else
     enum isLoggingActive = true;

void doSome()
{
     import std.stdio;
     writeln("isLoggingActive: ", isLoggingActive);
}
CODE

cat > main.d << CODE
import lib, std.stdio;

void main()
{
     writeln("isLoggingActive: ", isLoggingActive);
     doSome();
}
CODE

dmd -version=StdLoggerDisableLogging -lib lib.d
dmd main lib.a
./main



 Oct 26 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 26 October 2014 at 22:57:51 UTC, Martin Nowak wrote:

 On 10/26/2014 11:27 PM, Robert burner Schadek wrote:

 it is not really a dependency as the one template that uses 
 the version
 identifier uses them optionally.


 It simply doesn't work, e.g. you could not statically disable 
 logging in phobos without recompiling phobos.

 cat > lib.d << CODE
 version (StdLoggerDisableLogging)
     enum isLoggingActive = false;
 else
     enum isLoggingActive = true;

 void doSome()
 {
     import std.stdio;
     writeln("isLoggingActive: ", isLoggingActive);
 }
 CODE

 cat > main.d << CODE
 import lib, std.stdio;

 void main()
 {
     writeln("isLoggingActive: ", isLoggingActive);
     doSome();
 }
 CODE

 dmd -version=StdLoggerDisableLogging -lib lib.d
 dmd main lib.a
 ./main


If it where done this way, yes of course you're right. But it is 
not, please take a look a the source first.



 Oct 27 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 09:28 AM, Robert burner Schadek wrote:

 If it where done this way, yes of course you're right. But it is not,
 please take a look a the source first.


I'm looking at 
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d 
and this is exactly how this works.

Even if we make isLoggingActive a template, the problem persists.

cat > lib.d << CODE
version (StdLoggerDisableLogging)
     enum isLoggingActive() = false;
else
     enum isLoggingActive() = true;

void doSome()
{
     import std.stdio;
     writeln("loggingLib: ", isLoggingActive!());
}
CODE

cat > main.d << CODE
import lib, std.stdio;

void main()
{
     writeln("loggingMain: ", isLoggingActive!());
     doSome();
}
CODE

dmd -version=StdLoggerDisableLogging -lib lib.d
dmd main lib.a
./main
----
logginMain: true
logginLib: false
----
dmd -lib lib.d
dmd -version=StdLoggerDisableLogging main lib.a
./main
----
logginMain: false
logginLib: true
----



 Oct 27 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Monday, 27 October 2014 at 22:27:12 UTC, Martin Nowak wrote:

 On 10/27/2014 09:28 AM, Robert burner Schadek wrote:

 If it where done this way, yes of course you're right. But it 
 is not,
 please take a look a the source first.


 I'm looking at 
 https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d 
 and this is exactly how this works.


take a look at

https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L190
and
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L579

isLoggingActiveAt is instantiated at CT of the log function and 
the version statement inside the first link is evaluated at that 
moment.

Disabling a version at CT of the lib has no consequence to 
compile units that are not compiled with that version statement.

I tested it at 
https://github.com/burner/logger/blob/master/Makefile#L23
run "make info"



 Oct 27 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/28/2014 12:58 AM, Robert burner Schadek wrote:

 Disabling a version at CT of the lib has no consequence to compile units
 that are not compiled with that version statement.


Yes setting a version in my app has no effect on the library, that's the 
problem because I cannot disable logging in a library.
So I'll always have to pay some runtime overhead even though I don't 
want to use logging.



 Oct 27 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 28 October 2014 at 01:37:48 UTC, Martin Nowak wrote:

 On 10/28/2014 12:58 AM, Robert burner Schadek wrote:

 Disabling a version at CT of the lib has no consequence to 
 compile units
 that are not compiled with that version statement.


 Yes setting a version in my app has no effect on the library, 
 that's the problem because I cannot disable logging in a 
 library.
 So I'll always have to pay some runtime overhead even though I 
 don't want to use logging.


Actually, that is only true for LogLevel given to a log call at 
runtime. calls to info, trace etc. are guarded with static if. So 
you're not paying any runtime overhead when calling log functions 
with LogLevel build in their name.



 Oct 28 2014



  parent reply "Martin Nowak" <code dawg.eu>  writes:


On Tuesday, 28 October 2014 at 08:38:50 UTC, Robert burner
Schadek wrote:

 Actually, that is only true for LogLevel given to a log call at 
 runtime. calls to info, trace etc. are guarded with static if.
 So you're not paying any runtime overhead when calling log 
 functions with LogLevel build in their name.


If I cannot change the version identifiers that a library was
compiled with then the static if is always true. So you have to
perform the check at runtime. The only way around that is to make
each library function that performs logging a template, be it by
passing a Logger with CT LogLevel or by turning functions into
template functions so that the version identifiers leak into the
library.
The latter is more fragile because the compiler might omit
instatiating a template or it might inline a function.



 Oct 28 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 28 October 2014 at 09:39:24 UTC, Martin Nowak wrote:

 On Tuesday, 28 October 2014 at 08:38:50 UTC, Robert burner
 Schadek wrote:

 Actually, that is only true for LogLevel given to a log call 
 at runtime. calls to info, trace etc. are guarded with static 
 if.
 So you're not paying any runtime overhead when calling log 
 functions with LogLevel build in their name.


 If I cannot change the version identifiers that a library was
 compiled with then the static if is always true. So you have to
 perform the check at runtime. The only way around that is to 
 make
 each library function that performs logging a template, be it by
 passing a Logger with CT LogLevel or by turning functions into
 template functions so that the version identifiers leak into the
 library.


every log call is a template function or method already.


 The latter is more fragile because the compiler might omit
 instatiating a template or it might inline a function.


omitting the instantiation sound like undefined reference.
I don't see the problem with inlining. 1. there are no log 
functions only log templates. 2. The version statement will 
either leave empty template functions bodies, if(false) { ... } 
template functions bodies, or template function bodies that 
perform regular LogLevel checks on runtime LogLevel.

We're running in circles here. Lets find a new solution that 
allows us to
* dmd -version=StdLoggerDisableLogLevel -- disable a LogLevel at 
CT of user code
* that does not require a static Logger LogLevel
* does not leak version statements into phobos
* does not call log template functions where the LogLevel of the 
version statement is part of the name, or at least yields empty 
bodies for these template functions callls



 Oct 28 2014



  parent reply "Martin Nowak" <code dawg.eu>  writes:


On Tuesday, 28 October 2014 at 10:05:47 UTC, Robert burner 
Schadek wrote:

 On Tuesday, 28 October 2014 at 09:39:24 UTC, Martin Nowak wrote:

 On Tuesday, 28 October 2014 at 08:38:50 UTC, Robert burner
 Schadek wrote:

 Actually, that is only true for LogLevel given to a log call 
 at runtime. calls to info, trace etc. are guarded with static 
 if.
 So you're not paying any runtime overhead when calling log 
 functions with LogLevel build in their name.


 If I cannot change the version identifiers that a library was
 compiled with then the static if is always true. So you have to
 perform the check at runtime. The only way around that is to 
 make
 each library function that performs logging a template, be it 
 by
 passing a Logger with CT LogLevel or by turning functions into
 template functions so that the version identifiers leak into 
 the
 library.


 every log call is a template function or method already.


 The latter is more fragile because the compiler might omit
 instatiating a template or it might inline a function.


 omitting the instantiation sound like undefined reference.
 I don't see the problem with inlining. 1. there are no log 
 functions only log templates. 2. The version statement will 
 either leave empty template functions bodies, if(false) { ... } 
 template functions bodies, or template function bodies that 
 perform regular LogLevel checks on runtime LogLevel.

 We're running in circles here. Lets find a new solution that 
 allows us to
 * dmd -version=StdLoggerDisableLogLevel -- disable a LogLevel 
 at CT of user code
 * that does not require a static Logger LogLevel
 * does not leak version statements into phobos
 * does not call log template functions where the LogLevel of 
 the version statement is part of the name, or at least yields 
 empty bodies for these template functions callls


Yep, let's try that.

I think part of the misunderstanding is that I'm thinking of an 
app as user code plus a number of libraries all on top of phobos. 
Say I have an app using vibe.d and I want to enable logging in my 
app, but disable it in phobos.
Was it even a design goal that logging can be enabled on a per 
library level?



 Oct 28 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 28 October 2014 at 11:11:09 UTC, Martin Nowak wrote:

 Yep, let's try that.

 I think part of the misunderstanding is that I'm thinking of an 
 app as user code plus a number of libraries all on top of 
 phobos. Say I have an app using vibe.d and I want to enable 
 logging in my app, but disable it in phobos.
 Was it even a design goal that logging can be enabled on a per 
 library level?


It is a design goal to disable certain LogLevel at CT of a 
compile unit (CU).
e.g. make all logs to trace function template do nothing



 Oct 28 2014



  parent reply "Martin Nowak" <code dawg.eu>  writes:


On Tuesday, 28 October 2014 at 12:02:16 UTC, Robert burner 
Schadek wrote:

 It is a design goal to disable certain LogLevel at CT of a 
 compile unit (CU).
 e.g. make all logs to trace function template do nothing


One idea to make this working is to use prefixed version 
identifiers.
Obviously this is all boilerplate so it could be done in a mixin 
template in std.log.

module mylib.log;

version (MyLibLogInfo)
     enum logLevel = LogLevel.info;
version (MyLibLogTrace)
     enum logLevel = LogLevel.trace;
version (MyLibLogWarning)
     enum logLevel = LogLevel.warning;
version (MyLibLogError)
     enum logLevel = LogLevel.error;

static if (is(typeof(logLevel)))
{
     // wrapper around Logger interface/class for CT logLevel
     static struct FixedLogLevel
     {
         enum logLevel = logLevel;
         Logger impl;
         alias impl this;
     }

     void setLogger(Logger logger)
     {
         logger.impl = logger;
     }

     private __gshared FixedLogLevel logger;
}

module mylib.foo;

void foo()
{
     import mylib.log;
     logger.info("bla");
     logger.error("wat");
}

module client.bar;

void bar()
{
     import mylib.log, mylib.foo;
     auto logger = new FileLogger("log.txt", LogLevel.warning);
     setLogger(logger);
     foo(); // CT check for LogLevel
     logger.info("runtime check whether info logging is on");
}



 Oct 28 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/28/2014 07:22 PM, Martin Nowak wrote:

 On Tuesday, 28 October 2014 at 12:02:16 UTC, Robert burner Schadek wrote:

 It is a design goal to disable certain LogLevel at CT of a compile
 unit (CU).
 e.g. make all logs to trace function template do nothing


 One idea to make this working is to use prefixed version identifiers.
 Obviously this is all boilerplate so it could be done in a mixin
 template in std.log.


2nd iteration of that idea.

cat > main.d << CODE
import std_logger;

LoggerCT!"MyApp" appLogger = new StdoutLogger(LogLevel.info);
LoggerCT!"MyLib" libLogger = new StdoutLogger(LogLevel.trace);

void main()
{
     appLogger.log!(LogLevel.info)("app");
     appLogger.log!(LogLevel.warning)("app");
     libLogger.log!(LogLevel.info)("lib");
     libLogger.log!(LogLevel.warning)("lib");

     Logger rtLogger = appLogger;
     rtLogger.log!(LogLevel.info)("rt");
     rtLogger.log!(LogLevel.warning)("rt");
}
CODE

cat > std_logger.d << CODE
enum LogLevel { all, info, trace, warning, error, none }

template minLogLevel(string prefix)
{
     mixin("
     version ("~prefix~"LogAll)
         enum minLogLevel = LogLevel.all;
     else version ("~prefix~"LogInfo)
         enum minLogLevel = LogLevel.info;
     else version ("~prefix~"LogTrace)
         enum minLogLevel = LogLevel.trace;
     else version ("~prefix~"LogWarning)
         enum minLogLevel = LogLevel.warning;
     else version ("~prefix~"LogError)
         enum minLogLevel = LogLevel.error;
     else version ("~prefix~"LogNone)
         enum minLogLevel = LogLevel.none;
     else
         enum minLogLevel = LogLevel.all;
     ");
}

interface Logger
{
      property LogLevel logLevel();
     void write(LogLevel ll, string msg);
}

class StdoutLogger : Logger
{
     this(LogLevel l) { _logLevel = l; }
     LogLevel _logLevel;
      property LogLevel logLevel() { return _logLevel; }
     void write(LogLevel ll, string msg) { import std.stdio; writeln(ll, 
": ", msg); }
}

/// used for library/app specific version prefixes
struct LoggerCT(string prefix)
{
     this(Logger logger) { impl = logger; }
     enum versionPrefix = prefix;
     Logger impl;
     alias impl this;
}

void log(LogLevel ll)(Logger logger, string msg)
{
     if (ll >= logger.logLevel) // runtime check
         logger.write(ll, msg);
}

/// when using a logger with prefix
void log(LogLevel ll, L:LoggerCT!pfx, string pfx)(auto ref L logger, 
string msg)
     if (ll < minLogLevel!pfx)
{
}

void log(LogLevel ll, L:LoggerCT!pfx, string pfx)(auto ref L logger, 
string msg)
     if (ll >= minLogLevel!pfx)
{
     if (ll >= logger.logLevel) // additional runtime check, because 
logger might require a higher log level
         logger.write(ll, msg);
}
CODE

dmd std_logger -run main

dmd -version=MyAppLogWarning std_logger.d -run main
dmd -version=MyAppLogError -version=MyLibLogNone std_logger.d -run main



 Oct 28 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 28 October 2014 at 22:03:18 UTC, Martin Nowak wrote:

 On 10/28/2014 07:22 PM, Martin Nowak wrote:

 On Tuesday, 28 October 2014 at 12:02:16 UTC, Robert burner 
 Schadek wrote:

 It is a design goal to disable certain LogLevel at CT of a 
 compile
 unit (CU).
 e.g. make all logs to trace function template do nothing


 One idea to make this working is to use prefixed version 
 identifiers.
 Obviously this is all boilerplate so it could be done in a 
 mixin
 template in std.log.


 2nd iteration of that idea.

 cat > main.d << CODE
 import std_logger;

 LoggerCT!"MyApp" appLogger = new StdoutLogger(LogLevel.info);
 LoggerCT!"MyLib" libLogger = new StdoutLogger(LogLevel.trace);

 void main()
 {
     appLogger.log!(LogLevel.info)("app");
     appLogger.log!(LogLevel.warning)("app");
     libLogger.log!(LogLevel.info)("lib");
     libLogger.log!(LogLevel.warning)("lib");

     Logger rtLogger = appLogger;
     rtLogger.log!(LogLevel.info)("rt");
     rtLogger.log!(LogLevel.warning)("rt");
 }
 CODE

 cat > std_logger.d << CODE
 enum LogLevel { all, info, trace, warning, error, none }

 template minLogLevel(string prefix)
 {
     mixin("
     version ("~prefix~"LogAll)
         enum minLogLevel = LogLevel.all;
     else version ("~prefix~"LogInfo)
         enum minLogLevel = LogLevel.info;
     else version ("~prefix~"LogTrace)
         enum minLogLevel = LogLevel.trace;
     else version ("~prefix~"LogWarning)
         enum minLogLevel = LogLevel.warning;
     else version ("~prefix~"LogError)
         enum minLogLevel = LogLevel.error;
     else version ("~prefix~"LogNone)
         enum minLogLevel = LogLevel.none;
     else
         enum minLogLevel = LogLevel.all;
     ");
 }

 interface Logger
 {
      property LogLevel logLevel();
     void write(LogLevel ll, string msg);
 }

 class StdoutLogger : Logger
 {
     this(LogLevel l) { _logLevel = l; }
     LogLevel _logLevel;
      property LogLevel logLevel() { return _logLevel; }
     void write(LogLevel ll, string msg) { import std.stdio; 
 writeln(ll, ": ", msg); }
 }

 /// used for library/app specific version prefixes
 struct LoggerCT(string prefix)
 {
     this(Logger logger) { impl = logger; }
     enum versionPrefix = prefix;
     Logger impl;
     alias impl this;
 }

 void log(LogLevel ll)(Logger logger, string msg)
 {
     if (ll >= logger.logLevel) // runtime check
         logger.write(ll, msg);
 }

 /// when using a logger with prefix
 void log(LogLevel ll, L:LoggerCT!pfx, string pfx)(auto ref L 
 logger, string msg)
     if (ll < minLogLevel!pfx)
 {
 }

 void log(LogLevel ll, L:LoggerCT!pfx, string pfx)(auto ref L 
 logger, string msg)
     if (ll >= minLogLevel!pfx)
 {
     if (ll >= logger.logLevel) // additional runtime check, 
 because logger might require a higher log level
         logger.write(ll, msg);
 }
 CODE

 dmd std_logger -run main

 dmd -version=MyAppLogWarning std_logger.d -run main
 dmd -version=MyAppLogError -version=MyLibLogNone std_logger.d 
 -run main


That can actually be added to the current state of std.logger 
without breaking any api. The string mixin, version string 
matching isn't really pretty, but it gets the job done. Anyway 
IMO your approach presented here and my approach can go hand in 
hang. Yours should be propagated as the idiomatic way and if you 
really need the crowbar, which you need sometimes, use 
StdLoggerDisableXXXXX.



 Oct 29 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


The reason for the crowbar sometimes you need to disable all 
calls to the Logger or any calls to a specific LogLevel in the 
compile unit, even for Logger not wrapped in LoggerCT.



 Oct 29 2014



  parent  "Martin Nowak" <code dawg.eu>  writes:


On Wednesday, 29 October 2014 at 23:16:28 UTC, Robert burner 
Schadek wrote:

 The reason for the crowbar sometimes you need to disable all 
 calls to the Logger or any calls to a specific LogLevel in the 
 compile unit, even for Logger not wrapped in LoggerCT.


Only that there is no clean boarder between modules because of 
inlining and templates.



 Nov 16 2014



prev sibling next sibling parent  "Martin Nowak" <code dawg.eu>  writes:


On Wednesday, 29 October 2014 at 23:11:44 UTC, Robert burner 
Schadek wrote:

 That can actually be added to the current state of std.logger 
 without breaking any api.


Just a few additions to handle LoggerCT (which needs a better 
name).


 Anyway IMO your approach presented here and my approach can go 
 hand in hang. Yours should be propagated as the idiomatic way 
 and if you really need the crowbar, which you need sometimes, 
 use StdLoggerDisableXXXXX.


Yes, that would work for me.
It's just important to control logging on a per library basis.



 Nov 16 2014



prev sibling  parent  Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/30/2014 12:11 AM, Robert burner Schadek wrote:

 That can actually be added to the current state of std.logger without
 breaking any api. The string mixin, version string matching isn't really
 pretty, but it gets the job done. Anyway IMO your approach presented
 here and my approach can go hand in hang. Yours should be propagated as
 the idiomatic way and if you really need the crowbar, which you need
 sometimes, use StdLoggerDisableXXXXX.


I just found a very compelling solution to the LogLevel disabling problem.

Basically we can declare a logLevel for each module or package and let 
the logger do a reverse look up.

```d
module mymod;

import logger;

// can be anything that converts to a LogLevel
// calls with lower log level can be optimized away if this is a compile 
time constant
// runtime values also work nicely but incur a small overhead
// if this declaration is not present, logLevel from the package is used 
or a default LogLevel.
enum logLevel = LogLevel.critical;

void foo()
{
     info("information from foo"); // optimized out
     fatal("error"); // works
}
```

https://gist.github.com/MartinNowak/443f11aa017d14007c35

There is a tiny gotcha, this works because the logger module imports the 
callee module. So we'd need to avoid module constructors in the logger 
module or use a workaround.



 Dec 03 2014



prev sibling  parent reply "Dicebot" <public dicebot.lv>  writes:


On Tuesday, 28 October 2014 at 22:03:18 UTC, Martin Nowak wrote:

 2nd iteration of that idea.


For me it looks like the cure is worse than the disease. Simply 
not distributing precompiled libraries with log level 
force-reduced (or at least not exposing it in provided .di files) 
feels like a more practical approach than proposed API 
complication.



 Oct 31 2014



 next sibling parent reply "Dicebot" <public dicebot.lv>  writes:


Bringing this back to the first page



 Nov 01 2014



  parent reply "David Nadlinger" <code klickverbot.at>  writes:


There is still a critical issue with std.experimental.logger that 
would prevent it from being merged right now: The abuse of 
 trusted all over the code.

For example, the following piece of code compiles with 
burner/logger c87e1032:
---
import std.experimental.logger.core;

struct Dangerous {
     string toString() {
         *(cast(int*)0xdeadbeef) = 0xcafebabe;
         return null;
     }
}

void main()  safe {
     logf("%s", Dangerous());
}
---

To be quite honest, I'm rather disappointed that I was able to 
break  safe-ty
with the first, most straightforward piece of code I tried. Sure, 
many people don't understand how dangerous  trusted applied to 
templates really is. But this issue has already been pointed out 
multiple times in Robert's code, both in this pull request and 
other unrelated ones.

David



 Nov 01 2014



 next sibling parent  "Dicebot" <public dicebot.lv>  writes:


On Saturday, 1 November 2014 at 17:35:37 UTC, David Nadlinger 
wrote:

 There is still a critical issue with std.experimental.logger 
 that would prevent it from being merged right now: The abuse of 
  trusted all over the code.

 For example, the following piece of code compiles with 
 burner/logger c87e1032:
 ---
 import std.experimental.logger.core;

 struct Dangerous {
     string toString() {
         *(cast(int*)0xdeadbeef) = 0xcafebabe;
         return null;
     }
 }

 void main()  safe {
     logf("%s", Dangerous());
 }
 ---

 To be quite honest, I'm rather disappointed that I was able to 
 break  safe-ty
 with the first, most straightforward piece of code I tried. 
 Sure, many people don't understand how dangerous  trusted 
 applied to templates really is. But this issue has already been 
 pointed out multiple times in Robert's code, both in this pull 
 request and other unrelated ones.

 David


Oh, I have actually completely missed that. It is critical 
indeed. Will try having a look myself and possibly providing 
patches.



 Nov 01 2014



prev sibling next sibling parent reply "Jakob Ovrum" <jakobovrum gmail.com>  writes:


On Saturday, 1 November 2014 at 17:35:37 UTC, David Nadlinger 
wrote:

 There is still a critical issue with std.experimental.logger 
 that would prevent it from being merged right now: The abuse of 
  trusted all over the code.


Thank you. I was afraid I'd have to harp on it for the fourth 
time...



 Nov 01 2014



  parent  "Dicebot" <public dicebot.lv>  writes:


On Sunday, 2 November 2014 at 00:29:10 UTC, Jakob Ovrum wrote:

 On Saturday, 1 November 2014 at 17:35:37 UTC, David Nadlinger 
 wrote:

 There is still a critical issue with std.experimental.logger 
 that would prevent it from being merged right now: The abuse 
 of  trusted all over the code.


 Thank you. I was afraid I'd have to harp on it for the fourth 
 time...


Sorry, it is really hard to follow important bits in many 
discussions spawned :( I'll take a go at  safe-ty issue 
personally this time.



 Nov 02 2014



prev sibling  parent reply "Dicebot" <public dicebot.lv>  writes:


I had initial go at cleaning safety attributes : 
https://github.com/Dicebot/phobos/tree/logger-fix-trust

It still needs one druntime PR merged 
(https://github.com/D-Programming-Language/druntime/pull/1009) to 
be done properly but should be ready for destruction.

David snipet results in this after all changes:
std/experimental/logger/core.d(1675): Error: safe function 
'std.experimental.logger.core.Logger.logf!(11, "aaa.d", 
"aaa.main", "void aaa.main()  safe", "aaa", Dangerous).logf' 
cannot call system function 'std.format.formattedWrite!(MsgRange, 
char, Dangerous).formattedWrite'



 Nov 02 2014



  parent reply "David Nadlinger" <code klickverbot.at>  writes:


On Sunday, 2 November 2014 at 13:06:24 UTC, Dicebot wrote:

 I had initial go at cleaning safety attributes : 
 https://github.com/Dicebot/phobos/tree/logger-fix-trust

 It still needs one druntime PR merged 
 (https://github.com/D-Programming-Language/druntime/pull/1009) 
 to be done properly but should be ready for destruction.

 David snipet results in this after all changes:
 std/experimental/logger/core.d(1675): Error: safe function 
 'std.experimental.logger.core.Logger.logf!(11, "aaa.d", 
 "aaa.main", "void aaa.main()  safe", "aaa", Dangerous).logf' 
 cannot call system function 
 'std.format.formattedWrite!(MsgRange, char, 
 Dangerous).formattedWrite'


I don't recall off the top of my head some non-template innards 
actually might require  safe, but apart from that, why not just 
leave the job to template attribute inference entirely? If 
somebody wants to log a type with a  system toString in non- safe 
code, why not just let them?

David



 Nov 02 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


 I don't recall off the top of my head some non-template innards 
 actually might require  safe, but apart from that, why not just 
 leave the job to template attribute inference entirely? If 
 somebody wants to log a type with a  system toString in 
 non- safe code, why not just let them?

 David


In my opinion inference is better choice for small building 
blocks (like algorithms). For complete system like logging API 
forcing  safe makes more sense as whatever its internals are, 
exposed API should never be  system



 Nov 02 2014



  parent reply "David Nadlinger" <code klickverbot.at>  writes:


On Sunday, 2 November 2014 at 18:29:09 UTC, Dicebot wrote:

 In my opinion inference is better choice for small building 
 blocks (like algorithms). For complete system like logging API 
 forcing  safe makes more sense as whatever its internals are, 
 exposed API should never be  system


This isn't about the library internals. These should be presented 
in a safe way, of course. The point here is that you restrict 
what your users can do by forcing templates to be  safe.

Imagine somebody has a type that cannot be  trusted because of 
whatever reason. Maybe because it's legacy code, maybe it uses 
resources it does not manage, … If you forcibly make logf  safe, 
then this type cannot be used with logf without some crazy 
workaround (simply using to!string might produce an unneeded 
allocation if the type uses the sink-delegate signature for 
toString).

Why not leave this up to the compiler and support more use cases 
without degrading the experience for  safe clients?

David



 Nov 02 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Sunday, 2 November 2014 at 18:42:20 UTC, David Nadlinger wrote:

 Imagine somebody has a type that cannot be  trusted because of 
 whatever reason. Maybe because it's legacy code, maybe it uses 
 resources it does not manage, … If you forcibly make logf 
  safe, then this type cannot be used with logf without some 
 crazy workaround (simply using to!string might produce an 
 unneeded allocation if the type uses the sink-delegate 
 signature for toString).

 Why not leave this up to the compiler and support more use 
 cases without degrading the experience for  safe clients?

 David


You mean something like user type toString() which is 
legitimately  system and can't be made  trusted? Yes, this makes 
sense. Will need to also add tests for that.

Consider me convinced :)



 Nov 02 2014



 next sibling parent  Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 11/2/14 10:07 PM, Dicebot wrote:

 On Sunday, 2 November 2014 at 18:42:20 UTC, David Nadlinger wrote:

 Imagine somebody has a type that cannot be  trusted because of
 whatever reason. Maybe because it's legacy code, maybe it uses
 resources it does not manage, … If you forcibly make logf  safe, then
 this type cannot be used with logf without some crazy workaround
 (simply using to!string might produce an unneeded allocation if the
 type uses the sink-delegate signature for toString).

 Why not leave this up to the compiler and support more use cases
 without degrading the experience for  safe clients?

 David


 You mean something like user type toString() which is legitimately
  system and can't be made  trusted? Yes, this makes sense. Will need to
 also add tests for that.

 Consider me convinced :)


Fantastic, thanks! -- Andrei



 Nov 03 2014



prev sibling  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


I will remove the trusted later this week



 Nov 03 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Monday, 3 November 2014 at 20:41:14 UTC, Robert burner Schadek 
wrote:

 I will remove the trusted later this week


After few tweaks (to allows  system toString()) in my branch you 
should be able to just merge it directly to your PR branch if you 
are ok with proposed changes.



 Nov 03 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


And back to the frontpage.

Martin / Robert, have you managed to come to an agreement on 
conditional level thing?



 Nov 09 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 9 November 2014 at 21:42:09 UTC, Dicebot wrote:

 And back to the frontpage.

 Martin / Robert, have you managed to come to an agreement on 
 conditional level thing?


After your last reply we haven't done anything else.

I would merge his idea, but there is pushback from your side. 
Anyway, this idea would not break the api. So I don't think it is 
a blocking change, IMO.



 Nov 10 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Monday, 10 November 2014 at 11:46:34 UTC, Robert burner 
Schadek wrote:

 After your last reply we haven't done anything else.

 I would merge his idea, but there is pushback from your side. 
 Anyway, this idea would not break the api. So I don't think it 
 is a blocking change, IMO.


kk, I'll try contacting him via e-mail if Martin does not return 
to this thread soon enough :) This proposal may not break API 
directly but it greatly affects how derivative libraries are 
supposed to be designed and distributed so I'd prefer to nail it 
down immediately.

There are also some more  safe changes need, I hope to provide 
another branch with those soon enough (~tomorrow)



 Nov 10 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Monday, 10 November 2014 at 17:03:31 UTC, Dicebot wrote:

 On Monday, 10 November 2014 at 11:46:34 UTC, Robert burner 
 Schadek wrote:

 After your last reply we haven't done anything else.

 I would merge his idea, but there is pushback from your side. 
 Anyway, this idea would not break the api. So I don't think it 
 is a blocking change, IMO.


 kk, I'll try contacting him via e-mail if Martin does not 
 return to this thread soon enough :) This proposal may not 
 break API directly but it greatly affects how derivative 
 libraries are supposed to be designed and distributed so I'd 
 prefer to nail it down immediately.


I get that


 There are also some more  safe changes need, I hope to provide 
 another branch with those soon enough (~tomorrow)


thank you



 Nov 10 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Monday, 10 November 2014 at 18:32:03 UTC, Robert burner 
Schadek wrote:

 On Monday, 10 November 2014 at 17:03:31 UTC, Dicebot wrote:

 There are also some more  safe changes need, I hope to provide 
 another branch with those soon enough (~tomorrow)


 thank you


https://github.com/Dicebot/phobos/tree/logger-safety

I think that should be it for now



 Nov 11 2014



  parent reply "Jose" <jsancio gmail.com>  writes:


On Tuesday, 11 November 2014 at 15:06:49 UTC, Dicebot wrote:

 https://github.com/Dicebot/phobos/tree/logger-safety

 I think that should be it for now


I could have completely missed some details but I took sometime 
to look at the code after reading that the logger was holding a 
lock during the write operation. I noticed the following lines.

One shared Logger:
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L1696

One global function that references that shared Logger:
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L292

And more importantly a Mutex that is always acquired when writing:
https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L1035

Does this mean that if I use this library on a machine that has 
32 cores, 100s of threads and 1000s of fiber only one 
thread/fiber can write at a time no matter the concrete 
implementation of my logger? Am I missing something or isn't this 
a non-starter?

Thanks,
-Jose



 Nov 11 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Wednesday, 12 November 2014 at 05:36:40 UTC, Jose wrote:

 On Tuesday, 11 November 2014 at 15:06:49 UTC, Dicebot wrote:

 https://github.com/Dicebot/phobos/tree/logger-safety


 One shared Logger:
 https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L1696

 One global function that references that shared Logger:
 https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L292

 And more importantly a Mutex that is always acquired when 
 writing:
 https://github.com/burner/phobos/blob/logger/std/experimental/logger/core.d#L1035

 Does this mean that if I use this library on a machine that has 
 32 cores, 100s of threads and 1000s of fiber only one 
 thread/fiber can write at a time no matter the concrete 
 implementation of my logger? Am I missing something or isn't 
 this a non-starter?

 Thanks,
 -Jose


Only one thread can write to one Logger at a time, also known as 
synchronization. Anything else is properly wrong. But you can 
have as many Logger as you have memory.



 Nov 12 2014



 next sibling parent reply "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?=  writes:


On Wednesday, 12 November 2014 at 12:39:24 UTC, Robert burner 
Schadek wrote:

 Only one thread can write to one Logger at a time, also known 
 as synchronization. Anything else is properly wrong. But you 
 can have as many Logger as you have memory.


Taking a lock when the logging call doesn't flush to disk sounds 
rather expensive.



 Nov 12 2014



  parent  "Robert burner Schadek" <rburners gmail.com>  writes:


On Wednesday, 12 November 2014 at 15:05:29 UTC, Ola Fosheim 
Grøstad wrote:

 On Wednesday, 12 November 2014 at 12:39:24 UTC, Robert burner 
 Schadek wrote:

 Only one thread can write to one Logger at a time, also known 
 as synchronization. Anything else is properly wrong. But you 
 can have as many Logger as you have memory.


 Taking a lock when the logging call doesn't flush to disk 
 sounds rather expensive.


If you log to a FileLogger it will flush. It is just a costly 
thing to do. There is no way get the work done but doing it.



 Nov 13 2014



prev sibling  parent reply "Dicebot" <public dicebot.lv>  writes:


On Wednesday, 12 November 2014 at 12:39:24 UTC, Robert burner 
Schadek wrote:

 Only one thread can write to one Logger at a time, also known 
 as synchronization. Anything else is properly wrong. But you 
 can have as many Logger as you have memory.


One thing we can improve is to use thread-local stdlog singletons 
(that forward to global shared one by default) instead of using 
shared one as entry point. In server applications one would 
typically want to have logs buffered per thread or even sent to 
remote process / machine in parallel - while this can be done 
right now it would require applications to resort from using 
stdlog completely.

Better approach that comes to my mind is to have both 
thread-local and global configurable stdlog and have means to 
explicitly capture lock on global one from local proxies for 
optimized bulk logging.



 Nov 12 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


IMO this defeats the design goal off having the default case very 
easy and just working. Therefore, I think thread local global 
Logger and how to make them interact is something that should be 
left to the advanced user.



 Nov 13 2014



 next sibling parent  "Dicebot" <public dicebot.lv>  writes:


On Thursday, 13 November 2014 at 19:59:21 UTC, Robert burner 
Schadek wrote:

 IMO this defeats the design goal off having the default case 
 very easy and just working. Therefore, I think thread local 
 global Logger and how to make them interact is something that 
 should be left to the advanced user.


How so? For casual end user hardly anything changes.

Right now there is a single shared stdlog singleton 
(configurable). I propose to rename it to something like 
stdlog_shared and provide thread-local stdlog singletons which by 
default will simply forward calls to stdlog_shared. That way 
default case will work exactly like it does now but it will make 
possible for advanced user to implement bulk loggers while still 
using same `stdlog` entry point (which is crucial in my opinion)



 Nov 14 2014



prev sibling  parent reply "David Nadlinger" <code klickverbot.at>  writes:


On Thursday, 13 November 2014 at 19:59:21 UTC, Robert burner 
Schadek wrote:

 Therefore, I think thread local global Logger and how to make 
 them interact is something that should be left to the advanced 
 user.


Except that they can't actually get rid of all the overhead 
involved, as the locking is hard-coded into the Logger 
base-class. Granted, acquiring an uncontended lock isn't terribly 
expensive, but it's still a noticeable overhead if you are just 
logging to a thread-local buffer otherwise.

David



 Nov 14 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Friday, 14 November 2014 at 21:43:53 UTC, David Nadlinger 
wrote:

 Except that they can't actually get rid of all the overhead 
 involved, as the locking is hard-coded into the Logger 
 base-class. Granted, acquiring an uncontended lock isn't 
 terribly expensive, but it's still a noticeable overhead if you 
 are just logging to a thread-local buffer otherwise.

 David


You can always roll your own non locking Logger, but the default 
should be thread-safe.



 Nov 14 2014



  parent reply "David Nadlinger" <code klickverbot.at>  writes:


On Friday, 14 November 2014 at 21:46:00 UTC, Robert burner 
Schadek wrote:

 You can always roll your own non locking Logger, but the 
 default should be thread-safe.


You can't, since you need to inherit from Logger, which already 
does the locking for you in a way that's not overridable. Also, I 
take it you are familiar with the fact that locks aren't the only 
technique for achieving cross-thread synchronization.

David



 Nov 14 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Friday, 14 November 2014 at 21:49:09 UTC, David Nadlinger 
wrote:

 On Friday, 14 November 2014 at 21:46:00 UTC, Robert burner 
 Schadek wrote:

 You can always roll your own non locking Logger, but the 
 default should be thread-safe.


 You can't, since you need to inherit from Logger, which already 
 does the locking for you in a way that's not overridable. Also,


My bad, I mixed something up.


 I take it you are familiar with the fact that locks aren't the 
 only technique for achieving cross-thread synchronization.


Yes, but this way allows to add structured logging in an easy way.



 Nov 14 2014



  parent reply "David Nadlinger" <code klickverbot.at>  writes:


On Friday, 14 November 2014 at 22:18:41 UTC, Robert burner 
Schadek wrote:

 Yes, but this way allows to add structured logging in an easy 
 way.


What.



 Nov 14 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Friday, 14 November 2014 at 23:40:21 UTC, David Nadlinger 
wrote:

 On Friday, 14 November 2014 at 22:18:41 UTC, Robert burner 
 Schadek wrote:

 Yes, but this way allows to add structured logging in an easy 
 way.


 What.


The actual log call is split into multiple parts so that the user 
can overload logMsgPart and add, for instance, structured 
logging. In order to not have races with the default impl locking 
was added.



 Nov 14 2014



  parent  Marco Leise <Marco.Leise gmx.de>  writes:


Am Sat, 15 Nov 2014 02:11:34 +0000
schrieb "Robert burner Schadek" <rburners gmail.com>:


 On Friday, 14 November 2014 at 23:40:21 UTC, David Nadlinger 
 wrote:

 On Friday, 14 November 2014 at 22:18:41 UTC, Robert burner 
 Schadek wrote:

 Yes, but this way allows to add structured logging in an easy 
 way.


 What.


 
 The actual log call is split into multiple parts so that the user 
 can overload logMsgPart and add, for instance, structured 
 logging. In order to not have races with the default impl locking 
 was added.


Somehow I feel like I should be in the firing line here, hehe.
Yes, this is approach to thread-safety isn't the most efficient
imaginable. But it can guarantee thread-safety no matter how
little or much you know about the default logger internals or
what you plan to do with it. The lock also ensures that
public properties like logLevel cannot change while a logger
is working, which could result in log messages being dropped
that should have passed and - as Robert mentioned - ensures
that the 3 structural logging methods are called in order with
no other thread intervening.
I believe it will hit a sweet spot between safe extensibility
and performance.
Benchmarking... Hmm, you can take an uncontested lock
~14.000.000 times per second on Linux   2 Ghz with D's
synchronized(...) {} statement and you can append "The quick
brown fox jumps over the lazy dog." ~22.400.000 to an
Appender!(string[]). I begin to understand what you mean by
"noticeable". Somehow I hope that people don't log that much
or that the loggers themselves are sufficiently complex that
they take their time to process each message. But for a
thread-local logger that just appends to a list, it is quite
a bit of unnecessary overhead.

-- 
Marco



 Nov 18 2014



prev sibling  parent reply "Martin Nowak" <code dawg.eu>  writes:


On Friday, 14 November 2014 at 21:49:09 UTC, David Nadlinger 
wrote:

 On Friday, 14 November 2014 at 21:46:00 UTC, Robert burner 
 Schadek wrote:

 You can always roll your own non locking Logger, but the 
 default should be thread-safe.


 You can't, since you need to inherit from Logger, which already 
 does the locking for you in a way that's not overridable. Also, 
 I take it you are familiar with the fact that locks aren't the 
 only technique for achieving cross-thread synchronization.


Have you guys resolved this? If we cannot implement a non-locking 
logger than that's a blocking issue for the library.



 Nov 29 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Saturday, 29 November 2014 at 14:15:12 UTC, Martin Nowak wrote:

 On Friday, 14 November 2014 at 21:49:09 UTC, David Nadlinger 
 wrote:

 On Friday, 14 November 2014 at 21:46:00 UTC, Robert burner 
 Schadek wrote:

 You can always roll your own non locking Logger, but the 
 default should be thread-safe.


 You can't, since you need to inherit from Logger, which 
 already does the locking for you in a way that's not 
 overridable. Also, I take it you are familiar with the fact 
 that locks aren't the only technique for achieving 
 cross-thread synchronization.


 Have you guys resolved this? If we cannot implement a 
 non-locking logger than that's a blocking issue for the library.


Yes, there is a lock free, thread local indirection now. That can 
be used to build a lock free, thread local logger.

p.s. You should have taken the phun



 Nov 29 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 11/29/2014 05:25 PM, Robert burner Schadek wrote:

 Yes, there is a lock free, thread local indirection now. That can be
 used to build a lock free, thread local logger.

 p.s. You should have taken the phun


I commented on the relevant commit.
https://github.com/burner/phobos/commit/8a3aad5df5218bd995d4679f9b59a59909969b52#diff-59d32a64bcbd4492ff85f10091d73f5fR289

Let me propose a light-weight alternative solution.

```d
abstract class Logger
{
     Object.Monitor mutex;

     this()
     {
         this.mutex = createMutex();
     }

     Object.Monitor createMutex()
     {
         import core.sync.mutex;
         return new Mutex;
     }
}
```

This allows people to override the mutex with whatever fits their bill, 
e.g. one that supports fiber suspension. Especially they can do this.

Object.Monitor createMutex()
{
     static class NoLock : Object.Monitor
     {
         void lock() nothrow {}
         void unlock() nothrow {}
     }
     return new NoLock;
}



 Nov 29 2014



  parent  "Robert burner Schadek" <rburners gmail.com>  writes:


On Saturday, 29 November 2014 at 19:18:01 UTC, Martin Nowak wrote:

 On 11/29/2014 05:25 PM, Robert burner Schadek wrote:

 Yes, there is a lock free, thread local indirection now. That 
 can be
 used to build a lock free, thread local logger.

 p.s. You should have taken the phun


 I commented on the relevant commit.
 https://github.com/burner/phobos/commit/8a3aad5df5218bd995d4679f9b59a59909969b52#diff-59d32a64bcbd4492ff85f10091d73f5fR289


I missed that final.


 Let me propose a light-weight alternative solution.

 ```d
 abstract class Logger
 {
     Object.Monitor mutex;

     this()
     {
         this.mutex = createMutex();
     }

     Object.Monitor createMutex()
     {
         import core.sync.mutex;
         return new Mutex;
     }
 }
 ```

 This allows people to override the mutex with whatever fits 
 their bill, e.g. one that supports fiber suspension. Especially 
 they can do this.

 Object.Monitor createMutex()
 {
     static class NoLock : Object.Monitor
     {
         void lock() nothrow {}
         void unlock() nothrow {}
     }
     return new NoLock;
 }


I will add this and remove the final.

Thank you



 Nov 30 2014



prev sibling  parent reply Marco Leise <Marco.Leise gmx.de>  writes:


Am Wed, 12 Nov 2014 17:56:06 +0000
schrieb "Dicebot" <public dicebot.lv>:


 [=E2=80=A6] have means to=20
 explicitly capture lock on global one from local proxies for=20
 optimized bulk logging.


That's certainly something that occurred to me when talking
with Manu about logging. A locked bulk transfer could
be added, maybe replacing the single element transfer function
that exists already. Thread local loggers that accumulate logs
and pass them on to a global logger seem to be a well known
use case.

--=20
Marco



 Nov 13 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Thursday, 13 November 2014 at 22:13:53 UTC, Marco Leise wrote:

 Am Wed, 12 Nov 2014 17:56:06 +0000
 schrieb "Dicebot" <public dicebot.lv>:


 […] have means to explicitly capture lock on global one from 
 local proxies for optimized bulk logging.


 That's certainly something that occurred to me when talking
 with Manu about logging. A locked bulk transfer could
 be added, maybe replacing the single element transfer function
 that exists already. Thread local loggers that accumulate logs
 and pass them on to a global logger seem to be a well known
 use case.


One bad thing about that is that the global log is no longer 
sorted by time if you write to any file. That would make using 
the log much more difficult IMO.



 Nov 13 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Thursday, 13 November 2014 at 22:19:55 UTC, Robert burner
Schadek wrote:

 One bad thing about that is that the global log is no longer 
 sorted by time if you write to any file. That would make using 
 the log much more difficult IMO.


We do have timestamp as part of recorded log data, don't we?



 Nov 14 2014



  parent reply Steven Schveighoffer <schveiguy yahoo.com>  writes:


On 11/14/14 4:22 PM, Dicebot wrote:

 On Thursday, 13 November 2014 at 22:19:55 UTC, Robert burner
 Schadek wrote:

 One bad thing about that is that the global log is no longer sorted by
 time if you write to any file. That would make using the log much more
 difficult IMO.


 We do have timestamp as part of recorded log data, don't we?


I'm not following this thread, but please please please -- output log 
data in chronological order. If you have to have another object that 
does it, fine. But I don't want to have to rely on sorting utilities to 
properly read a log file.

-Steve



 Nov 14 2014



 next sibling parent reply "Dicebot" <public dicebot.lv>  writes:


On Friday, 14 November 2014 at 22:02:30 UTC, Steven Schveighoffer 
wrote:

 On 11/14/14 4:22 PM, Dicebot wrote:

 On Thursday, 13 November 2014 at 22:19:55 UTC, Robert burner
 Schadek wrote:

 One bad thing about that is that the global log is no longer 
 sorted by
 time if you write to any file. That would make using the log 
 much more
 difficult IMO.


 We do have timestamp as part of recorded log data, don't we?


 I'm not following this thread, but please please please -- 
 output log data in chronological order. If you have to have 
 another object that does it, fine. But I don't want to have to 
 rely on sorting utilities to properly read a log file.

 -Steve


I have meant that shared logger can do sorting based on timestamp 
upon receiving chunks for thread-local ones before sending it 
forward - it would require defining strict syncing period between 
loggers though.



 Nov 14 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


I will test something this weekend regarding the additional 
indirection.



 Nov 14 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Friday, 14 November 2014 at 22:20:17 UTC, Robert burner 
Schadek wrote:

 I will test something this weekend regarding the additional 
 indirection.


Thanks! I may try hacking some sample implementation too but 
pessimistic about ETA



 Nov 14 2014



  parent  "Robert burner Schadek" <rburners gmail.com>  writes:


On Friday, 14 November 2014 at 23:06:22 UTC, Dicebot wrote:

 On Friday, 14 November 2014 at 22:20:17 UTC, Robert burner 
 Schadek wrote:

 I will test something this weekend regarding the additional 
 indirection.


 Thanks! I may try hacking some sample implementation too but 
 pessimistic about ETA


So, I added a layer of thread local indirection to the Logger. It 
now goes by default like:

log|trace|... -> threadLocalLogger -> globalLogger

The threadLocalLogger is just another Logger so it can be 
replaced with whatever you need may need. Or just keep it 
forwarding.



 Nov 24 2014



prev sibling  parent  "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?=  writes:


On Friday, 14 November 2014 at 22:02:30 UTC, Steven Schveighoffer 
wrote:

 I'm not following this thread, but please please please -- 
 output log data in chronological order.


I'm not sure if you can define a strict chronological order 
between threads since logging happens "after the fact" which may 
be truly parallel (in a race). If you want strict chronological 
order between threads you can use an atomic counter for a serial 
id…

For a web server this is usually not that interesting. You are 
interested in chronological order on the first dispatch and then 
on the request (the fiber id?)...

When you are doing buffering with one producer and one consumer 
on a circular buffer you usually don't need locking or atomics 
since single read/writes are atomic on x86 AFAIK. And with 
double/triple buffering you only need an occasional CAS.

I can think of at least of 3-4 ways of getting a performant 
multithreaded logger with almost no locking.



 Nov 14 2014



prev sibling  parent  "Martin Nowak" <code dawg.eu>  writes:


On Friday, 31 October 2014 at 15:35:55 UTC, Dicebot wrote:

 On Tuesday, 28 October 2014 at 22:03:18 UTC, Martin Nowak wrote:

 2nd iteration of that idea.


 For me it looks like the cure is worse than the disease. Simply 
 not distributing precompiled libraries with log level 
 force-reduced (or at least not exposing it in provided .di 
 files) feels like a more practical approach than proposed API 
 complication.


There are no .di files in a dub package.
And for certain libraries the runtime overhead for checking log 
levels isn't acceptable, so we need a non-global way to configure 
logging of a library.



 Nov 16 2014



prev sibling  parent  "Kevin Lamonte" <kevindotlamnodotonte gmail.com>  writes:


On Tuesday, 28 October 2014 at 11:11:09 UTC, Martin Nowak wrote:

 I think part of the misunderstanding is that I'm thinking of an 
 app as user code plus a number of libraries all on top of 
 phobos. Say I have an app using vibe.d and I want to enable 
 logging in my app, but disable it in phobos.
 Was it even a design goal that logging can be enabled on a per 
 library level?


Whether it was an original design goal or not, I don't think it 
can be done with today's std.logger, and I would argue that it 
should not be a function of std.logger at all.

Selective logging of a mix of libraries requires a namespace to 
identify each library, along with a method for libraries to 
obtain their Loggers based on that namespace (e.g. giving Logger 
a name, which it does not have now, and using a factory method 
like Logger.getLogger()).  We could assume that module names will 
always be available to do that job (and by default I do provide 
that as a reasonable default in log4d), but I can easily imagine 
other namespace choices for unknown future users.  I also think 
that most end users would prefer their selective 
enabling/disabling of libraries' logging to happen at runtime, 
not compile time, and the HOW of turning things on/off will also 
be very user-dependent (regex, config file, environment var, ...).

We are bleeding between "mechanism" and "policy" here.  I feel 
that std.logger is mechanism: given that a client has a Logger 
"thing" (struct, class, whatever), which methods will the client 
call on it to emit output to somewhere?  Frameworks like log4d 
are policy: how are the Loggers that are obtained by lots of 
clients organized into a coherent whole, and the outputs 
filtered/directed/controlled by the final end-user?  Policy 
should always be deferred to third party libraries.  I obviously 
like the log4d/log4j approach, but other people will like vibe.d 
logger, some will roll-your-own, some will just send everything 
to syslog, etc.

If we really want to insist that std.logger provides selective 
control of libraries, then I think we will also need to establish 
policy at this time.  I see two reasonable choices.

Option 1:  minimal change

1. The policy is that we control library logging by letting the 
client replace Logger references at runtime.
2. We say that libraries either use stdlog, i.e. info(...), 
error(...), etc., OR that they expose a Logger reference that 
clients can replace.
3. Clients replace stdlog or the libraries' Loggers with Loggers 
from their chosen framework.

Option 2:  invasive change

1. We make policy that there is a namespace of all loggers and it 
is __MODULE__.
2. stdlog is removed.
3. Loggers are obtained only via a getLogger() type call.  Any 
function that wants to log must call getLogger() first.
4. log(...), info(...), etc. are removed.  All logging goes 
through a Logger: Logger.info/error/etc(...)
5. Clients replace the getLogger() factory method with a 
function/delegate provided by their framework.

I think that option 1 would scale better long-term.  Phobos can 
choose to use stdlog for its own logging, and other libraries can 
make their own (vibelog, foolog, ...).  std.logger would need no 
changes.



Final thought: filtering.  We have several filtering methods now, 
and I am confused as to what is actually missing:

* ANY AND ALL logging can be disabled at compile time via 
versions.  This only includes client code and libraries the 
client is compiling too; I would assume that the default shipped 
phobos would not version-remove log calls.

* ALL logging can be disabled at runtime via globalLogLevel and a 
single if check at the call site.

* Per-Logger logging can be disabled at runtime via 
Logger.logLevel and a single if check at the call site.

* All other kinds of filtering (on msg, timestamp, method, etc.) 
are available at runtime to Logger subclasses, but happens after 
the logImpl call and setup of LogEntry.

What other capabilities are needed for filtering?



 Oct 28 2014



prev sibling  parent reply "Dicebot" <public dicebot.lv>  writes:


On Sunday, 26 October 2014 at 22:12:20 UTC, Martin Nowak wrote:

 On 10/25/2014 06:43 PM, Dicebot wrote:

 Because of that I am going to start voting despite some 
 arguments being
 still in process. I hope that won't cause any tension.


 The dependency on external version identifiers in phobos is 
 still a complete bummer and there are still many implementation 
 issues.
 One reason why this is taking so long is that there were many 
 issues some of which still need to be addressed.


I don't consider it a major issue as I don't think std.logger 
should be used inside Phobos at all.



 Oct 27 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Monday, 27 October 2014 at 07:03:11 UTC, Dicebot wrote:

 I don't consider it a major issue as I don't think std.logger 
 should be used inside Phobos at all.


Yes, using std.logger inside of phobos is a no-no



 Oct 27 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Monday, 27 October 2014 at 08:23:48 UTC, Robert burner Schadek 
wrote:

 On Monday, 27 October 2014 at 07:03:11 UTC, Dicebot wrote:

 I don't consider it a major issue as I don't think std.logger 
 should be used inside Phobos at all.


 Yes, using std.logger inside of phobos is a no-no


Ayway, let's come with an agreement/compromise with Martin and 
I'll start voting immediately after.



 Oct 27 2014



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Monday, 27 October 2014 at 12:03:33 UTC, Dicebot wrote:

 Ayway, let's come with an agreement/compromise with Martin and 
 I'll start voting immediately after.


Well, as far as I can see his argument was based on old code that 
has long been rewritten and he hasn't answered since I pointed 
that out.



 Oct 27 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/27/2014 01:36 PM, Robert burner Schadek wrote:

 Well, as far as I can see his argument was based on old code that has
 long been rewritten and he hasn't answered since I pointed that out.


How do come to that insight?



 Oct 27 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Monday, 27 October 2014 at 22:20:04 UTC, Martin Nowak wrote:

 On 10/27/2014 01:36 PM, Robert burner Schadek wrote:

 Well, as far as I can see his argument was based on old code 
 that has
 long been rewritten and he hasn't answered since I pointed 
 that out.


 How do come to that insight?


because the code you show as an example:

"cat > lib.d << CODE
version (StdLoggerDisableLogging)
     enum isLoggingActive() = false;
else
     enum isLoggingActive() = true;

void doSome()
{
     import std.stdio;
     writeln("loggingLib: ", isLoggingActive!());
}
CODE"

is different from the code that has been in the PR for quite some 
time. And the code you show does exactly what you say and the 
current code does something different.



 Oct 27 2014



  parent reply Martin Nowak <code+news.digitalmars dawg.eu>  writes:


On 10/28/2014 01:01 AM, Robert burner Schadek wrote:

 is different from the code that has been in the PR for quite some time.
 And the code you show does exactly what you say and the current code
 does something different.


No it behaves the same.

isLoggingActive is a template in phobos

doSome is a function in a lib that performs logging and instantiates 
isLoggingActive

main is a function in the app that performs logging and instantiates 
isLoggingActive and also calls doSome

Now which of those functions actually logs depends on the compilation 
settings of the library, the compilation settings of the app and the 
logger that's being used.



 Oct 27 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 28 October 2014 at 01:42:12 UTC, Martin Nowak wrote:

 On 10/28/2014 01:01 AM, Robert burner Schadek wrote:

 is different from the code that has been in the PR for quite 
 some time.
 And the code you show does exactly what you say and the 
 current code
 does something different.


 No it behaves the same.

 isLoggingActive is a template in phobos

 doSome is a function in a lib that performs logging and 
 instantiates isLoggingActive

 main is a function in the app that performs logging and 
 instantiates isLoggingActive and also calls doSome

 Now which of those functions actually logs depends on the 
 compilation settings of the library, the compilation settings 
 of the app and the logger that's being used.


The second two are wanted and disabling a LogLevel at CT of 
phobos should be banned anyway. But no the less, it is one more 
option the user has to manipulate the Logger.



 Oct 28 2014



  parent  "Martin Nowak" <code dawg.eu>  writes:


On Tuesday, 28 October 2014 at 09:00:54 UTC, Robert burner 
Schadek wrote:

 The second two are wanted and disabling a LogLevel at CT of 
 phobos should be banned anyway. But no the less, it is one more 
 option the user has to manipulate the Logger.


And this is where the leakage happens because there is no sharp 
border between app and library. Functions of the library may get 
unlined into the app, templates of the library are instantiated 
in the app and some instantiations that are already in the lib 
won't be reinstantiated in the app.



 Oct 28 2014



prev sibling  parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 10/27/14 5:03 AM, Dicebot wrote:

 On Monday, 27 October 2014 at 08:23:48 UTC, Robert burner Schadek wrote:

 On Monday, 27 October 2014 at 07:03:11 UTC, Dicebot wrote:

 I don't consider it a major issue as I don't think std.logger should
 be used inside Phobos at all.


 Yes, using std.logger inside of phobos is a no-no


 Ayway, let's come with an agreement/compromise with Martin and I'll
 start voting immediately after.


Agreed. Just to restate my position: so long as we don't have a way to 
statically control maximum logging level in the client, we don't have a 
logging library. There is no negotiation. -- Andrei



 Oct 28 2014



  parent reply "Dicebot" <public dicebot.lv>  writes:


On Tuesday, 28 October 2014 at 16:05:19 UTC, Andrei Alexandrescu 
wrote:

 Agreed. Just to restate my position: so long as we don't have a 
 way to statically control maximum logging level in the client, 
 we don't have a logging library. There is no negotiation. -- 
 Andrei


We have way to statically control logging level of the client 
from the client. Argument is mostly about precompiled 3d party 
libraries.



 Oct 28 2014



  parent  Johannes Pfau <nospam example.com>  writes:


Am Tue, 28 Oct 2014 16:32:34 +0000
schrieb "Dicebot" <public dicebot.lv>:


 On Tuesday, 28 October 2014 at 16:05:19 UTC, Andrei Alexandrescu 
 wrote:

 Agreed. Just to restate my position: so long as we don't have a 
 way to statically control maximum logging level in the client, 
 we don't have a logging library. There is no negotiation. -- 
 Andrei


 
 We have way to statically control logging level of the client 
 from the client. Argument is mostly about precompiled 3d party 
 libraries.


Could somebody summarize that discussion?
You can't 'statically' disable logging in a _precompiled_ library. So
is this about statically disabling logging in libraries when source
code is available or disabling logging at runtime for precompiled
libraries?



 Oct 29 2014



prev sibling next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 10/25/14 9:43 AM, Dicebot wrote:

 Jut for the reference, my position on current state of things as review
 manager is this:

 I agree with some of mentioned concerns and would like to see those
 fixed. However, I don't think any of those are truly critical and this
 proposal has been hanging there for just too long. In my opinion it will
 be more efficient to resolve any remainining issues in follow-up pull
 requests on case by case basis then forcing Robert to do it himself -
 there will be some time left before next release to break a thing or two
 before public statement is made.

 Because of that I am going to start voting despite some arguments being
 still in process. I hope that won't cause any tension.


Being able to select maximum logging level statically at client 
application level is a deal maker/breaker for me. The mechanics aren't 
important but it's likely they will affect the API. So I think that 
needs to be resolved now, not in a future pull request.

Andrei



 Oct 27 2014



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 28 October 2014 at 05:44:48 UTC, Andrei Alexandrescu 
wrote:

 Being able to select maximum logging level statically at client 
 application level is a deal maker/breaker for me. The mechanics 
 aren't important but it's likely they will affect the API. So I 
 think that needs to be resolved now, not in a future pull 
 request.

 Andrei


please elaborate. It is a must to be able to disable LogLevel 
when calling the compiler? Xor the opposite?

passing -version=StdLoggerDisableTrace when compiling the user 
code will yield empty functions (thank you static if) for every 
call to any function or method having the word "trace" in its 
name, like for example trace("My log call");

If you call something like log(computeMyLogLevelAtRuntime(), "my 
log data)
and that function returns LogLevel.trace and you haved passed 
-version=StdLoggerDisableTrace at CT of the user code you will 
have to pay for one if.



 Oct 28 2014



  parent  Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 10/28/14 2:46 AM, Robert burner Schadek wrote:

 On Tuesday, 28 October 2014 at 05:44:48 UTC, Andrei Alexandrescu wrote:

 Being able to select maximum logging level statically at client
 application level is a deal maker/breaker for me. The mechanics aren't
 important but it's likely they will affect the API. So I think that
 needs to be resolved now, not in a future pull request.

 Andrei


 please elaborate. It is a must to be able to disable LogLevel when
 calling the compiler? Xor the opposite?

 passing -version=StdLoggerDisableTrace when compiling the user code will
 yield empty functions (thank you static if) for every call to any
 function or method having the word "trace" in its name, like for example
 trace("My log call");

 If you call something like log(computeMyLogLevelAtRuntime(), "my log data)
 and that function returns LogLevel.trace and you haved passed
 -version=StdLoggerDisableTrace at CT of the user code you will have to
 pay for one if.


isFloatingPoint is an exact check, which is fine. We'd need isLikeXxx 
templates that support Liskov. -- Andrei



 Oct 28 2014



prev sibling  parent  "Kevin Lamonte" <kevindotlamnodotonte gmail.com>  writes:


On Saturday, 25 October 2014 at 16:43:10 UTC, Dicebot wrote:

 However, I don't think any of those are truly critical and this 
 proposal has been hanging there for just too long. In my 
 opinion it will be more efficient to resolve any remainining 
 issues in follow-up pull requests on case by case basis then 
 forcing Robert to do it himself - there will be some time left 
 before next release to break a thing or two before public 
 statement is made.


(Am I a voter?  No idea.  But here is my feedback, based on my 
recent experience implementing a fuller backend for std.logger.)

I agree, I think std.logger is ready to go forward and needs to 
get into end-user hands.

I see four areas that are incomplete but SHOULD be deferred to 
future bug reports / enhancement requests:

1. Additional LogEntry fields: Thread, Fiber, Throwable, maybe 
others.  These should only be adopted if the end users can 
provide a compelling case for why their Logger subclasses (or 
backend framework) can't give them what they want.

2. Additional Logger functions: caught(), throwing(), maybe 
others.  These should be driven by the needs of phobos' use of 
std.logger.  (And phobos SHOULD use std.logger and eat its own 
dog food.)

3. Modification of the order or method in which the Logger 
functions obtain their data in constructing the LogEntry, i.e. 
call Clock.currTime() differently or not at all.  These should be 
controlled by future additional fields/flags in Logger, with no 
changes to the functions one calls on a Logger.

4. Control of logging across multiple libraries/applications.  
This should absolutely be deferred until phobos itself starts 
using std.logger.


impossible to make everyone happy, and we keep poking into it as 
a result of not accepting that the only people who can decide on 
it are the application writers, not the language and phobos 
designers.  The two solutions for it at the extremes are:

A) Deciding if a Logger is enabled when it is obtained by the 
client (via a Logger.getLogger() factory function), which 
requires imposing a namespace of some kind that ALL future 
libraries will have to adhere to.  Note that log4j -- currently 
the fastest and "biggest" logging system out there -- started 
with a fixed className namespace, but moved away from that later 
on to more general user-defined "categories".  Even in a 
straightjacketed language like Java one namespace isn't good 
enough.

B) Deciding if a Logger should emit (based on end-user decision 
via config file, environment variable, ...) at the 
Logger.writeLogEntry() call via filtering on the LogEntry fields.

Option A is cumbersome in client code but very fast in practice 
because getLogger() can set the Logger's logLevel to filter 
messages before the logImpl() call.  Option B is easier in client 
code (and also lets many functions/modules/libraries write to a 
single Logger ala stdlog) but very slow at runtime because the 
namespace filtering can't occur until after the LogEntry is fully 
formed (formatted (including allocation) + Clock.currTime + ...).

The application writers are the only people in a position to 
choose which end of this tradeoff makes the most sense for them.  
Phobos will have to choose for itself someday, and I think that 
various subsystems may very well choose differently based on 
their most common usage.  std.net.curl might choose option B 
(just logging to stdlog) because it's got this nice 
clearly-defined border between the client and libcurl; whereas 
std.range might go for option A (letting the users set a 
getLogger() delegate) because it is very cross-cutting and users 
will only want to see log entries when doing range stuff in their 
code and not all over phobos or other libraries.



 Oct 29 2014



prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com>  writes:


Anyone know anything about this?

https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac



 Nov 24 2014



 next sibling parent reply "Brian Schott" <briancschott gmail.com>  writes:


On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


You are posting to page 16 of the third iteration of a single 
review.



 Nov 24 2014



  parent reply Walter Bright <newshound2 digitalmars.com>  writes:


On 11/24/2014 4:51 PM, Brian Schott wrote:

 On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


 You are posting to page 16 of the third iteration of a single review.


I know, and the reddit comment refers to this.



 Nov 24 2014



  parent reply "ponce" <contact gam3sfrommars.fr>  writes:


On Tuesday, 25 November 2014 at 01:12:03 UTC, Walter Bright wrote:

 On 11/24/2014 4:51 PM, Brian Schott wrote:

 On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright 
 wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


 You are posting to page 16 of the third iteration of a single 
 review.


 I know, and the reddit comment refers to this.


This discussion is indeed most unsettling to read. Third review 
of a much-needed module in the ecosystem, and I remember of 
previous attempts at logging, each time taken down because it 
does not satisfy the whims of top-tier D developers that would 
have done it differently (and of course "better").

What is accepted or not in Phobos no longer interest me. I can 
rely on interesting modules through DUB which has versionned 
dependencies, while Phobos has not.
Better XML parsers/JSON parsers/serialization/argument parsers 
exist outside of Phobos currently, and in my opinion maybe they 
didn't belong there in the first place.



 Nov 25 2014



 next sibling parent  "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?=  writes:


On Tuesday, 25 November 2014 at 14:29:12 UTC, ponce wrote:

 Better XML parsers/JSON parsers/serialization/argument parsers 
 exist outside of Phobos currently, and in my opinion maybe they 
 didn't belong there in the first place.


Yes, it is often a good idea to standardize after solutions have 
been established.

Creating a good logger is harder than it sounds like as the 
logger have to used by everyone or else you have to deal with N 
incompatible versions in the same project due to different 
libraries using different loggers.

Starting with the interface with no performant proof of concept 
means you go for a long run. Starting with an existing performant 
solution and abstracting it into a cleaner interface would reach 
completion faster and with less risk of hitting gotchas later.

A logger that isn't good enough for just about everyone will 
become dead weight as people will gravitate towards an external 
adhoc standard solution instead (re Tango vs Phobos).



 Nov 25 2014



prev sibling  parent  "Martin Nowak" <code dawg.eu>  writes:


On Tuesday, 25 November 2014 at 14:29:12 UTC, ponce wrote:

 On Tuesday, 25 November 2014 at 01:12:03 UTC, Walter Bright 
 wrote:

 On 11/24/2014 4:51 PM, Brian Schott wrote:

 On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright 
 wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


 You are posting to page 16 of the third iteration of a single 
 review.


 I know, and the reddit comment refers to this.


 This discussion is indeed most unsettling to read. Third review 
 of a much-needed module in the ecosystem, and I remember of 
 previous attempts at logging, each time taken down because it 
 does not satisfy the whims of top-tier D developers that would 
 have done it differently (and of course "better").


Things in phobos just have to sit, we already carry around too 
many crap modules (signals, XML, curl).


 What is accepted or not in Phobos no longer interest me. I can 
 rely on interesting modules through DUB which has versionned 
 dependencies, while Phobos has not.


That's a good thing because a package system can cover different 
needs with much more variety.


 Better XML parsers/JSON parsers/serialization/argument parsers 
 exist outside of Phobos currently, and in my opinion maybe they 
 didn't belong there in the first place.


I partly agree with this, having certain things covered by 3-rd 
party libraries allows for faster iteration. Something shpuld 
only become a Phobos module if there can be definite design which 
is paired with a very good implementation.



 Nov 29 2014



prev sibling  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


You mean the second part, about him leaving D because of the 
discussion about the logger?



 Nov 25 2014



  parent reply Walter Bright <newshound2 digitalmars.com>  writes:


On 11/25/2014 2:26 AM, Robert burner Schadek wrote:

 On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


 You mean the second part, about him leaving D because of the discussion about
 the logger?


Yes.



 Nov 25 2014



 next sibling parent  "Robert burner Schadek" <rburners gmail.com>  writes:


On Tuesday, 25 November 2014 at 23:41:51 UTC, Walter Bright wrote:

 On 11/25/2014 2:26 AM, Robert burner Schadek wrote:

 On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright 
 wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


 You mean the second part, about him leaving D because of the 
 discussion about
 the logger?


 Yes.


Not really, this is the first time I read the name SiCl4. Also 
google SiCl4 dlang only points to the reddit post.



 Nov 26 2014



prev sibling  parent  "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?=  writes:


On Tuesday, 25 November 2014 at 23:41:51 UTC, Walter Bright wrote:

 On 11/25/2014 2:26 AM, Robert burner Schadek wrote:

 On Tuesday, 25 November 2014 at 00:37:00 UTC, Walter Bright 
 wrote:

 Anyone know anything about this?

 https://www.reddit.com/r/programming/comments/2n9gfb/d_is_for_data_science/cmbssac


 You mean the second part, about him leaving D because of the 
 discussion about
 the logger?


 Yes.


He wants a "better C" for system level programming:

https://www.reddit.com/r/programming/comments/2n60wv/the_first_enemy_of_c_is_its_past/cmbn1n4

https://www.reddit.com/r/programming/comments/2n60wv/the_first_enemy_of_c_is_its_past/cmb4ows

And there is no doubt an open spot between C and C++ for a 
modernized system level language.



 Nov 26 2014



prev sibling  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


recent updates:
* Martins CT log function disabling (thanks Martin)
* new thread local indirection Logger between free standing log 
functions and program global Logger
* more documentation
* some  trusted have been remove (thanks Dicebot)
* local imports

please review



 Jan 06 2015



 next sibling parent  Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 1/6/15 8:51 AM, Robert burner Schadek wrote:

 recent updates:
 * Martins CT log function disabling (thanks Martin)
 * new thread local indirection Logger between free standing log
 functions and program global Logger
 * more documentation
 * some  trusted have been remove (thanks Dicebot)
 * local imports

 please review


Links for the lazy.

Code:

https://github.com/D-Programming-Language/phobos/pull/1500

Dox:

http://burner.github.io/phobos/phobos-prerelease/std_experimental_logger_core.html
http://burner.github.io/phobos/phobos-prerelease/std_experimental_logger_filelogger.html
http://burner.github.io/phobos/phobos-prerelease/std_experimental_logger_multilogger.html
http://burner.github.io/phobos/phobos-prerelease/std_experimental_logger_nulllogger.html


Andrei



 Jan 06 2015



prev sibling next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 1/6/15 8:51 AM, Robert burner Schadek wrote:

 recent updates:
 * Martins CT log function disabling (thanks Martin)
 * new thread local indirection Logger between free standing log
 functions and program global Logger
 * more documentation
 * some  trusted have been remove (thanks Dicebot)
 * local imports

 please review


I propose we pull this in today and make it available for 2.067 as 
std.experimental.logger.

We've been through a number of iterations with this and the best way to 
move forward is to accumulate a bit of real-world experience with it. 
Since we're deploying to std.experimental there is understanding 
breaking changes are still possible.

Dicebot, as the review manager you get to decide. What do you say?


Andrei



 Jan 23 2015



 next sibling parent  "Joseph Cassman" <jc7919 outlook.com>  writes:


On Friday, 23 January 2015 at 20:24:54 UTC, Andrei Alexandrescu 
wrote:

 On 1/6/15 8:51 AM, Robert burner Schadek wrote:

 recent updates:
 * Martins CT log function disabling (thanks Martin)
 * new thread local indirection Logger between free standing log
 functions and program global Logger
 * more documentation
 * some  trusted have been remove (thanks Dicebot)
 * local imports

 please review


 I propose we pull this in today and make it available for 2.067 
 as std.experimental.logger.

 We've been through a number of iterations with this and the 
 best way to move forward is to accumulate a bit of real-world 
 experience with it. Since we're deploying to std.experimental 
 there is understanding breaking changes are still possible.

 Dicebot, as the review manager you get to decide. What do you 
 say?


 Andrei


+1



 Jan 23 2015



prev sibling next sibling parent  "Kapps" <opantm2+spam gmail.com>  writes:


On Friday, 23 January 2015 at 20:24:54 UTC, Andrei Alexandrescu 
wrote:

 I propose we pull this in today and make it available for 2.067 
 as std.experimental.logger.

 We've been through a number of iterations with this and the 
 best way to move forward is to accumulate a bit of real-world 
 experience with it. Since we're deploying to std.experimental 
 there is understanding breaking changes are still possible.

 Dicebot, as the review manager you get to decide. What do you 
 say?


 Andrei


Agreed. Logger is such a subjective module. No matter what you 
do, there is a guarantee that people will have something more 
they desire. D badly needs at least some basic logging though, 
and if people need more complex features, they then do their own. 
But so long as the default is fine for most people, which this 
looks like it should be, then having it is a valuable addition; 
it's an important module. At this point, the most useful next 
step would be experience as you mention. There might be some 
improvements desired before full Phobos integration, such as 
cleaning up the documentation for some symbols (particularly the 
12 'alias trace/info/etc(f)') by using version(D_Doc) to hide 
part of it, but that's certainly not necessary for 
std.experimental inclusion.



 Jan 23 2015



prev sibling  parent reply "Dicebot" <public dicebot.lv>  writes:


On Friday, 23 January 2015 at 20:24:54 UTC, Andrei Alexandrescu 
wrote:

 I propose we pull this in today and make it available for 2.067 
 as std.experimental.logger.

 We've been through a number of iterations with this and the 
 best way to move forward is to accumulate a bit of real-world 
 experience with it. Since we're deploying to std.experimental 
 there is understanding breaking changes are still possible.

 Dicebot, as the review manager you get to decide. What do you 
 say?


 Andrei


I was in favor of merging it long time ago. It is _your_ 
defintion of being good enough for std.experimental I am trying 
to comply now ;) Merging it right now and continuing with fixes 
as follow-up pull requests is a good idea but you must understand 
that chance of breaking chance is very high.

That said I have just now tested last version (with thread-local 
log support) and apart from one bug found it works as expected. 
Assuming you give your LGTM I will merge the PR as soon as this 
bug is fixed and PR is rebased/squashed.



 Jan 24 2015



  parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 1/24/15 7:06 AM, Dicebot wrote:

 On Friday, 23 January 2015 at 20:24:54 UTC, Andrei Alexandrescu wrote:

 I propose we pull this in today and make it available for 2.067 as
 std.experimental.logger.

 We've been through a number of iterations with this and the best way
 to move forward is to accumulate a bit of real-world experience with
 it. Since we're deploying to std.experimental there is understanding
 breaking changes are still possible.

 Dicebot, as the review manager you get to decide. What do you say?


 Andrei


 I was in favor of merging it long time ago. It is _your_ defintion of
 being good enough for std.experimental I am trying to comply now ;)
 Merging it right now and continuing with fixes as follow-up pull
 requests is a good idea but you must understand that chance of breaking
 chance is very high.

 That said I have just now tested last version (with thread-local log
 support) and apart from one bug found it works as expected. Assuming you
 give your LGTM I will merge the PR as soon as this bug is fixed and PR
 is rebased/squashed.


Fantastic. Thanks! Robert, please mind the bug and then Dicebot please 
do the merge honors.

Thanks Robert for this work, Dicebot for overseeing the review process, 
and everybody in the community who provided feedback and ideas.


Andrei



 Jan 24 2015



 next sibling parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


I will fix the bug this weekend and rebase to upstream/master. 
The alias doc has seen some updates this week (please check the 
rebuild gh-pages, links are in the PR description)



 Jan 24 2015



  parent reply Jacob Carlborg <doob me.com>  writes:


On 2015-01-24 18:45, Robert burner Schadek wrote:

 I will fix the bug this weekend and rebase to upstream/master. The alias
 doc has seen some updates this week (please check the rebuild gh-pages,
 links are in the PR description)


Is the generated documentation for package module available somewhere? 
This [1] is empty.

[1] 
http://burner.github.io/phobos/phobos-prerelease/std_experimental_logger_package.html

-- 
/Jacob Carlborg



 Jan 25 2015



  parent reply "Robert burner Schadek" <rburners gmail.com>  writes:


On Sunday, 25 January 2015 at 09:43:36 UTC, Jacob Carlborg wrote:

 On 2015-01-24 18:45, Robert burner Schadek wrote:

 I will fix the bug this weekend and rebase to upstream/master. 
 The alias
 doc has seen some updates this week (please check the rebuild 
 gh-pages,
 links are in the PR description)


 Is the generated documentation for package module available 
 somewhere? This [1] is empty.

 [1] 
 http://burner.github.io/phobos/phobos-prerelease/std_experimental_logger_package.html


fixed



 Jan 25 2015



  parent reply zeljkog <zeljkog home.com>  writes:


On 25.01.15 15:06, Robert burner Schadek wrote:

 On Sunday, 25 January 2015 at 09:43:36 UTC, Jacob Carlborg wrote:

 Is the generated documentation for package module available somewhere? 
 This [1] is empty.

 [1] 
 http://burner.github.io/phobos/phobos-prerelease/std_experimental
logger_package.html 


 
 fixed


Interface looks to me convenient.

Is there a way to log to buffer or a like?



 Jan 25 2015



  parent  "Dicebot" <public dicebot.lv>  writes:


On Sunday, 25 January 2015 at 14:50:46 UTC, zeljkog wrote:

 Is there a way to log to buffer or a like?


By implementing own Logger. Idea is that std.experimental.logger 
provides only basic building blocks and defines API - any more 
advanced logger implementations can be provided via dub packages.



 Jan 25 2015



prev sibling  parent reply "Dicebot" <public dicebot.lv>  writes:


Sadly, the issue I have been referring to is actually a DMD bug : 
https://issues.dlang.org/show_bug.cgi?id=14050

It seems a blocker, at least I can't quickly imagine a workaround 
for it.



 Jan 26 2015



  parent  Andrei Alexandrescu <SeeWebsiteForEmail erdani.org>  writes:


On 1/26/15 6:52 AM, Dicebot wrote:

 Sadly, the issue I have been referring to is actually a DMD bug :
 https://issues.dlang.org/show_bug.cgi?id=14050

 It seems a blocker, at least I can't quickly imagine a workaround for it.


Thanks Kenji for the fix! Just merged it. -- Andrei



 Jan 26 2015



prev sibling  parent reply Bastiaan Veelo <Bastiaan Veelo.net>  writes:


On Tuesday, 6 January 2015 at 16:51:26 UTC, Robert burner Schadek 
wrote:

 please review


Two small doc issues on 
http://dlang.org/phobos/std_experimental_logger.html

"The LogLevel of an log call" -> "[...] a log call"

"[...] or The additional f enables printf-style logging" missing 
parts of a preceding sentence?

Bastiaan.



 Jan 06 2016



  parent  Bastiaan Veelo <Bastiaan Veelo.net>  writes:


On Wednesday, 6 January 2016 at 16:30:23 UTC, Bastiaan Veelo 
wrote:

 Two small doc issues on 
 http://dlang.org/phobos/std_experimental_logger.html


https://github.com/D-Programming-Language/phobos/pull/3907



 Jan 07 2016