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

Shouldn't middleware.Logger's default output to be the same as echo.Logger #1260

Closed
nattawitc opened this issue Jan 18, 2019 · 6 comments · Fixed by #1336
Closed

Shouldn't middleware.Logger's default output to be the same as echo.Logger #1260

nattawitc opened this issue Jan 18, 2019 · 6 comments · Fixed by #1336

Comments

@nattawitc
Copy link
Contributor

Issue Description

The current behavior of middleware.Logger() is kind of counter-intuitive. When I set echo.Logger.SetOutput to be something else, I expect all log from echo should go to them. But right now if I use echo.Use(middleware.Logger()), the log will still write to os.Stdout.

I think the DefaultLoggerConfig.Output should be set to nil, and at the end of middleware.LoggerWithConfig, if config.Output == nil then use context.Logger.Output() instead

@alexaandru
Copy link
Contributor

At the glance, this sounds reasonable to me, however it also sounds backward incompatible. If all of the sudden stdout logging is gone, that might break some setups. Esp., that it is the recommended way of logging for Docker, so people may actually rely on it quite a bit.

So I'm with you, I just don't see an easy way to handle this. Maybe keep it in mind for V4? Thoughts @vishr ?

@andradei
Copy link

V4 is here but just for compatibility with Go Modules. V5 then?

@vishr
Copy link
Member

vishr commented Feb 20, 2019

I think

I think the DefaultLoggerConfig.Output should be set to nil, and at the end of middleware.LoggerWithConfig, if config.Output == nil then use context.Logger.Output() instead

I am ok with this change, please send a PR.

@stale
Copy link

stale bot commented Apr 21, 2019

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 stale bot added the wontfix label Apr 21, 2019
@vishr vishr added v5 and removed wontfix labels Apr 21, 2019
@nattawitc
Copy link
Contributor Author

After trying to fix this issue, I've stumbled upon some weird unit test.

In the function TestLoggerIPAddress that call assert.Contains(t, ip, bb), it checks whether bb (which is an empty string for all 3 cases) is contained in ip which is what we put in. Shouldn't it be the opposite, that we check for an ip if it is contained in the resulting logs bb. Is this intentional or a typo, because this test failed for my change.

@stale
Copy link

stale bot commented Jun 21, 2019

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 stale bot added the wontfix label Jun 21, 2019
@stale stale bot closed this as completed Jun 28, 2019
vishr pushed a commit that referenced this issue Jul 18, 2019
* fix TestLoggerIPAddress reverse assertion

* change middleware.Logger default output

* remove nil field declaration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants