Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.logger #1500

Merged
merged 9 commits into from Jan 26, 2015
Merged

std.logger #1500

merged 9 commits into from Jan 26, 2015

Conversation

@JakobOvrum
Copy link
Member

Please open a thread on the official D forums/newsgroup to facilitate discussion and review for any new Phobos module proposition. Consult the review queue and review process wiki pages for more information.

@burner
Copy link
Member Author

burner commented Aug 22, 2013

I have send the mail minutes ago. Should pop up every second now

@monarchdodra
Copy link
Collaborator

I think you meant s/Fatel/Fatal/g ? Fatel ain't no english I know of.

@burner
Copy link
Member Author

burner commented Aug 22, 2013

shit, My feult ;-)

@burner
Copy link
Member Author

burner commented Sep 2, 2013

bump, I really could use some comments on this one.

@ghost
Copy link

ghost commented Sep 2, 2013

�Really cool, very glad someone is looking at this.

Some suggestions:

  1. Add Debug and Trace log levels, in the order: Trace < Debug < Info < Warning < Error < Critical < Fatal . "Trace" is meant for logging function enter and exit. "Debug" is for general debugging, the equivalent of JDK logger's Fine or Log4perl's Debug.
  2. Speaking of Log4perl, instead of sub-classing a Logger to get different outputs (file, stdio), you might consider adopting the Log4j/Log4perl method of appenders. What's nice about that is the same log message can appear in multiple appenders, so for example there could be a logfile containing only Error+ messages and another containing Debug+ messages. You could still have a FileLogger, which would just be a Logger with its first appender being to a file.
  3. Convenience functions Logger.info(), Logger.warning(), etc. which always pass to logf(LogLevel, ...).
  4. The quick-and-dirty logger I've been using: http://bazaar.launchpad.net/~kevin-lamonte/nib/devel/view/head:/source/logging.d . Might be something useful in there.
  5. Might consider adopting Log4perl-like configuration file with the init_and_watch() functionality, so that someone can enable/disable logging on-the-fly externally.

@burner
Copy link
Member Author

burner commented Sep 3, 2013

thanks for your input. I will look into it

@grogancolin
Copy link
Contributor

Would it be possible to add convenience methods for each log level like:
logger.logDebug("This is a debug message");
logger.logInfo("This is an info message");
logger.logSevere(...);
logger.logFatal(...);

This would be easier than typing:
logger.log(LogLevel.Info, "...");
every time?

Edit:
I see that this has already been suggested above.

@burner
Copy link
Member Author

burner commented Feb 12, 2014

It is already implemented this way. As these methods are just proxy to the original log(LogLevel, ...) method and are therefor trivial to implement, I used a mixin to create them. Unfortunately https://d.puremagic.com/issues/show_bug.cgi?id=648 currently stops documentation from being generated.

@fugalh
Copy link

fugalh commented Mar 6, 2014

I like it. I agree a super-noisy-debug level (trace, spew, whatever you call it) is a good thing to have, then you can leave some very noisy but occasionally useful logging statements in the code for those really hairy debug sessions, without making debug output useless for daily debugging.

Tracing messages can be really useful - entering/exiting a function - especially if you auto-indent them. I once wrote a few log functions that did that, they incremented and decremented the indent level. As long as the coder put them in the right place it worked well. I wonder if you could do that automagically in D (or whether that would even be a good idea to trace all functions).

It loks like the fatal level doesn't actually terminate the program. Some logging libraries trigger the end of program when a fatal message is logged. Just how you would want to terminate in D I'm not sure. SIGABRT? Without the termination behavior, what value does it add above the critical level? I'm not sure it adds anything, but having the extra level doesn't bother me either.

@burner
Copy link
Member Author

burner commented Mar 6, 2014

How about making the LogLevel a bitset, so you log traces and react on fatal?

Scope tracing is easy, RAII. I will add a something to do that.

I know, but I'm not sure how to quit a program the "correct" way. Maybe with exit.

@fugalh
Copy link

fugalh commented Mar 6, 2014

A bitset sounds like more sophistication than is needed, but as long as I can do log.warning() etc. I'm happy.

The thing with exiting is people set up signal handlers to do cleanup, which don't get called if you just exit(). An exception might be appropriate. Or maybe not exiting at all, and just let people use critical followed with exiting however they want. Or maybe it's ok that the exit is exceptional and signal handlers don't get called.

