Echo: Please don't use the default echo.Logger in framework code

[rocketspacer]

As you know, logging in go is a nightmare to deal with. I already have to deal with a bunch of libraries, each using their own logging library.

Instead of logging arbitrarily using your custom echo.Logger (especially in the middleware package), I suggest defining specific Error types that can be handled centrally (using type assertion at the HTTPErrorHandler maybe?).

A good library/framework shouldn't log anything itself, but instead, pipe the error/event up for the user to decide how or whether to log it.

I really love echo, and I know that this is not echo's fault itself (the standard lib without a universal logging interface makes me want to not log anything at all), but I hope that we can avoid logging within our code in the future, and, if possible, remove all the logging in future versions

[weisd]

+1

[zouhuigang]

+1

[dxvgef]

+1

[renathoaz]

I stumbled at the same thing, I'm doing my application log:
it goes like this:

[2018-01-16 10:35:24.38231] [MiddlewareServer/server.go:147] [INFO ] – Starting....
[2018-01-16 10:35:24.38252] [MiddlewareServer/server.go:148] [ERROR] – some error

Instead of Json format,
so It annoys me having echo sending logs in json format on my log file.
Is there any workaround to it ?
Is it safe to set echo.Logger OFF and use only middleware logger? So I could set my own custom format.

[memwey]

Agree. A middleware logger is enough.

[roman-vynar]

You can easily disable the default json logger:

    e := echo.New()
    e.Logger.SetLevel(99)

Then define you own LoggingMiddleware.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[L11R]

@roman-vynar there is a problem. Okay, I disabled it. But then I want to log something from handler and fill it with usefull info from request, for example. Logger interface is not generic, it developed specifically for gommon/log package :(

[alexaandru]

@roman-vynar could you please detail what you mean by "Logger interface is not generic" ? What exactly could it make "more generic"? As far as I understand it, as long as you have a type that implements that interface it, you can pass it as the logger, and then your type can decide exactly what to log and what to skip.

That being said, the interface is huge, that's a lot to implement...

@vishr @im-kulikov given how many upvotes this story has, I'd reopen it and give it some more thought. Maybe we can find a way to make it easier for end users to implement custom logic for logging... Thoughts?

[vishr]

@vishr @im-kulikov given how many upvotes this story has, I'd reopen it and give it some more thought. Maybe we can find a way to make it easier for end users to implement custom logic for logging... Thoughts?

Sure

[im-kulikov]

I'm close to the idea that the logger interface is too complicated. In my work, I have to implement dummy methods only to fit the interface. Also, the fact that the interface requires the gommon package is a bit depressing. For example, I use uber.zap, in which the logger is set up when it is initialized, and re-set is considered inappropriate. In echo, for some purposes, the user is allowed to do that. What's the point, I don't understand.

[im-kulikov]

If we can provide the ability to use a minimal interface, just what is really needed, that would be great.

[L11R]

@alexaandru it has many not necessary to implement methods, I have to make them do nothing or return empty.

Anyway I fully agree with topic starter, Echo almost perfect framework for me, but stricting to use gommon/log package is pitfall for us. Now we are rewriting project to go-chi.

Echo v5 should return all errors and let user to decide what to do with this error.

[im-kulikov]

I think we were too quick with release v4.

[im-kulikov]

I think... In the future, we should collect more wishes, fix the maximum number of issues and only after diligent testing to roll out a new version.

[L11R]

@im-kulikov well, I guess it's okay that you released v4 without any notable changes except modules. v5 and fewer will be for features.

[alexaandru]

Thanks @L11R and @im-kulikov for your feedback! Alright then let's see what we can do about this for V5... :-)

[runner-mei]

Logger interface is unnecessary. It is rarely used and is not required in the echo.

````
github.com\labstack\echo\echo.go:
func (e *Echo) DefaultHTTPErrorHandler(err error, c Context) {
360 }
361 if err != nil {
362: e.Logger.Error(err)
363 }
364 }
...
659 func (e *Echo) StartServer(s *http.Server) (err error) {
660 // Setup
661: e.colorer.SetOutput(e.Logger.Output())
662 s.ErrorLog = e.StdLogger
663 s.Handler = e
664 if e.Debug {
665: e.Logger.SetLevel(log.DEBUG)
666 }
667

github.com\labstack\echo\middleware\proxy.go:
124 err = <-errCh
125 if err != nil && err != io.EOF {
126: c.Logger().Errorf("proxy raw, copy body error=%v, url=%s", t.URL, err)
127 }
128 })

github.com\labstack\echo\middleware\proxy_1_11.go:
18 desc = fmt.Sprintf("%s(%s)", tgt.Name, tgt.URL.String())
19 }
20: c.Logger().Errorf("remote %s unreachable, could not forward: %v", desc, err)
21 c.Error(echo.NewHTTPError(http.StatusServiceUnavailable))
22 }

