Change the log level for 4xx HttpExceptions? #7993
Conversation
|
What about a |
|
👎 as I consider 404 (and 40x) as real errors that I have to monitor and maybe fix, if it is accepted I will have to monitor all notices (the ones i don't really care right now) |
|
@mpdude I personally don't think that backporting the log level change is a great idea, we often have reporting tools configured to report on specific exceptions and if the log levels change its introducing a potential break in a system's monitoring logic. I'm 100% behind the log level change for 2.3+ though |
|
@jaitsu87 This change would only be considered for 2.3+ anwyay @pborreli Have you ever monitored 404 errors? Even on a not-so-popular website, you have a bunch of script-kiddies attacks (resulting in many 404s), bots that requests stupid URLs, and more. So, it means that logging 404 at the error levels gives you a lot of noise. |
|
@fabpot thanks my admins those attacks are filtered way before reaching the webserver (firewall + nagios, rules in nginx or Varnish) all the 404 I have are errors in css, images, missing actions or errors in my applications (yes it happens a lot :p), but yes I don't have much "public" websites with lot of traffic, it's more backends. |
|
Doesn't it make sense to follow the log level web servers use (which is |
|
👎 From my point of view 40x responses should be considered as either errors or at least warnings, definitely not notices. Also, depending on what your business is (I am thinking about big SEO-friendly websites and aggregators, with complicated rewrite rules), monitoring 404 is really important (even though it is true that there is a lot of noise with script-kiddies attacks). And 👎 also to treat this a bug fix. |
|
@mpdude Can you remove the log level change from this PR as the other part is a no-brainer and can be merged? Thanks. |
|
I totaly agree with ricardclau . |
|
from my point of view the log levels should be configurable as requested in #7972. |
|
+1 for 404 error... It becomes headache in prod.log file and real errors are hard to find. Making it configurable is a good solution I think... |
|
How about fixing symfony/monolog-bundle#5 instead of changing the error level to a notice? It would be a good thing to have and if flexible enough (like take an array of URL patterns to whitelist/blacklist) it could fit the needs of most people, or be disabled completely. |
|
Well, as @mpdude says 404 can be also monitored in apache log files and most of the times you just need the url that is failing, not the full Symfony2 exception trace, so I would go 👍 on treating 404 differently. And if it is configurable in case someone wants to have it in a higher level than notice, even better But please not for other 4xx What do you think @pborreli @mickaelandrieu? |
|
Well, exception traces can be usefull for 404 errors (if those are thrown by the Symfony application). |
|
404 errors can be real errors, but chances are slim. Being able to remove them from my logs with a configured error level is a 👍 |
|
@ricardclau in your application you don't need to monitor throw $this->createNotFoundException('Page not found for whatever reason');or AccessDeniedException ? When someone tries to access sensitive information the apache log is enough ? you don't care if it's someone already connected, account and more ? I guess we all have different kinds of application from the public website to sensitive backend or API who need lot of attention.. |
|
Thank you all for a really good discussion! I think it is pretty clear now that there are good reasons for both log levels and for or against this change. So let's please focus on sketching possible solutions that comfort everyone.
Other ideas? |
|
@pborreli I said "most of the times you just need the url that is failing" and "should work for most cases" not that I did not need to monitor those examples you are saying :). I was mostly thinking about these big sitemap websites, but of course as you say, if the exception comes from the application it should be treated at least as a warning. As you have said in your previous post, you mostly deal with backends but trust me that when you deal with public websites there is a lot of crap about script-kiddies attacks, stuff like what @rdohms is saying and at the end it becomes really painful to dig into it Anyway, there are a lot of points of view and valid arguments, so probably going for the configurable level for 404 is the better approach for everyone. Maybe the default value could be kept as it is right now, so no BC, and also giving the ability to lower the level. This way you could even set it to error for some minutes, watch some 404 for some time and set to warning or notice afterwards so that logs don't grow forever with lots of favicon problems But @fabpot seems to be 👎 for configurable level, so let's see how this goes in the end :) |
|
Sorry @mpdude you just wrote almost at the same time than me! |
|
Thanks, missed that! Comment updated. |
|
@ricardclau @mpdude I originally thought dropping 404 to notice was a good idea, but having read through the arguments wouldn't it be better to keep 404s at the error level, remove exception traces for |
|
I think making the level configurable makes sense, as it's up to the application developer to decide which events are critical, and which can be handled in other ways. If making it configurable is not an option, it should be lowered to warning or less. It's quite likely that there's nothing wrong with my application, even though someone requests a page that doesn't exist. I would consider 404 as something that you might want to monitor, but not something that necessarily demands immediate action. |
|
I'd like to hear @fabpot's opinion on a configurable level. How powerful or simple does it have to be? |
|
I vote -1 for the configurable level. The FingersCrossedHandler already supports complex logic to determine if it should be triggered through the ActivationStragegyInterface (using a level-based implementation by default). As the exception is now available in the context, building the activation strategy skipping the 404 HttpException is very simple. So it should be implemented and shipped in the bridge IMO (solving symfony/monolog-bundle#5). This way, people can choose to log 404 in prod or to skip them by changing the activation strategy. |
|
@stof are you for downgrading 400 to notice level ? |
|
I have a bad gut feeling with a special case for 404 (instead of 4xx) only... I don't like such special rules, fwiw. Be it configuration for the ExceptionListener or the FingersCrossedHandler's ActivationStrategy, people end up with more or less powerful configuration settings they need to master (and we'd need to support in the long run). In case of the ActivationStrategy suggestion, you'd also have to use FingersCrossedHandler to make it work for you. That's not a downvote though. Opinions on a dedicated logging channel? Maybe with |
|
@mpdude the FingersCrossedHandler is used in the default config. |
|
How do you think should this strategy be best configured, assuming its not for 404 only? And it would still have to be log level-aware for non-HttpExceptions, that needs to be configurable as well... |
|
👍 I want 404s out of my |
|
Having thought about it a little, changing the ActivationStrategy means logging the exception on a higher level and then treating it like a lower level again when it comes to taking action on it. IMO that shows that you don't really want that particular exception to be an So here's the next suggestion: Refactor log level selection into a dedicated protected method in the ExceptionListener. Folks can subclass the ExceptionListener and override a single method where they can choose the level. The subclass can probably easily be injected by changing a class name parameter. Deluxe variant: Make that a dedicated |
This is only true if you assume that an ActivationStrategy works on |
|
@richardclau: jaitsu87 resume perfectly my opinion. We don't need traces for 404's, but IMO it does'nt make sens to downgrad error level of 400's server error to notice. |
|
So we're currently discussing two ways of making it "configurable" (more or less) by writing and injecting code and/or having generic implementations that can be parametrized? Could we please gather the pros/cons for both approaches:
I guess both are straightforward and should look similar. @Seldaek @stof Do you have any suggestions how configuration for the ActivationStrategy could look like? It probably should look for an exception in the context and fall back to the log level if there is none? |
|
Closing as the MonologBundle now has support for tweaking logging of 404 errors. |
|
Excellent news. @fabpot is referring to this commit which also contains instructions on how to configure it once you've installed MonologBundle 2.4 (in your |
|
Excellent news indeed! And quite a nice discussion throughout the issue! |
(Updated at 2013-05-10 13:15 UTC)
The initial motivation was in #7972 which I closed in favor of this one as we've got a much more valuable discussion here. Other changes nobody objected to have already been merged as #8000.
My POV is that HttpExceptions with 4xx status codes (and 404 in particular) are pretty normal events in webapps. Dynamic content comes and goes, people mistype URLs, browsers look for favicons, spiders want the robots.txt...
RFC2616 uses 4xx "for cases in which the client seems to have erred". The application detects that, handles it gracefully by telling the client and showing a nice error page - and all is well from a user experience perspective.
Having such exceptions at the
errorlevel means a lot of clutter in log files unless you use the FingersCrossedHandler at a level above that - which in turn would miss otherwarninglevel things.For me, the
applog channel is for application-level events, to record states and conditions in my application. In order to track "popular" 404s and user experience (for example, a page many folks suddenly miss), we're using the webserver access log files.I chose the
noticelevel to leave a bit more of options below so you don't get all the info if you want to get the HttpExceptions.But as the following discussion shows, clearly there are good reasons why higher log levels might be appropriate as well.