*: internal logging package refactor #898
Conversation
internal/lg/lg.go
Outdated
| FATAL = 5 | ||
| ) | ||
|
|
||
| func ParseLogLevel(levelstr string, verbose bool) (int, error) { |
There was a problem hiding this comment.
Here's what I had in mind: https://play.golang.org/p/bUb1ETf4zY
What do you think?
There was a problem hiding this comment.
Yes, I can use a type, but I don't think it helps much.
logger.Output(3, fmt.Sprintf("%s: "+f, msgLevel, args...))doesn't work, go says
../../go/src/github.com/nsqio/nsq/internal/lg/lg.go:66: too many arguments in call to fmt.Sprintf
have (string, LogLevel, []interface {}...)
want (string, ...interface {})
Thus the uglier thing I pushed up.
nsqadmin/options.go
Outdated
| LogPrefix string `flag:"log-prefix"` | ||
| Verbose bool `flag:"verbose"` // for backwards compatibility | ||
| Logger Logger | ||
| logLevel int // private, not really an option |
There was a problem hiding this comment.
Maybe a bit confusing: I did some moving-around to all three, but then used the new internal shared logging only for nsqd, while experimenting.
Since you approve of this style I'll roll it out to the rest (and we'll see some nice line-count savings).
nsqlookupd/options.go
Outdated
| @@ -10,15 +10,15 @@ type Options struct { | |||
| LogLevel string `flag:"log-level"` | |||
| LogPrefix string `flag:"log-prefix"` | |||
| Verbose bool `flag:"verbose"` // for backwards compatibility | |||
| Logger Logger | |||
| logLevel int // private, not really an option | |||
|
@ploxiln this is looking good. It's pretty annoying that we're continuing to support |
mreiferson
changed the title
ploxiln
force-pushed
the
logging_refactor_2
branch
2 times, most recently
from
f66b3f7 to
ce77eb1
Compare
|
nsqadmin and nsqlookupd converted to There's more to do for |
|
👍 |
|
@ploxiln how're you feeling about this? |
|
I think I'm done with changes for this round. Need to squash this down, obviously :) One remaining piece I can find, which has not yet been converted, is the now-separate disk-queue. I'm not sure of the best way to handle that, but it can be in a separate PR IMHO. |
minor fix for --log-level flag help text must set opts before logging errors in New() re-order log stuff in Options structs treat logLevel the same in nsqd, nsqlookupd, nsqadmin Fix tests: multiple nsqlookupds need their own Options structs, now that nsqlookupd has logLevel in its Options. Otherwise the race detector complains when one of the nsqlookupds writes the derived int logLevel while another reads it for logging.
introduce LogLevel type NilLogger for disabling logging consolidate LogLevel tests into internal/lg/ consolidate TestNoLogger
Instead of setting a Logger for github.com/nsqio/nsq/internal packages, pass a logf() function, so it is called with and honors a LogLevel. * internal/clusterinfo/ * internal/http_api/ * internal/protocol/ nsqd lookupPeer also needed to be converted Get rid of interal.app.Logger type, but internal/test/ needs its own Logger definition to avoid circular import with internal/lg/ tests.
ploxiln
force-pushed
the
logging_refactor_2
branch
from
259547b to
c2c139d
Compare
|
Tastefully squashed, sanity-tested a bit with a light random mix of nsqd/nsqlookupd/nsqadmin/nsq_tail usage. Coveralls ratings are annoyingly jittery - probably due to the asynchronous parts of the applications randomly hitting some cases depending on runtime scheduling. |
|
Thanks, nice work. |
follow-up to #892
In-progress. So far just nsqd converted to use the internal package, while we decide on design and naming. Also haven't converted the internal
api_responseandclusterinfopackages to use log levels and such yet (planning to do that after the main parts of nsqadmin and nsqlookupd, by passing a function similar to NSQD.logf() instead of a Logger to them).Critique and better ideas are welcome @mreiferson