github.com\labstack\echo\middleware\recover.go:
71 length := runtime.Stack(stack, !config.DisableStackAll)
72 if !config.DisablePrintStack {
73: c.Logger().Printf("[PANIC RECOVER] %v %s\n", err, stack[:length])
74 }
75 c.Error(err)

github.com\labstack\echo\response.go:
54 func (r *Response) WriteHeader(code int) {
55 if r.Committed {
56: r.echo.Logger.Warn("response already committed")
57 return
58 }
````

Or add a simple logger like:

````go
type Field struct {
Key string
Value interface{}
}

type Logger interface {
Write(level int, msg string, fileds ...Field)
}
````

[danielparks]

I'm not sure this is relevant, but if the default logger outputs something like JSON, then it should actually output JSON. Currently it's just a string that values are inserted into without escaping.

In other words, either do JSON or do something that doesn't look like JSON.

[haneefkassam]

I had to replace gommon logger with logrus (due to missing features in gommon log), but still had to include references to gommon logger and re-implement the types....so cumbersome. It would be great to standardize the logger interface and allow developers/users of echo to add any logger they wish.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ghostsquad]

Probably not stale...

[vishr]

@haneefkassam

I had to replace gommon logger with logrus (due to missing features in gommon log), but still had to include references to gommon logger and re-implement the types....so cumbersome. It would be great to standardize the logger interface and allow developers/users of echo to add any logger they wish.

Don't we have Logger interface in the framework? https://github.com/labstack/echo/blob/master/log.go#L11

[ghostsquad]

It would be great to standardize the logger interface and allow developers/users of echo to add any logger they wish.

If this is going to be done, I would strongly recommend not using the built-in go logger methods as a base for the interface. There are many examples out there that talk about the anti-pattern of using log methods as control flow, as is such with fatal and panic methods. Additionally, I've seen many threads about having too many or not enough logging levels.

go-logr is an opinionated logger interface (used by Kubernetes), which defines only info and error methods, and it's up to the implementer to make additional decisions (such as choosing not to log when extra fields such as "debug: true" are set) https://github.com/go-logr/logr/blob/master/logr.go

The basis of this interface is derived from the post made by Dave Cheney: https://dave.cheney.net/2015/11/05/lets-talk-about-logging

type InfoLogger interface {
    // Info logs a non-error message with the given key/value pairs as context.
    //
    // The msg argument should be used to add some constant description to
    // the log line.  The key/value pairs can then be used to add additional
    // variable information.  The key/value pairs should alternate string
    // keys and arbitrary values.
    Info(msg string, keysAndValues ...interface{})

    // Enabled tests whether this InfoLogger is enabled.  For example,
    // commandline flags might be used to set the logging verbosity and disable
    // some info logs.
    Enabled() bool
}

// Logger represents the ability to log messages, both errors and not.
type Logger interface {
    // All Loggers implement InfoLogger.  Calling InfoLogger methods directly on
    // a Logger value is equivalent to calling them on a V(0) InfoLogger.  For
    // example, logger.Info() produces the same result as logger.V(0).Info.
    InfoLogger

    // Error logs an error, with the given message and key/value pairs as context.
    // It functions similarly to calling Info with the "error" named value, but may
    // have unique behavior, and should be preferred for logging errors (see the
    // package documentations for more information).
    //
    // The msg field should be used to add context to any underlying error,
    // while the err field should be used to attach the actual error that
    // triggered this log line, if present.
    Error(err error, msg string, keysAndValues ...interface{})

    // V returns an InfoLogger value for a specific verbosity level.  A higher
    // verbosity level means a log message is less important.  It's illegal to
    // pass a log level less than zero.
    V(level int) InfoLogger

    // WithValues adds some key-value pairs of context to a logger.
    // See Info for documentation on how key/value pairs work.
    WithValues(keysAndValues ...interface{}) Logger

    // WithName adds a new element to the logger's name.
    // Successive calls with WithName continue to append
    // suffixes to the logger's name.  It's strongly reccomended
    // that name segments contain only letters, digits, and hyphens
    // (see the package documentation for more information).
    WithName(name string) Logger
}

or even simpler, is in go-kit https://github.com/go-kit/kit/blob/master/log/log.go

type Logger interface {
    Log(keyvals ...interface{}) error
}

With some additional methods available from the library such as With, WithPrefix.

[rcdmk]

@haneefkassam

I had to replace gommon logger with logrus (due to missing features in gommon log), but still had to include references to gommon logger and re-implement the types....so cumbersome. It would be great to standardize the logger interface and allow developers/users of echo to add any logger they wish.

Don't we have Logger interface in the framework? https://github.com/labstack/echo/blob/master/log.go#L11

Yes, we have, but have you noticed the gomon import right there? It uses gomon types for levels and JSON.

A better approach would be to abstract that into extended basic types. That would allow for removing the gomon dependency.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.