samber / slog-fiber

🚨 Fiber middleware for slog logger
https://pkg.go.dev/github.com/samber/slog-fiber
MIT License
53 stars 9 forks source link

feat: add support for fiber.ErrorHandler logging #14

Closed samber closed 5 months ago

samber commented 10 months ago

This PR allows to move the logger into *fiber.ErrorHandler:

Use either:

NewXXX constructors have been renamed to NewMiddlewareXXX. Previous functions are marked as deprecated.

Via error handler

app := fiber.NewMiddleware(fiber.Config{
        // ...
    ErrorHandler: slogfiber.NewErrorHandler(logger.WithGroup("http")),
})

Via middleware

app.Use(slogfiber.NewMiddleware(logger))
codecov-commenter commented 10 months ago

Codecov Report

Attention: 147 lines in your changes are missing coverage. Please review.

Comparison is base (38a183c) 0.00% compared to head (bfa4885) 0.00%.

Files Patch % Lines
middleware.go 0.00% 87 Missing :warning:
error-handler.go 0.00% 39 Missing :warning:
config.go 0.00% 15 Missing :warning:
deprecated.go 0.00% 6 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #14 +/- ## ===================================== Coverage 0.00% 0.00% ===================================== Files 2 5 +3 Lines 331 380 +49 ===================================== - Misses 331 380 +49 ``` | [Flag](https://app.codecov.io/gh/samber/slog-fiber/pull/14/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Samuel+Berthe) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/samber/slog-fiber/pull/14/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Samuel+Berthe) | `0.00% <0.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Samuel+Berthe#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

daenney commented 9 months ago

Sorry, it took me a bit longer to check on this.

The Error handler works and now all the context attributes I set do show up in the log line. That's great.

However, there's still two issues:

Also, for requests that are found, this no longer logs anything (since the error handler never executes). That makes sense, but it makes me wonder why there's the "error handler is now preferred" in the README since it seems like you would need both?

I'm really puzzled by the status thing since that's definitely not slogfiber messing it up. It's just the value on ctx.Response().StatusCode() which I really don't expect to be a 200 if we're in the error handler. My guess is that in the error handler this has to be explicitly extracted from the err passed to it, instead of using c.Response().StatusCode() since the error handler is expected to write the reply on the wire.

UncleVic commented 7 months ago

I think so, we should handle an error like that

        status := c.Response().StatusCode()
        var e *fiber.Error
        if errors.As(err, &e) {
            status = e.Code
        }
samber commented 5 months ago

Closing in favor of #19