Skip to content

Commit

Permalink
Don't bother with checking for go-kit/log stack frames until we've se…
Browse files Browse the repository at this point in the history
…en SpanLogger

This makes the cost of spanlogger.Caller negligible for use outside of
SpanLogger:

                            │ SpanLoggerAwareCaller/with_go-kit's_Caller-10 │ SpanLoggerAwareCaller/with_dskit's_spanlogger.Caller-10 │
                            │                    sec/op                     │                 sec/op                  vs base         │
comparison-after-step-2.txt                                     927.5n ± 5%                              932.1n ± 2%  ~ (p=0.093 n=6)

                            │ SpanLoggerAwareCaller/with_go-kit's_Caller-10 │ SpanLoggerAwareCaller/with_dskit's_spanlogger.Caller-10 │
                            │                     B/op                      │                  B/op                   vs base         │
comparison-after-step-2.txt                                      732.5 ± 0%                               733.0 ± 0%  ~ (p=0.636 n=6)

                            │ SpanLoggerAwareCaller/with_go-kit's_Caller-10 │ SpanLoggerAwareCaller/with_dskit's_spanlogger.Caller-10 │
                            │                   allocs/op                   │              allocs/op                vs base           │
comparison-after-step-2.txt                                      9.000 ± 0%                             9.000 ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal
  • Loading branch information
charleskorn committed Oct 10, 2024
1 parent 647f153 commit 9e662ca
Showing 1 changed file with 14 additions and 3 deletions.
17 changes: 14 additions & 3 deletions spanlogger/spanlogger.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ func (s *SpanLogger) SetSpanAndLogTag(key string, value interface{}) {
func Caller(defaultStackDepth int) log.Valuer {
return func() interface{} {
stackDepth := defaultStackDepth
seenSpanLogger := false

for {
_, file, line, ok := runtime.Caller(stackDepth)
Expand All @@ -206,12 +207,22 @@ func Caller(defaultStackDepth int) log.Valuer {
return "<unknown>"
}

// If we're in this file, or log.go in the go-kit/log package, we need to continue searching in the stack.
//
// If we're in this file, or log.go in the go-kit/log package after having seen this file, we need to continue searching in the stack.
if strings.HasSuffix(file, "dskit/spanlogger/spanlogger.go") {
seenSpanLogger = true
stackDepth++
continue
}

// The path to log.go varies depending on how the package is imported:
// - if go-kit/log is vendored, the path is something like <project root>/vendor/github.com/go-kit/log/log.go
// - if it is not vendored, the path is something like /home/user/go/pkg/mod/github.com/go-kit/[email protected]/log.go and varies based on the version imported
if strings.HasSuffix(file, "dskit/spanlogger/spanlogger.go") || (strings.HasSuffix(file, "/log.go") && strings.Contains(file, "github.com/go-kit/log")) {
//
// We need to check for go-kit/log stack frames because log.With, log.WithPrefix or log.WithSuffix (including the various level methods like level.Debug,
// level.Info etc.) to wrap a SpanLogger introduces an additional context.Log stack frame that calls into the SpanLogger. This is because the use of SpanLogger
// as the logger means the optimisation to avoid creating a new logger in https://github.com/go-kit/log/blob/c7bf81493e581feca11e11a7672b14be3591ca43/log.go#L141-L146
// used by those methods can't be used, and so the SpanLogger is wrapped in a new logger.
if seenSpanLogger && (strings.HasSuffix(file, "/log.go") && strings.Contains(file, "github.com/go-kit/log")) {
stackDepth++
continue
}
Expand Down

0 comments on commit 9e662ca

Please sign in to comment.