When the origin server returns 404 for a file its treated as and logged as an error. In reality this can be caused by lots of normal site behaviors that are not errors like removing a locally uploaded file from the dev environment while there is still a link, CSS cache misses, typos on a document, etc.

It would be nice if the log level was lower or configurable so these could be handled as less serious messages.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

I'm happy to write up a patch but I'm not sure about the best approach. Specifically:

Should the level just be lower or should it be configurable?
Should this be only 40x responses or all non 200 responses?

greggles’s picture

Priority: Minor » Normal

I think making it configurable is too much.

I'd be fine with just lowering it a notch. I think it would be fine to do for all 4xx/5xx responses. What log level seems right to you?

I'm curious if you can share - in what way does this cause a problem for you?

neclimdul’s picture

Status: Active » Needs review
FileSize
562 bytes

"Problem" seems a bit overkill but I send error level logs to NewRelic and a flood of these trigger error thresholds. I also used this in a production site to "migrate" files from an old D6 where there was a _very_ large amount of cruft in the files directory but also random files linked from marketing emails, external marketing pages, etc that where hard to impossible to track down. That site also had random index bots or something that would hit it with garbage. The crying wolf started getting to me so I thought I'd file an issue. :)

I think a good argument could be made for them to be the same level as "access denied" and "page not found" which is warning.

Status: Needs review » Needs work

The last submitted patch, 4: 3086884-4-stagefileproxy-warning.patch, failed testing. View results

greggles’s picture

Status: Needs work » Needs review

Seems fine to me. I'll leave this here for a bit in case others have thoughts. The test "fail with successful build" is b/c we don't have any tests.

neclimdul’s picture

I applied this and it didn't quite work as well as I expected. Or rather exposed a quirk of how the fetch works. Probably a bug.

FetchManager::fetch() logs messages when something fails and return false. Unless it doesn't know what happened and then it just return false. That mostly makes sense.

The code that calls fetch in ProxySubscriber then looks at the return. If its false it says "and unknown error occurred."

The problem is that basically all cases are handled and log specific errors already. That means if I'm reading everything correctly, there are 2 errors logged for most problem and the patch only lowers that to one.

I think the fix is just to move the message into the fetch method where its useful.

Status: Needs review » Needs work

The last submitted patch, 7: 3086884-7-stagefileproxy-warning.patch, failed testing. View results

neclimdul’s picture

Status: Needs work » Needs review
greggles’s picture

Status: Needs review » Reviewed & tested by the community

Interesting research, thanks. That would unnecessarily fill logs. Your change makes sense to me. Still leaving uncommitted in case others want to review.

  • greggles committed 6b14e10 on 8.x-1.x authored by neclimdul
    Issue #3086884 by neclimdul: Lower log level or setting for missing...
greggles’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your help, neclimdul!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.