promlog: Fix caller #334
promlog: Fix caller #334
Conversation
DefaultCaller is hard-coded to select the 3rd stack frame up, but we first filter by level, which means we need to take a 4th frame. Signed-off-by: Julien Pivotto <[email protected]>
|
cc @bboreham |
promlog/log.go
Outdated
| @@ -167,10 +169,15 @@ func (l *logger) SetLevel(lvl *AllowedLevel) { | |||
| l.mtx.Lock() | |||
| defer l.mtx.Unlock() | |||
| if lvl != nil { | |||
| if l.currentLevel != nil && l.currentLevel.s != lvl.s { | |||
| if (l.currentLevel != nil && l.currentLevel.s != lvl.s) || l.currentLevel == nil { | |||
| l.leveled = log.With(l.base, "ts", timestampFormat, "caller", log.Caller(5)) | |||
There was a problem hiding this comment.
Does this work? Looks like l.base is initialised with the caller field already set.
There was a problem hiding this comment.
If I understand the code correctly, line 139 being deleted sets up the caller but doesn't pay attention to whether or not the level is set, so the caller is going to be wrong if the level is in fact set.
If the user sets the level after creating the dynamic logger, the caller is also going to be wrong.
So instead of setting up the caller in NewDynamic, set it up in SetLevel so that it's correct in both cases.
LGTM.
promlog/log.go
Outdated
| @@ -167,10 +169,15 @@ func (l *logger) SetLevel(lvl *AllowedLevel) { | |||
| l.mtx.Lock() | |||
| defer l.mtx.Unlock() | |||
| if lvl != nil { | |||
| if l.currentLevel != nil && l.currentLevel.s != lvl.s { | |||
| if (l.currentLevel != nil && l.currentLevel.s != lvl.s) || l.currentLevel == nil { | |||
| l.leveled = log.With(l.base, "ts", timestampFormat, "caller", log.Caller(5)) | |||
| _ = l.base.Log("msg", "Log level changed", "prev", l.currentLevel, "current", lvl) | |||
There was a problem hiding this comment.
If I understand the logic correctly, this message will now also be logged when a new dynamic logger is created. I don't think that's expected.
Signed-off-by: Julien Pivotto <[email protected]>
promlog/log.go
Outdated
| } | ||
| l.leveled = level.NewFilter(l.base, lvl.o) | ||
| l.currentLevel = lvl | ||
| l.leveled = level.NewFilter(l.leveled, lvl.o) |
There was a problem hiding this comment.
Could we just make the previous assignment to leveled the function parameter?
| l.leveled = level.NewFilter(l.leveled, lvl.o) | |
| l.leveled = level.NewFilter(log.With(l.base, "ts", timestampFormat, "caller", log.Caller(5)), lvl.o) |
Signed-off-by: Julien Pivotto <[email protected]>
|
thanks! |
DefaultCaller is hard-coded to select the 3rd stack frame up, but we
first filter by level, which means we need to take a 5th strack frame.
Signed-off-by: Julien Pivotto [email protected]