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

global SetLevel #26

Open
jbenet opened this issue Oct 1, 2014 · 6 comments
Open

global SetLevel #26

jbenet opened this issue Oct 1, 2014 · 6 comments

Comments

@jbenet
Copy link

jbenet commented Oct 1, 2014

We've multiple loggers, but would be nice to set the level of all of them in one place (e.g. normally all are at Error, but can bump one down specifically to debug that portion.

logging.SetLevel(logging.ERROR, "") // or "*"
@op
Copy link
Owner

op commented Oct 1, 2014

That sounds like a great feature.

I like the idea of thinking of this as the argument beeing a prefix, eg. "".

The level logic would not set all loggers beneath it to a level but the
opposite; when fetching the level, stop when a defined level is found,
splitting the module name by '/'.

@jbenet
Copy link
Author

jbenet commented Oct 2, 2014

@op sgtm. In that case, I suggest prefixing everything with /. Many systems make the mistake of removing the leading slash (favoring things like "foo/bar", "foo/bar/baz"), but pathing systems have the leading / for good reasons, like namespacing/mounting.

Extending this analogy, i could totally see this making sense:

log1 := logging.MustGetLogger("/")
log2 := logging.MustGetLogger("/foo")
log3 := logging.MustGetLogger("/foo/bar")

logging.SetLevel(logging.ERROR, "/foo") // log2 and log2 are now at logging.ERROR

Or:

log1 := logging.RootLogger()  // has prefix of "/"
log2 := logging.NewLogger(log1, "foo")  // has prefix /foo
log3 := logging.NewLogger(log2, "bar") // has prefix /foo/bar

logging.SetLevel(logging.ERROR, "/foo") // log2 and log2 are now at logging.ERROR

Logger Tree

Note that up there I'm constructing loggers explicitly, whereas you prefer static loggers (GetLogger). It's your module and probably should think thrice before breaking the interface, but I think constructing Loggers explicitly makes sense. This is particularly apparent when instantiating objects in a module twice, and wanting to redirect their logging separately.

I think go-logging could work phenomenally well with a logger-tree model similar to how google's context module works. If you haven't seen it, check it out. it's really a great piece of interface design. http://godoc.org/code.google.com/p/go.net/context

@op
Copy link
Owner

op commented Oct 2, 2014

@jbenet That's worth a thought. Why do you want to have relative paths for loggers though? I believe that might lead to code that is harder to reason about. Very flexible, I agree.

I've been thinking of going the opposite route. Somehow, make it possible to just say logging.New(), and have that automatically figure out the package path and re-use that.

Keep the ideas coming. :)

@jbenet
Copy link
Author

jbenet commented Oct 7, 2014

Why do you want to have relative paths for loggers though? I believe that might lead to code that is harder to reason about. Very flexible, I agree.

On the contrary. Static initialziation is very hard to reason about / deal with when changing the package structure or instantiation of things. I'd follow the context example.

logging.New(), and have that automatically figure out the package path and re-use that.

seems too magic to me. sometimes i don't care about logging per package, but --say-- per incoming request. (useful to change logging level on specific requests across packages)

@jbenet
Copy link
Author

jbenet commented Oct 8, 2014

One more reason. being able to do this (on a per-request, or whatever, basis):

log := logger.WithPrefix("[Peer: %s] ", local.peer.ID())

log.Debug("trying to do something with %s and %s, with %s", thisThing, thatThing, anotherPeer.ID())
// [Peer: abcbacbacbbacbabc] trying to do something with aaaaa and bbbbb, with dfdffdfdfdffdfdfdfd

log.Error("something bad happened: %s", err)  
// [Peer: abcbacbacbbacbabc] something bad happened: all the things broke.

Instead of, everywhere:

log.Debug("[Peer: %s] trying to do something with %s and %s, with %s", local.peer.ID(), thisThing, thatThing, anotherPeer.ID())
// [Peer: abcbacbacbbacbabc] trying to do something with aaaaa and bbbbb, with dfdffdfdfdffdfdfdfd

log.Error("[Peer: %s] something bad happened: %s", local.peer.ID(), err)  
// [Peer: abcbacbacbbacbabc] something bad happened: all the things broke.

@rasky
Copy link

rasky commented Dec 9, 2014

Instead of "WithPrefix", I would check the design of structlog and go that way:

log := log.Bind("Peer", local.peer.ID())
log = log.Bind("Other", foobar)
log.Debug("doing something")

so basically store a map[string]interface{} and dump that as a prefix. You need a value semantic for return values, but it's more powerful because you can even overwrite an object by simply rebinding it. Moreover, it opens the door to having a json.formatter to dump logs in a machine-parsable format.

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

No branches or pull requests

3 participants