Closed Bug 1409920 Opened 2 years ago Closed 7 months ago

add 404 as a filter option for the network panel

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox57 wontfix, firefox58 wontfix, firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox68 --- fixed

People

(Reporter: zac.spitzer, Assigned: tanhengyeow, Mentored)

Details

Attachments

(1 file, 1 obsolete file)

It would be really handy to be able easily to drill down to any 404 requests in the network panel via a toolbar filter option. Searching for 404 doesn't work 

I have filed the same request for Chrome
https://bugs.chromium.org/p/chromium/issues/detail?id=775913
as the comment from Chrome team, you could filter with `status-code:404` on Firefox as well.

Check https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor
Filtering by properties section

The feature is there but the problem might because we didn't provide a handy reference to empower the user.

Victoria, not sure if it worth to add MDN link somewhere so user could learn these advanced filter feature?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(victoria)
The filter toolbar item could also show a total count of 404 items as well, otherwise, you might not even
know there is a problem.

If you are working on the development version of a large web app with a lot of individual files, 
you might not even see that there is a 404 being thrown.

advanced filtering is useful, but this is a pretty common workflow and it would be nice to have a
one click button instead of having to type out a filter expression each and every time.

I did try just searching for 404. which seemed like a reasonable option, but that didn't work either
After discussion with Honza & Ricky, we can add 200/302/404 directly as the filter option, so the filter will work as expect when user type 200 or 404 to filter requests.

We can parse these flag in 
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/filter-autocomplete-provider.js#156

Here's all the status code
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/mdn-utils.js#95

welcome to contribute
Just for the sake of not being too specific on one status case, I would expand this to

Success: 2xx
Redirects: 3xx
Errors: 4xx

Adding redirects as they are important on mobile and slow connections, where latency makes every roundtrip costly. Ads and ad-bidding are also bad offenders as requests are redirected between bidders.

Just as an idea, given how much meaning the numbers have, we can probably even use the 2xx style for labels.
Being able to directly search for status codes sounds great! 