I sometimes get frustrated when I'm reading code and come across a fatal log call because I have to go figure out if the logging framework they're using does or does not terminate (and how) on a fatal-level logging call.

@burner
Copy link
Member Author

burner commented Mar 6, 2014

warning will stay, no doubt about that.

I know what you mean, side-effect-hell.
Maybe throwing an Error by default and allowing a delegate as a custom hook?

@Geod24
Copy link
Member

Geod24 commented Mar 7, 2014

IMHO, it would be a bad idea to terminate it on fatal. This may be something some library do, but I don't think it's a good practice to do that in the standard library.

@burner
Copy link
Member Author

burner commented Mar 7, 2014

I pushed a new version which calls a assignable delegate on fatal messages.

@andralex
Copy link
Member

andralex commented Mar 9, 2014

I have one request. Seems like there's no way to turn logging on/off during compilation. The only mechanism I saw is version(DisableLogging) but I was hoping for more - fine-grained control of what level is allowed for logging, and no overhead for anything disabled.

I also recall the previous proposal had some nice mechanisms for throttling logging ("log this once in n calls" or "log this once in n milliseconds"). Any plans to integrate that?

@burner
Copy link
Member Author

burner commented Mar 10, 2014

I will add version(Disable[Trace,Info,Warning,...]Logging) and version(Disable[Stdio,File,Multi]Logging) statements.

std.log had a "when filter". std.logger takes a boolean as the first arguments. Appropriate predicates are very user specific, IMO. I would rather stay very generic here.

@braddr
Copy link
Member

braddr commented Mar 10, 2014

Just a reminder, this pull request should NOT be merged without having gone through the standard new module review process. std.log[ger] is too critical a component to sneak in by way of a pull request without proper review and discussion. Particularly considering that the last review of a similar module didn't pass the review.

@burner
Copy link
Member Author

burner commented Mar 11, 2014

That will not happen. https://d.puremagic.com/issues/show_bug.cgi?id=648 has to be fixed before it can be merged, because most documentation is generated.

@AndrejMitrovic

@ghost
Copy link

ghost commented Mar 11, 2014

Yeah well every time I ping people it takes 1+ months for a response, by the time the pull needs another rebase. I'm not gonna rebase 10 times a year.

@burner
Copy link
Member Author

burner commented Mar 11, 2014

Well, that are bad news, at least for me. I'm not big into the dmd source, but the code looks not that hard and I'm will willing to rebase. I now see that it only works for template mixins and not normal templates, as far as I see. I would like to work on your patch, If you don't mind ? @AndrejMitrovic

@ghost
Copy link

ghost commented Mar 11, 2014

That's not the problem, I can rebase it. But I need someone around who's willing to review and pull soon.

@andralex
Copy link
Member

@burner fine - just make sure you don't evaluate arguments if the condition is false (I recall that was what made when() desirable).

@burner
Copy link
Member Author

burner commented Mar 11, 2014

if there is a bool passed, the first line in reads

if (cond &&

@andralex
Copy link
Member

@burner I'm referring to the arguments passed to the logging functions. Consider:

logger.log(sometimes(), "We're in file: " ~ file);

vs.

if (sometimes()) logger.log("We're in file: " ~ file);

In the first case you need to carefully make your parameters scope lazy to make sure concatenation isn't done when sometimes() is false. Make sure the right code is generated by using e.g. http://d.godbolt.org. I recall I made some experiments work but it took some tweaking.

@burner
Copy link
Member Author

burner commented Mar 13, 2014

it lazy now

@burner
Copy link
Member Author

burner commented May 6, 2014

I updated the docs, and the github preview (see the link at the top)

@MetaLang
Copy link
Member

MetaLang commented May 6, 2014

Isn't lazy very slow? Is using lazy worth it just to avoid concatenating some strings?

@burner
Copy link
Member Author

burner commented May 7, 2014

It is not just used to concatenate strings, it is used to pass the args for the printf version in. They may require expensive computation and are not always need.

@burner burner mentioned this pull request May 11, 2014
@burner
Copy link
Member Author

burner commented Jan 26, 2015

take your time I waited 1.5 Years

@mihails-strasuns
Copy link

Reduced to this DMD bug:

static if (__traits(compiles, { import missing; }))
{
    pragma(msg, "importing missing");
    import missing;
}
else
{
    pragma(msg, "not importing missing");
}

/*
$ ./dmd/src/dmd -o- -v reduced.d 
binary    ./dmd/src/dmd
version   v2.067-devel-6b86b12
config    ./dmd/src/dmd.conf
parse     reduced
importall reduced
import    object    (/home/dicebot/devel/dlang/druntime/src/object.di)
semantic  reduced
import    missing   (missing.d)
not importing missing
semantic2 reduced
semantic3 reduced
*/

It manifests on all DMD versions I have used, doesn't matter what kind of installation it is (it doesn't even use runtime or phobos). I have no idea why it works for you to be honest. Maybe you have a rogue test_loggerconfig.d hidden somewhere in the system? :)

