ziflex / lecho

Zerolog wrapper for Echo framework 🍅
MIT License
99 stars 11 forks source link

fix: expose zerologger on context instead of lecho #13

Closed matelang closed 3 years ago

matelang commented 3 years ago

Hi,

Thanks for maintaining this library.

We have observed that there is support for propagating the request ID through the standard context.Context, like the zerolog library supports it.

We think that the support for such propagation is not working as expected, since it breaks the expectation of a plain zerolog user of having a zerolog logger instance on the context, since the lecho.Logger is saved inside the middleware. As a consequence client code calling zerolog.Logger.log.Ctx(ctx) will not receive a logger and no log events will be generated.

Imagine an example codebase having API and a servicer struct. Somewhere in the API handler you have

// ...
func (h *ApiHandler) GetFoo(ctx echo.Context) error {
    resp, err := h.fooServicer.Get(ctx.Request().Context())

In FooServicer:

import "github.com/rs/zerolog/log"

func (p *Servicer) Get(ctx context.Context) (*Foo, error) {
    log.Ctx(ctx).Info().Msg("something")

Expectation is that the log line above produces a log event, optionally with a requestId present (should the request ID middleware be mounted).

Actually, nothing happens for the reasons described above.

This PR fixes this scenario.

ziflex commented 3 years ago

Hey, yeah, it makes sense. Thanks for taking care of it! Since it's a breaking change in the API I will release it as the next major version.

matelang commented 3 years ago

Thanks for reviewing and merging it so quickly. 🍾