go-kit / log

A minimal and extensible structured logger
MIT License
185 stars 18 forks source link

Logging error type that panic on Error() will break json format #19

Closed dwiyanr closed 2 years ago

dwiyanr commented 2 years ago

Hi, i found that logging error that cause panic when Error() func called, will cause json logger implementation to break. in contrast, logfmt format implementation safely output the log even if its failed to log the error description. i also put a sample code to replicate the issue on the bottom.

i think the safeError() & safeString on json format implementation should always recover from panic and instead return the panic description that happen when its encode the value into string.

i can help to provide a pr for it if you agree with my suggestion. thanks in advance!

package main

import (
    kitlog "github.com/go-kit/log"
    "os"
)

type myerror struct {
}

func main() {
    // will safely output log even its failed
    log := kitlog.NewLogfmtLogger(os.Stdout)
    log.Log("err", myerror{})

    // will panic
    log = kitlog.NewJSONLogger(os.Stdout)
    log.Log("err", myerror{})
}

func (e myerror) Error() string {
    panic("fail retrurn error value")
    return "test"
}

output:

go run main.go
err="PANIC:fail retrurn error value"
panic: fail retrurn error value [recovered]
    panic: fail retrurn error value

goroutine 1 [running]:
github.com/go-kit/log.safeError.func1()
    /home/runner/go/pkg/mod/github.com/go-kit/log@v0.2.0/json_logger.go:85 +0x134
...
ChrisHines commented 2 years ago

Thanks for the report. Yes, I would review a PR for this if you submit one.