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

[rkt2spc]

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.

[savely-krasovsky]

@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.

[savely-krasovsky]

@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.

[savely-krasovsky]

@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... :-)

[mei-rune]

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:

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.

[aldas]

As changing Logger is breaking change it will not happens in v4. As popular logging frameworks all have their own different methods or ways to use fluent (ala zerolog) vs functions etc there are no one interface to use.

You do not need to use Echo.Logger or Context.Logger(). As Logger interface/instance is used in few places in code it should not be a problem to use your own implementations instead.

Places where you could see logger used and have some use to you:

• echo.DefaultHTTPErrorHandler - just use custom echo.HTTPErrorHandler instead.
• Context.Logger() - in handlers etc call your own logger instance or functions where you need to. No need to use c.Logger()
• Recover middleware - set RecoverConfig.DisablePrintStack = true and logger will not use used and error is passed to echo.HTTPErrorHandler
• Logger middleware - set LoggerConfig.Output value to your logger (assuming it implements writer interface)

Also there are implementations where logrus is wrapped into Logger interface. ala https://github.com/plutov/echo-logrus

I'll reiterate - due backwards comparability we can not just go around and change it any time soon and you have to work around opinions that Echo has about logging, for now.

p.s. this does not mean that situation with Logger may not change in v5.

[lammel]

I'd also vote for removing the dependency on gommon/log, minimizing the interface of the internal logger and allow to configure log output to a wrapper implementing this minimal interface. We definitely should consider that for v5