apache / incubator-pagespeed-mod

Apache module for rewriting web pages to reduce latency and bandwidth.
http://modpagespeed.com
Apache License 2.0
696 stars 158 forks source link

Improve the messages generated with `debug` in ModPagespeedEnableFilters #1086

Open kezabelle opened 9 years ago

kezabelle commented 9 years ago

Background

Whilst trying to get mod_pagespeed working within a vagrant-issued virtualbox image, I encountered a number of messages ostensibly meant to help me debug why things weren't working. Off the back of a somewhat snarky tweet of mine, it was suggested that I raise a ticket to track any possibility of these messages being improved.

Ultimately, I think my issue probably boiled down to having my Host machine proxying port 1080 to the Guest machine, where Apache was listening on port 80, though mod_pagespeed was then trying to do an HTTP sub-request to Guest:1080, where nothing was listening.

Unfortunately, I didn't document the error messages as I encountered them, so I'm not entirely sure of the filter combinations that led to those I'll discuss below; I did google them, however, and it's from my browser history from which I picked the specifics back out.

Messages encountered

4xx status code, preventing rewriting of [url]

As a debug aid, it would be much easier to narrow down the problem's scope if the error code was not masked. Off the top of my head, I could imagine 401, 403, 404, 408 being returned in such a scenario, the most likely two of which being 403 and 404 - knowing if it were a permissions issue, a timeout, or a non-existent file would really help figure out what might need amending.

Fetch failed to start [url]

It's not clear what a fetch is, nor why it might've failed to do whatever it was supposed to. I think a fetch means an HTTP sub-request by default? Possibly it applies even if it's reading from disk via ModPagespeedLoadFromFile? Replacing fetch with a less amorphous descriptive would presumably make googling for related issues easier, and if it does apply to more than just one type (ie: HTTP request and read from disk) may improve debuggability on this issue tracker and any mailing lists etc. Knowing what a fetch is might make the failed to start part clearer, though any improvement that could be made there would, I expect, be useful - presumably there are conditions that must be met for a fetch to start OK, so composing the conditions which failed into the debug message would be ideal.

Fetch status not set when IsSafeToRewrite was called

I don't know what IsSafeToRewrite is. I don't know what the fetch status could be, or should be, and like the previous error message, I'm not sure what a fetch represents. This last message, when googled, ultimately just led me to the pagespeed source, and I'm not savvy in C++ or whatever it's written in.

Addendum

I appreciate that pagespeed is ultimately a tool better targeted at ISPs, CDNs and the like, rather than one-shot users like myself who merrily wade into territories just for funsies, so there is probably a valid expectation that users are better-versed in Apache and pagespeed when it comes to debugging. However, the rise of roll-your-own-servers in the shape of AWS, CoreOS, Docker etc probably mean more isolated instances of pagespeed being deployed, and by less knowledgable folk (ie: me) than has traditionally been the case.

I ultimately managed to get the situation sorted by dropping the whole vagrant-proxy setup and spinning up a real staging server, which was orders of magnitude easier because it transpired I was only one or two rules away from having it all working (specifically, ModPagespeedMapOriginDomain and ModPagespeedLoadFromFileMatch), so this ticket exists only to see if the messages may be improved for others.

jeffkaufman commented 9 years ago

Thanks for the detailed feedback!

4xx status code, preventing rewriting of [url]

The exact error code would be much more helpful here, yes. Unfortunately the code that reports the error doesn't know the status code, because that's already been dropped, but we may be able to adjust the API to pass this though.

Fetch failed to start [url]

This is another case where we do have more detail, we're just not passing it along to where we'd need to report it. I'll try to fix this too.

Fetch status not set when IsSafeToRewrite was called

We're fixing this one now; I agree that it doesn't make any sense if you don't already know the code.

I appreciate that pagespeed is ultimately a tool better targeted at ISPs, CDNs and the like, rather than one-shot users like myself who merrily wade into territories just for funsies, so there is probably a valid expectation that users are better-versed in Apache and pagespeed when it comes to debugging.

Smaller users are also within the range of what we'd like to support, so feedback like yours is really helpful.

jeffkaufman commented 9 years ago

Fetch failed to start [url]

This wasn't a EnableFilters=+debug message, was it? Looking at the code it looks like something that would go in the error log. And looking at the code I think we'll print other debug messages right before it saying why the fetch didn't start?

kezabelle commented 9 years ago

This wasn't a EnableFilters=+debug message, was it?

You're probably right. Given you know what you're talking about, let's go with that :) Whilst I know I got at least 3 distinct errors out of the HTML comments, it's pretty feasible that I've plucked one out of my browser history which was after I enabled the admin via URI endpoint and subsequently googled some of those messages.

I will attempt to find the other one I encountered - I feel like it overlapped with those listed - I think it may've involved fetch and prevented rewriting, but that's no certainty, and I'd assume most errors would prevent rewriting. Apologies for the goose chase.


Presumably there are a finite number of error messages. Perhaps a longer-term goal could be to enumerate them all and either improve them (for a given value of improve), or have a FAQ page or admin URI endpoint which clarifies their meaning if it cannot be done inline or in the console history?

jeffkaufman commented 9 years ago

Perhaps a longer-term goal could be to enumerate them all and either improve them (for a given value of improve), or have a FAQ page or admin URI endpoint which clarifies their meaning if it cannot be done inline or in the console history?

Clarifying them is definitely doable. The main reason we have vague messages, though, is that the place where we need to explain the failure is several steps along from the place where the failure happened, and information you need for a good informative error message isn't around anymore. I'm trying to fix some of these currently, but some of these are pretty tricky.