@mihails-strasuns
Copy link

@burner
Copy link
Member Author

burner commented Jan 26, 2015

that is very strange. I just copied the test and ran it. it did not import missing.

I tested on two separated machines multiple times in different folders.

@mihails-strasuns
Copy link

it does not actually import (second pragma triggers) but lists it in verbose output in import list. It isn't there on your machine?

@burner
Copy link
Member Author

burner commented Jan 26, 2015

"""
import foo (foo.d)
not importing foo
"""

but it does not fail.

@Dicebot I think I understand why this is a problem know. All the build tools using this output can find it afterwards.

This reverts commit 299f018.

thread local forward

thread local default log function forward

nicer imports

forwardMsg must not be final

moved comments to package and started to integerade Martin's log disabling

more compile time function disabling

style and dscanner suggestions

stdThreadLog fix and doc

a lot of updates

* spell fixes
* better tests
* docu changes

docu update

whitespace

moduleLogLevel docu
@mihails-strasuns
Copy link

I will create a regression test PR for your branch based on that snippet and should be good to merge after.

@mihails-strasuns
Copy link

burner#4

@burner
Copy link
Member Author

burner commented Jan 26, 2015

merged burner#4 thanks

@mihails-strasuns
Copy link

Auto-merge toggled on

@mihails-strasuns
Copy link

fingers crossed :)

I suggest making NG announcement which both describes most recent state of the logger (new features added since last mass review) and encourages people to create pull requests to fixup any remaining flaws before 2.067 (to minimize breakage). Can also do it myself tomorrow.

mihails-strasuns pushed a commit that referenced this pull request Jan 26, 2015
@mihails-strasuns mihails-strasuns merged commit 9d54553 into dlang:master Jan 26, 2015
@quickfur
Copy link
Member

Whoa. std.logger finally merged?! Time to celebrate!!

@burner
Copy link
Member Author

burner commented Jan 26, 2015

indeed

@MartinNowak
Copy link
Member

I'm glad to see this in std.experimental.
Can we please get a small announcement in the changelog?

@burner
Copy link
Member Author

burner commented Jan 27, 2015

@Dicebot If you don't mind.
http://forum.dlang.org/post/hmqlrknrjezovnvttxdj@forum.dlang.org should be a good start. thats pretty much what is new.

@joakim-noah
Copy link
Contributor

One note: std.experimental.logger.core leaves behind a file someFile.log when the unittests are run. I'm not sure what that unit test even does, considering it doesn't contain an assert.

@burner
Copy link
Member Author

burner commented Jun 2, 2015

I will have a look:

Update:
can you tell me which OS and in which folder you found the file

update":
#3364

@JackStouffer
Copy link
Member

This package has been in std.experimental for six releases (3 major, 3 minor) and for seven months. What is the time table for moving it?

@mihails-strasuns
Copy link

My original plan was like this : author feels module has matured enough to not undergo any serious changes in future, calls for review manager, standard voting happens for inclusion into next release. I don't know how it is supposed to be handled now as I don't do any review manager duties anymore.

@burner
Copy link
Member Author

burner commented Nov 10, 2015

I wish that logger had been moved to into "stable" already, but http://forum.dlang.org/post/mlhj3t$1bdi$1@digitalmars.com sort of blocked it.
IMO when logger has to wait for RCString, or RC in any other way than what we have right now in phobos it is going to be a long time.

@JackStouffer
Copy link
Member

That's really a shame, it could be a year before a RC scheme is decided on let alone it's implementation being added to DMD.

@burner
Copy link
Member Author

burner commented Nov 10, 2015

it could be a year

I wish I had your optimism. My prediction: RC build into the language will not happen, ever. There will be thread local COW container and the RefCounted will become better. But there will be no ref counted schema that is build into the compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet