Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LogLevelSetter to recover middleware #2072

Merged
merged 2 commits into from Jan 24, 2022
Merged

Conversation

ant1k9
Copy link
Contributor

@ant1k9 ant1k9 commented Jan 17, 2022

I've come across the situation when I need to distinguish log levels for different recovered values.

For example, I want to handle all panics as ERRORs except situations when I've got panic(ErrAbortHandler). It may happen in the ProxyWithConfig middleware when the client aborts the request.

For now, we have the LogLevel and LogLevelSetter both in the RecoverConfig to not break compatibility and introduce a new feature to the recover middleware.

Usage:

config := DefaultRecoverConfig
config.LogLevelSetter = func(value interface{}) log.Lvl {
  if err, ok := value.(error); ok {
    if errors.Is(err, http.ErrAbortHandler) {
      return log.WARN
    }
  }
  return log.ERROR
}

@aldas
Copy link
Contributor

aldas commented Jan 17, 2022

maybe adding something like that to config LogErrorFunc func(c echo.Context, err error, stack []byte) error

and allowing completely custom handling with:

			defer func() {
				if r := recover(); r != nil {
					err, ok := r.(error)
					if !ok {
						err = fmt.Errorf("%v", r)
					}
					var stack []byte
					var length int

					if !config.DisablePrintStack {
						stack = make([]byte, config.StackSize)
						length = runtime.Stack(stack, !config.DisableStackAll)
						stack = stack[:length]
					}

					if config.LogErrorFunc != nil {
						err = config.LogErrorFunc(c, err, stack)
					} else if !config.DisablePrintStack {
						msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack)
						switch config.LogLevel {
						case log.DEBUG:
...
...

and create middleware as

	e.Use(middleware.RecoverWithConfig(middleware.RecoverConfig{
		LogErrorFunc: func(c echo.Context, err error, stack []byte) error {
			msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack)
			if errors.Is(err, http.ErrAbortHandler) {
				c.Logger().Warn(msg)
			} else {
				c.Logger().Error(msg)
			}
			return err
		},
	}))

it could maybe even made to return an error instead of resulting to c.Error(err) which stops handler chain returning errors upstream middlewares.

@aldas
Copy link
Contributor

aldas commented Jan 17, 2022

Although recover middleware is extremely simple middleware it has uses when you want to log something with stack. If you are not interested in stack there absolutely no need to use Echo provided middleware as it your custom one is vert easy to create. If you want to log stack I think it would be better to give users means to choose how logging is done. This is more flexible as to give them ability to choose only log level.

@aldas
Copy link
Contributor

aldas commented Jan 17, 2022

This is example for handling recovered errors - lets say more correctly

func main() {
	e := echo.New()

	e.Use(middleware.Logger())

	customRecovery := func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) (err error) {
			defer func() {
				if r := recover(); r != nil {
					rErr, ok := r.(error)
					if !ok {
						rErr = fmt.Errorf("%v", r)
					}
					// c.Logger().Error(rErr)  // you do not need to log here. Logger middleware is up in chain.
					err = rErr // allows upstream middlewares to receive recovered error
				}
			}()
			return next(c)
		}
	}
	e.Use(customRecovery)

	e.GET("/", func(c echo.Context) error {
		panic(errors.New("test"))
	})

	log.Fatal(e.Start(":8080"))
}

LogErrorFunc provides more general interface to handle errors
in the recover middleware.
@ant1k9
Copy link
Contributor Author

ant1k9 commented Jan 17, 2022

Thanks for your suggestions!

This is more flexible as to give them ability to choose only log level.

I like your idea of a more general function in the RecoverConfig. It solves our problem!

Although recover middleware is extremely simple middleware it has uses when you want to log something with stack. If you are not interested in stack there absolutely no need to use Echo provided middleware as it your custom one is vert easy to create.

We use stack traces because they provide more information for identifying the core of the problem, so why I propose to extend the config. For now, we create a copy of the current middleware for our purposes.

This is example for handling recovered errors - lets say more correctly

You're right, it's not too elaborate to create a custom middleware, but I search the compromise between adding some customization for the service and finding a way to customize an existing solution in the echo repo.

@ant1k9
Copy link
Contributor Author

ant1k9 commented Jan 17, 2022

I've commited the new version of code and applied your ideas.

@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

Merging #2072 (4730deb) into master (aada6f9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2072      +/-   ##
==========================================
+ Coverage   91.57%   91.59%   +0.01%     
==========================================
  Files          33       33              
  Lines        2921     2927       +6     
==========================================
+ Hits         2675     2681       +6     
  Misses        157      157              
  Partials       89       89              
Impacted Files Coverage Δ
middleware/recover.go 84.21% <100.00%> (+2.96%) ⬆️
echo.go 94.20% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d2c45e...4730deb. Read the comment docs.

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lammel
Copy link
Contributor

lammel commented Jan 23, 2022

Nice, we wanted other formatting in one of our apps too, guess we could make use of that too.

@ant1k9 ant1k9 closed this Jan 24, 2022
@ant1k9 ant1k9 reopened this Jan 24, 2022
@lammel
Copy link
Contributor

lammel commented Jan 24, 2022

@ant1k9 Thanks for your contribution!

@lammel lammel merged commit 7c41b93 into labstack:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants