Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
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
8 changes: 8 additions & 0 deletions log/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,16 @@ var locationTrims = []string{
// format output.
func PrintOrigins(print bool) {
locationEnabled.Store(print)
if print {
stackEnabled.Store(true)
}
}

// storeStackRecords is an atomic flag controlling whether the log handler needs
Comment thread
holiman marked this conversation as resolved.
Outdated
// to store the callsite stack. This is needed in case any handler wants to
// print locations (locationEnabled), use vmodule, or print full stacks (BacktraceAt).
var stackEnabled atomic.Bool

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

linter's gonna lint :P field and comment name don't match


// locationEnabled is an atomic flag controlling whether the terminal formatter
// should append the log locations too when printing entries.
var locationEnabled atomic.Bool
Expand Down
8 changes: 6 additions & 2 deletions log/handler_glog.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ func (h *GlogHandler) Vmodule(ruleset string) error {
h.patterns = filter
h.siteCache = make(map[uintptr]Lvl)
h.override.Store(len(filter) != 0)

// Enable location storage (globally)
if len(h.patterns) > 0 {
stackEnabled.Store(true)
}
return nil
}

Expand Down Expand Up @@ -172,7 +175,8 @@ func (h *GlogHandler) BacktraceAt(location string) error {

h.location = location
h.backtrace.Store(len(location) > 0)

// Enable location storage (globally)
stackEnabled.Store(true)
return nil
}

Expand Down
9 changes: 6 additions & 3 deletions log/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,19 +177,22 @@ type logger struct {
}

func (l *logger) write(msg string, lvl Lvl, ctx []interface{}, skip int) {
l.h.Log(&Record{
record := &Record{
Time: time.Now(),
Lvl: lvl,
Msg: msg,
Ctx: newContext(l.ctx, ctx),
Call: stack.Caller(skip),
KeyNames: RecordKeyNames{
Time: timeKey,
Msg: msgKey,
Lvl: lvlKey,
Ctx: ctxKey,
},
})
}
if stackEnabled.Load() {
record.Call = stack.Caller(skip)

@rjl493456442 rjl493456442 Sep 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's problematic. e.g. in the log/handler_glog.go, L188, we assume this is available.

But I agree it's a good optimization, but just need more checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, interesting. I dug into this, it turns out that r.Call.String() == %!v(NOFUNC).
due to

// String implements fmt.Stinger. It is equivalent to fmt.Sprintf("%v", c).
func (c Call) String() string {
	return fmt.Sprint(c)
}

So there's no crash, just it won't print the backtrace.

@holiman holiman Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be noted, though: the Call is stack.Call -- not a pointer. Nothing is nil here. And it goes on

type Call struct {
	frame runtime.Frame
}

(no pointers)

type Frame struct {
	// PC is the program counter for the location in this frame.
	// For a frame that calls another frame, this will be the
	// program counter of a call instruction. Because of inlining,
	// multiple frames may have the same PC value, but different
	// symbolic information.
	PC uintptr
...

This struct too is (nearly) pointer-free.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ha I spent two hours yesterday trying to create a test to hit this, but couldn't because no pointer. So it looks good to me

}
l.h.Log(record)
}

func (l *logger) New(ctx ...interface{}) Logger {
Expand Down
68 changes: 68 additions & 0 deletions log/logger_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package log

import (
"bytes"
"os"
"strings"
"testing"
"time"
)

func BenchmarkTraceLogging(b *testing.B) {
Root().SetHandler(LvlFilterHandler(LvlInfo, StreamHandler(os.Stderr, TerminalFormat(true))))
b.ResetTimer()
for i := 0; i < b.N; i++ {
Trace("a message", "v", i)
}
}

type notimeHandler struct {
next Handler
}

func (n notimeHandler) Log(r *Record) error {
r.Time = time.Unix(0, 0)
return n.next.Log(r)
}

// TestLoggingWithTrace checks that if BackTraceAt is set, then the
// gloghandler is capable of spitting out a stacktrace
func TestLoggingWithTrace(t *testing.T) {
defer stackEnabled.Store(stackEnabled.Load())
out := new(bytes.Buffer)
logger := New()
{
glog := NewGlogHandler(StreamHandler(out, TerminalFormat(false)))
glog.Verbosity(LvlTrace)
if err := glog.BacktraceAt("logger_test.go:42"); err != nil {
t.Fatal(err)
}
logger.SetHandler(notimeHandler{glog})
}
logger.Trace("a message", "foo", "bar") // Will be bumped to INFO
have := out.String()
wantPrefix := "INFO [01-01|01:00:00.000] a message\n\ngoroutine"
if len(have) < len(wantPrefix) || !strings.HasPrefix(have, wantPrefix) {
t.Errorf("\nhave: %q\nwant: %q\n", have, wantPrefix)
}
}

// TestLoggingWithVmodule checks that vmodule works.
func TestLoggingWithVmodule(t *testing.T) {
defer stackEnabled.Store(stackEnabled.Load())
out := new(bytes.Buffer)
logger := New()
{
glog := NewGlogHandler(StreamHandler(out, TerminalFormat(false)))
glog.Verbosity(LvlCrit)
logger.SetHandler(notimeHandler{glog})
logger.Warn("This should not be seen", "ignored", "true")
glog.Vmodule("logger_test.go=5")
}
logger.Trace("a message", "foo", "bar")
have := out.String()
want := "TRACE[01-01|01:00:00.000] a message foo=bar\n"
if have != want {
t.Errorf("\nhave: %q\nwant: %q\n", have, want)
}
}