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

promlog: Fix caller #334

Merged
merged 3 commits into from
Oct 20, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix caller
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]>
roidelapluie committed Oct 13, 2021
commit f8a4d9b9de1ab11fef1e1c9594a2737cc241e19b
17 changes: 12 additions & 5 deletions promlog/log.go
Original file line number Diff line number Diff line change
@@ -118,10 +118,11 @@ func New(config *Config) log.Logger {
l = log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
}

l = log.With(l, "ts", timestampFormat, "caller", log.DefaultCaller)

if config.Level != nil {
l = log.With(l, "ts", timestampFormat, "caller", log.Caller(5))
l = level.NewFilter(l, config.Level.o)
} else {
l = log.With(l, "ts", timestampFormat, "caller", log.DefaultCaller)
}
return l
}
@@ -136,15 +137,16 @@ func NewDynamic(config *Config) *logger {
} else {
l = log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
}
l = log.With(l, "ts", timestampFormat, "caller", log.DefaultCaller)

lo := &logger{
base: l,
leveled: l,
}

if config.Level != nil {
lo.SetLevel(config.Level)
}

return lo
}

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? Looks like l.base is initialised with the caller field already set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_ = l.base.Log("msg", "Log level changed", "prev", l.currentLevel, "current", lvl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
l.currentLevel = lvl
} else {
l.leveled = log.With(l.base, "ts", timestampFormat, "caller", log.DefaultCaller)
l.currentLevel = nil
return
}
l.leveled = level.NewFilter(l.base, lvl.o)
l.leveled = level.NewFilter(l.leveled, lvl.o)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just make the previous assignment to leveled the function parameter?

Suggested change
l.leveled = level.NewFilter(l.leveled, lvl.o)
l.leveled = level.NewFilter(log.With(l.base, "ts", timestampFormat, "caller", log.Caller(5)), lvl.o)

}