(In reply to Fred Lin [:gasolin] from comment #1)
> Victoria, not sure if it worth to add MDN link somewhere so user could learn
> these advanced filter feature?

Re: MDN links, will consider if this could still be added somewhere near the filter input.

(In reply to Zac Spitzer from comment #2)
> The filter toolbar item could also show a total count of 404 items as well,
> otherwise, you might not even
> know there is a problem.

That's an interesting thought. I wonder if we should just sort the table by highest status code, by default. I guess the current default sorting (by load time) is important too though, for bandwidth reasons.
Flags: needinfo?(victoria)
(In reply to Fred Lin [:gasolin] from comment #3)
> After discussion with Honza & Ricky, we can add 200/302/404 directly as the
> filter option, so the filter will work as expect when user type 200 or 404
> to filter requests.
> 
> We can parse these flag in 
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> utils/filter-autocomplete-provider.js#156
> 
> Here's all the status code
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> utils/mdn-utils.js#95

To summarize what should be done in this bug:
1) If the user type a number into the filter bar it'll be considered as status-code
and the list will be filtered accordingly.

For instance:
`200`
is the same as:
`status-code:200`

2) If the use types `status-code:4` the list will be filtered so *all* requests with
error status code will be displayed (including 400, 404, etc.)

Examples:
`status-code:4` -> all errors displayed
`status-code:3` -> all redirects displayed
`status-code:2` -> all success requests displayed

Honza
Hi Honza,
I have taken a stab at this issue and have created a review-request. 
Although, I have fixed a failing test(which was failing due to my changes), I would still need to add more tests which would be covering this new functionality.
I would add the test after a first round of review. Can you please check if this is what you had in mind?
@Ricky: can you please review the patch? (my review queue is quite long these days)

Thanks!
Honza
Flags: needinfo?(rchien)
Comment on attachment 8927594 [details]
Bug 1409920 - Add status-codes, partial status-codes as filter options in the network panel.

https://reviewboard.mozilla.org/r/198884/#review205824

I appreciate all your hard work Abhinav Koppula!

I've verfied behavior does work as expected as well as running on launchpad. Overall changes look good to me.

Here is my concern before review approval:

Couldn't we display status-code: 200 in autocomplete list when typing 200? For instance:

User types "2" in filter input

status-code: 200
status-code: 201
status-code: 202
status-code: 203
...

instead of merely number. Sometime I feel a bit confused since autocomplete shows number without any hits.

What do you think?

::: devtools/client/netmonitor/src/constants.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const { SUPPORTED_HTTP_CODES } = require("../src/utils/mdn-utils");

I'm curious about why SUPPORTED_HTTP_CODES is defined in mdn-utils.js instead of constants.js?

Let's move SUPPORTED_HTTP_CODES to here since it makes more sense to share with other modules when needed.

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:59
(Diff revision 1)
>      let colonIndex = part.indexOf(":");
>      if (colonIndex === -1) {
> +      let negative = part.startsWith("-");
> +      let key = negative ? SUPPORTED_HTTP_CODES.indexOf(part.substring(1)) :
> +        SUPPORTED_HTTP_CODES.indexOf(part);
> +      if (key != -1) {

nit: !==

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:63
(Diff revision 1)
> +        SUPPORTED_HTTP_CODES.indexOf(part);
> +      if (key != -1) {
> +        flags.push({
> +          type: "status-code",
> +          value: negative ? part.substring(1) : part,
> +          negative: negative

nit: using negative is enough.

flags.push({
  type: "status-code",
  value: negative ? part.substring(1) : part,
  negative,
});

::: devtools/client/netmonitor/src/utils/mdn-utils.js:198
(Diff revision 1)
>  module.exports = {
>    getHeadersURL,
>    getHTTPStatusCodeURL,
>    getNetMonitorTimingsURL,
>    getPerformanceAnalysisURL,
> +  SUPPORTED_HTTP_CODES

nit: adding tailing comma `,` to avoid messing up commit history.
Attachment #8927594 - Flags: review-
BTW, Honza is busy in other review requests, feel free to forward and revise reviewer to me in your commit message. Thanks!
Flags: needinfo?(rchien)
> instead of merely number. Sometime I feel a bit confused since autocomplete shows number without any hits.

Network also doesn't autocomplete other words. The numbers are just another meta data field that gets searched. I'd be OK to accept that behavior as is and file a follow up bug.
Comment on attachment 8927594 [details]
Bug 1409920 - Add status-codes, partial status-codes as filter options in the network panel.

https://reviewboard.mozilla.org/r/198884/#review205824

> nit: !==

I have changed the logic a bit in this part.
Previously, I was doing an exact match now I have changed it to `item.toLowerCase().startsWith(part.toLowerCase());`.
Reason: Previously, typing `20` wasn't filtering out the requests whose status-codes start with 20.
Now, the output of typing `20` matches with the output of typing `status-code:20`
Comment on attachment 8927594 [details]
Bug 1409920 - Add status-codes, partial status-codes as filter options in the network panel.

https://reviewboard.mozilla.org/r/198884/#review205824

Hi Ricky,
I have updated the review-request and have changed the auto-suggestion logic to show `status-code:200`,`status-code:201` etc on typing 2.
Assignee: nobody → abhinav.koppula
Comment on attachment 8927594 [details]
Bug 1409920 - Add status-codes, partial status-codes as filter options in the network panel.

https://reviewboard.mozilla.org/r/198884/#review209252

Nice! it looks better now. I think it's getting close.

One more thing, we should filter and only display existing status results. While I type: `2`, we're showing all status-code results which start with 2. However, the expected result should be inconsistent with `status-code: 2`, which only display existed status-code in reuqests. You have to add a filter to strip out those redundant results.
Attachment #8927594 - Flags: review?(rchien) → review-
Product: Firefox → DevTools

Allow number to be mapped to "status-code:<number>"

Hi Honza

I updated the patch, included meaningful comments and also addressed Comment 16 made by Ricky. However, I realized that Comment 16 contradicts with point 2 of Comment 6.

Would like your input on this.

Flags: needinfo?(odvarko)
Assignee: abhinav.koppula → E0032242
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Attachment #8927594 - Attachment is obsolete: true

Hi Honza

Added test for the patch as seen in Phabricator. Ready for final review :)

Try results here for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d335312453d7c5667c56e25c74ae3c9d136b7462

Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a61000074155
add 404 as a filter option for the network panel. r=Honza
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Flags: needinfo?(odvarko)
You need to log in before you can comment on or make changes to this bug.