Skip to content
This repository has been archived by the owner on May 9, 2019. It is now read-only.

Logger log levels are off: please fix them or make them customizable #139

Closed
ddossot opened this issue Oct 16, 2014 · 4 comments
Closed

Comments

@ddossot
Copy link
Contributor

ddossot commented Oct 16, 2014

Currently the Logger middleware uses the following mapping:

    if (status >= 500) {
        logger.fatal(message);
    } else if (status >= 400) {
        logger.error(message);
    } else if (status >= 300) {
        logger.warn(message);
    } else {
        logger.info(message);
    }

This is problematic when you use a monitoring system that inspects log lines because client errors (4xx) are reported at the ERROR level, which is typically the level at which server errors are logged. This leads to a mix of genuine server side errors and client side errors.

Moreover 3x responses are perfectly fine, there's no reason to log them at WARN level.

So IMO the "correct" mapping should be:

    if (status >= 500) {
        logger.error(message);
    } else if (status >= 400) {
        logger.warn(message);
    } else {
        logger.info(message);
    }

I can totally understand that you may not want to change the existing implementation so could you make it possible to alter the the current behaviour either by:

  • extracting the above code in a protected method that a sub-class can implement differently,
  • or making the mapping configurable.

Thank you for considering this 😸

@pmlopes
Copy link
Owner

pmlopes commented Oct 17, 2014

I do like the protected override method approach, can you make a PR? :)

@ddossot
Copy link
Contributor Author

ddossot commented Oct 17, 2014

Here you go: #140

pmlopes added a commit that referenced this issue Oct 18, 2014
#139 extracted message logging into protected method
@pmlopes
Copy link
Owner

pmlopes commented Oct 18, 2014

Cool, thanks!

@pmlopes pmlopes closed this as completed Oct 18, 2014
@ddossot
Copy link
Contributor Author

ddossot commented Oct 27, 2014

Any hope to have this in 2.0.8?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants