labstack/gommon

Why set runtime caller to 3?

Mr-Dai opened this issue · 10 comments

I was trying out echo, and I used e.Logger to as my application's logger, but I found that file locations in the log output were wrong, they all reported the upper layers of their real locations.

Then I found this commit, which changed runtime.Caller(2) to runtime.Caller(3). I set it back to 2, then everything works just fine.

I wonder why it was set to 3 back then.

vishr commented

@Mr-Dai Can you provide an example to debug?

Test file main.go:

package main

import (
    "github.com/labstack/echo"
    "github.com/labstack/gommon/log"
)

func main() {
    e := echo.New()
    e.Logger.SetLevel(log.INFO)
    e.Logger.Infof("Hello, World!")
}

Run it with go run main.go, got:

{"time":"2018-03-12T10:57:54.8390043+08:00","level":"INFO","prefix":"echo","file":"proc.go","line":"185","message":"Hello, World!"}

This is proc.go, basically it's where the go runtime invokes the main function.

vishr commented

Thanks for the information, I have reverted the changed.

@vishr I have the issuse with caller level 2, because I use the log command:

import (
   "github.com/labstack/gommon/log"
)
func main() {
   log.Info("Hello, World!")
}

With caller level 3 all is fine again. All my projects where I used the logger show the wrong file and line now

This code in log.go could fix it:

_, file, line,_ := runtime.Caller(2)
    if strings.Contains(file, "github.com/labstack/gommon/log/log.go") {
        _, file, line,_ = runtime.Caller(3)
    }
vishr commented

@alexandrestein Can you please help me with it?

I have the same problem in my project.

func main() {
// Echo instance
e := echo.New()

// Middleware
e.Use(middleware.Logger())
e.Use(middleware.Recover())

//correct line etc
e.Logger.Info("Testlog")
//wrong line etc
log.Info("Wrong log")

}

e.Logger.Info produces correct line number:
{"time":"...","level":"INFO","prefix":"echo","file":"main.go","line":"20","message":"Testlog"}

log.Info produces wrong file and line number:
{"time":"...","level":"INFO","prefix":"-","file":"log.go","line":"285","message":"Wrong log"}

baba2k`s change fixes it locally.

@baba2k I think the better way to fix this is to store an extraLevel field in logger struct, which defaults to 0, and change the runtime.Caller invocation in log.go to

_, file, line, _ := runtime.Caller(2 + l.extraLevel)

Another way to do this is to add a calldepth parameter to the log method, like the log.Output method from the standard library.

Point is, this should become a generic parameter/configuration, instead of just adding special processing just for invocations from github.com/labstack/gommon/log/log.go.

@vishr I made a pull request about this issue #21.

The problem is that when you use the default logger you add an other layer.
So you need to switch between level 3 and 2 if it's default logger or not.

Look at the pull request and check the test file I call the default logger directly.

I think that merging the pull request would solve the issue.

Hey @vishr.
It does fix the issue of the caller level.

But same kind of issue could be reintroduce in the future.

In the test, you are not checking that the give response display the correct file or line.
I think this must be check to prevent any future bug related to this.

vishr commented

@alexaandru thanks for pointing that out, I will put the test back soon.