Add IP address column to the Network Monitor

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P3
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Daniel Morante, Assigned: Ruturaj Vartak)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

51 Branch
Firefox 55
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8843647 [details]
firefox.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Inspect the current page using the context menu item "Inspect Element", click on the network tab, then reload the page.


Actual results:

I see columns for several items including domain.  I do not see a column for IP address.
The IP address is shown only as a tooltip.  I am unable to sort columns by IP address.

See attachment "firefox.png"


Expected results:

A column should appear for the IP address.  Allowing you to sort and/or be able to identify loaded resources by IP address.  This is critical for debugging DNS related issues.

See attachment 'firebug.png"
(Reporter)

Comment 1

a year ago
Created attachment 8843648 [details]
firebug.png
(Reporter)

Updated

a year ago
Attachment #8843647 - Attachment description: firefox → firefox.png

Updated

a year ago
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Netmonitor
Ever confirmed: true
Priority: -- → P3
(Assignee)

Comment 2

a year ago
Is the IP is already available ? without writing any code on C / C++.
Flags: needinfo?(odvarko)
(Assignee)

Comment 3

a year ago
- Its available, hover (tooltip / title) of the domain name shows the IP Address [1]
- Should the RemoteIP column be listed next to the domain column before the "Cause" column ?

[1] - https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/components/request-list-item.js#284
Flags: needinfo?(odvarko) → needinfo?(jsnajdr)
(Assignee)

Comment 4

a year ago
I was willing to take this, needed few clarifications regarding the UI, If we add the RemoteIP column

- the exact position of the column (comment#3)
- would we completely remove the tooltop (title) from the domain ?
- Any tooltip associated with this list-item.
- Any other rendering / beautification / color associated to the item, or plain old "<IP>:<Port>" should suffice?
Flags: needinfo?(ntim.bugs)

Comment 5

a year ago
(In reply to Ruturaj Vartak from comment #3)
> - Its available, hover (tooltip / title) of the domain name shows the IP
> Address [1]
> - Should the RemoteIP column be listed next to the domain column before the
> "Cause" column ?
Makes sense to have it right after Domain.

> - would we completely remove the tooltop (title) from the domain ?
It would make sense with the new column.

> - Any tooltip associated with this list-item.
Not sure what useful info we could add there.

> - Any other rendering / beautification / color associated to the item, or plain old "<IP>:<Port>" should suffice?
<IP>:<Port> should be fine.


Though I don't know whether we want a new column. We have more columns than Firebug, so we might not have space for a new IP column.

Honza ?
Flags: needinfo?(ntim.bugs) → needinfo?(odvarko)
(In reply to Tim Nguyen :ntim from comment #5)
> Though I don't know whether we want a new column. We have more columns than
> Firebug, so we might not have space for a new IP column.
Yes, the problem is with the horizontal space. I am happy to have new columns but, we need a way tho show/hide them. See bug 862855. 

Honza
Depends on: 862855
Flags: needinfo?(odvarko)
Depends on: 991806

Comment 7

a year ago
Note that I'm currently working on bug 862855, so this bug should be actionable once that's done.

Comment 8

a year ago
Ruturaj, are you interested in working on this now that bug 862855 is done ?
Flags: needinfo?(jsnajdr) → needinfo?(ruturaj)

Updated

a year ago
Keywords: dev-doc-needed
(Assignee)

Comment 9

a year ago
Yes, ill pick it up.
Flags: needinfo?(ruturaj)
(Assignee)

Updated

a year ago
Assignee: nobody → ruturaj
(Assignee)

Comment 10

a year ago
Hey Tim,

As of 10.30AM IST Apr 5 (5.00AM UTC) I don't see any UI feature for bug#862855 - I'm on master@bceaacd2b

I don't see any right click interaction with Headers of the Network list that'll provide for Add / remove cols.

I'm not sure if your patch has been pushed in master branch, I see bug#1350228 as ur last commit.
Flags: needinfo?(ntim.bugs)

Comment 11

a year ago
(In reply to Ruturaj Vartak from comment #10)
> Hey Tim,
> 
> As of 10.30AM IST Apr 5 (5.00AM UTC) I don't see any UI feature for
> bug#862855 - I'm on master@bceaacd2b
> 
> I don't see any right click interaction with Headers of the Network list
> that'll provide for Add / remove cols.
> 
> I'm not sure if your patch has been pushed in master branch, I see
> bug#1350228 as ur last commit.

Ah, bug 862855 is cuurently only on autoland, it should arrive in mozilla-central (master) anytime today. You should be able to apply the patch manually though, and work on top of it.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 12

a year ago
Nice! that worked, thanks.
(Assignee)

Comment 13

a year ago
Created attachment 8856074 [details] [diff] [review]
WIP-fix-1344523.patch

This is a WIP patch, for verification if I'm in the right direction.

- This patch currently fails 2 tests
- I'll be adding few more tests for sort and filtering
Attachment #8856074 - Flags: review?(ntim.bugs)
(Assignee)

Comment 14

a year ago
Created attachment 8856075 [details]
Preview of RemoteIP implementation

- Is it OK to not have remoteIP is certain requests?
- Also "RemoteIP" label has been selected from Firebug
- Currently the port is not being shown, since we already have a Green padlock for domain, should I also show the port?
Flags: needinfo?(ntim.bugs)

Comment 15

a year ago
Comment on attachment 8856074 [details] [diff] [review]
WIP-fix-1344523.patch

Review of attachment 8856074 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! This is going in the right direction, here are a few minor comments.

::: devtools/client/locales/en-US/netmonitor.properties
@@ +411,5 @@
>  # in the network table toolbar, above the "domain" column.
>  netmonitor.toolbar.domain=Domain
>  
> +# LOCALIZATION NOTE (netmonitor.toolbar.domain): This is the label displayed
> +# in the network table toolbar, above the "domain" column.

nit: fix this comment.

@@ +412,5 @@
>  netmonitor.toolbar.domain=Domain
>  
> +# LOCALIZATION NOTE (netmonitor.toolbar.domain): This is the label displayed
> +# in the network table toolbar, above the "domain" column.
> +netmonitor.toolbar.remoteip=RemoteIP

Should be "Remote IP" (with a space). Firebug also uses a space here.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +1307,5 @@
>    }
>  
>    .requests-list-status {
>      max-width: none;
> +    width: 8vw;

hmm, width: 8wv seems like duplicated code from above, let's just remove the width declaration.

::: devtools/client/netmonitor/src/reducers/ui.js
@@ +22,5 @@
>    status: true,
>    method: true,
>    file: true,
>    domain: true,
> +  remoteip: true,

We probably don't want this enabled by default atm to avoid taking up more space initially. We can potentially re-evaluate later which columns to show by default though.

So, let's keep this to false for now.

You might want to adjust the following files for that:
- devtools/client/preferences/devtools.js
- devtools/client/netmonitor/src/middleware/prefs.js

::: devtools/client/netmonitor/src/utils/filter-predicates.js
@@ +103,5 @@
>  }
>  
> +function isFreetextMatch({ url, remoteAddress = "" }, text) {
> +  // This ensures search is for URL and RemoteIP
> +  let lowerCaseUrl = url.toLowerCase() + remoteAddress.toLowerCase();

I would rather fix this with bug 1041895, and have it as an operator. Let's leave this aside for now.
Attachment #8856074 - Flags: review?(ntim.bugs) → feedback+

Comment 16

a year ago
(In reply to Ruturaj Vartak from comment #14)
> Created attachment 8856075 [details]
> Preview of RemoteIP implementation
> 
> - Is it OK to not have remoteIP is certain requests?
Hmm, I'm surprised some requests don't have a Remote IP. Anyway, that's fine for now.

> - Also "RemoteIP" label has been selected from Firebug
Firebug has a space between Remote and IP, so let's add that as well.

> - Currently the port is not being shown, since we already have a Green
> padlock for domain, should I also show the port?

The green padlock does not necessarily represent the port. I think we should also show the port in this case.
Flags: needinfo?(ntim.bugs)

Comment 17

a year ago
Btw, I've added more tests that test column showing/hiding in bug 1353380. So you might want to have that locally to make sure that you don't regress anything (no need to add extra testcases though).
(Assignee)

Comment 18

a year ago
Hey Tim,
Thanks for the review, I'll work on the changes.

It seems a lot is going on with the search :) personally i think `status-code:` / gmail kinda search would be over-engineering. There ain't a big worksheet with multiple rows in the network tab that I'd need such complex tools like specific filtering, etc..

Comment 19

a year ago
(In reply to Ruturaj Vartak from comment #18)
> It seems a lot is going on with the search :) personally i think
> `status-code:` / gmail kinda search would be over-engineering. There ain't a
> big worksheet with multiple rows in the network tab that I'd need such
> complex tools like specific filtering, etc..

I want to bring search to be in parity with Chrome (in fact I've made sure the flag names match those from chrome).
(Assignee)

Comment 20

a year ago
I forgot to ask - I had set the width of RemoteIP col to 8vw and other cols' width I had reduced from 10vw to 8vw so that the total would be 100%. But if the remoteIP col is going to be disabled by default, what should I set its width?
Flags: needinfo?(ntim.bugs)

Comment 21

a year ago
(In reply to Ruturaj Vartak from comment #20)
> I forgot to ask - I had set the width of RemoteIP col to 8vw and other cols'
> width I had reduced from 10vw to 8vw so that the total would be 100%. But if
> the remoteIP col is going to be disabled by default, what should I set its
> width?

Same as domain sounds good.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 22

a year ago
Created attachment 8856206 [details] [diff] [review]
fix-1344523-1.patch

- using "Remote IP" as label
- using "IP:Port" as the item value
- Adding "remoteip" as default hidden column
- Ignoring filter/search, as its gonna be reworked in another bug
Attachment #8856074 - Attachment is obsolete: true
Attachment #8856206 - Flags: review?(ntim.bugs)

Comment 23

a year ago
Comment on attachment 8856206 [details] [diff] [review]
fix-1344523-1.patch

Review of attachment 8856206 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks mostly fine, I will do some manual testing before r+ ing this.

Do you have access to the try server? https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

(Please request access if you don't, I can vouch you)

::: devtools/client/netmonitor/src/middleware/prefs.js
@@ +35,5 @@
>            .map(([column, shown]) => column);
>          break;
>  
>        case RESET_COLUMNS:
> +        Prefs.hiddenColumns = ["remoteip"];

I would prefer removing this bit of code, and replace:

case TOGGLE_COLUMN:

with:

case TOGGLE_COLUMN:
case RESET_COLUMNS:

It should just work, because the column state is being changed in both cases.

Comment 24

a year ago
Comment on attachment 8856206 [details] [diff] [review]
fix-1344523-1.patch

Review of attachment 8856206 [details] [diff] [review]:
-----------------------------------------------------------------

So sorting doesn't work as expected.
103.**.**.**::**
should go *after*
87.**.**.**::**

when sorting in ascending order. Feel free to fix this in a follow up bug.
Attachment #8856206 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 26

a year ago
Hey Tim,
- Would we also get IPV6 address in `remoteAddress` ? Fx supports it? - I need to take care of it in the sort-predicates.js
- Regarding the `TOGGLE_COLUMN`.. you wanted something like the following ?
```
      case TOGGLE_COLUMN:
      case RESET_COLUMNS:
        Prefs.hiddenColumns = [...store.getState().ui.columns]
          .filter(([column, shown]) => !shown)
          .map(([column, shown]) => column);
        Prefs.hiddenColumns = ["remoteip"];
        break;
```
Flags: needinfo?(ntim.bugs)

Comment 27

a year ago
(In reply to Ruturaj Vartak from comment #26)
> Hey Tim,
> - Would we also get IPV6 address in `remoteAddress` ? Fx supports it? - I
> need to take care of it in the sort-predicates.js
I think it already works with ipv6, not sure, you can test with https://ipv6.google.com/

> - Regarding the `TOGGLE_COLUMN`.. you wanted something like the following ?
> ```
>       case TOGGLE_COLUMN:
>       case RESET_COLUMNS:
>         Prefs.hiddenColumns = [...store.getState().ui.columns]
>           .filter(([column, shown]) => !shown)
>           .map(([column, shown]) => column);
>         Prefs.hiddenColumns = ["remoteip"];
>         break;
> ```

No, this is what I'm suggesting:
```
       case TOGGLE_COLUMN:
       case RESET_COLUMNS:
         Prefs.hiddenColumns = [...store.getState().ui.columns]
           .filter(([column, shown]) => !shown)
           .map(([column, shown]) => column);
         break;
 ```
Flags: needinfo?(ntim.bugs)

Comment 28

a year ago
bug 1041895 has landed (it's on master), you might want to add the operator yourself.

Comment 29

a year ago
Something like:

remote-ip:**.**.**.** to search by IP.
(Assignee)

Comment 30

a year ago
(In reply to Tim Nguyen :ntim from comment #29)
> Something like:
> 
> remote-ip:**.**.**.** to search by IP.

aah !! nice.. let me try that.

I forgot to answer your question about try, no I don't have access to try. I think try needs hg / or git-cinnabar. Last time I tried installing cinnabar, I lost half of the day and I gave up :P

Comment 31

a year ago
(In reply to Ruturaj Vartak from comment #30)
> (In reply to Tim Nguyen :ntim from comment #29)
> > Something like:
> > 
> > remote-ip:**.**.**.** to search by IP.
> 
> aah !! nice.. let me try that.
To be clear, I meant that you can add the operator yourself (it's not implemented yet for IP):

In devtools/client/netmonitor/src/constants.js, register your column with canFilter: true (and optionally a filter key, if you don't register a filter key, it will use the column name).

Then in devtools/client/netmonitor/src/utils/filter-text-utils.js, add a new case statement for your new column.
 
> I forgot to answer your question about try, no I don't have access to try. I
> think try needs hg / or git-cinnabar. Last time I tried installing cinnabar,
> I lost half of the day and I gave up :P
It's worth it if you get the chance to do it (ask in #developers if you have trouble doing this) since you can use mozreview as well.
(Assignee)

Comment 32

a year ago
Created attachment 8856977 [details] [diff] [review]
fix-1344523-2.patch

- Implemented after merging latest master
- All tests are passing
- however, Sort of RemoteIP isn't working as expected. I wanted another eye to go over it.
Attachment #8856206 - Attachment is obsolete: true
Attachment #8856977 - Flags: review?(ntim.bugs)

Comment 33

a year ago
Comment on attachment 8856977 [details] [diff] [review]
fix-1344523-2.patch

Review of attachment 8856977 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +125,5 @@
>      case "domain":
>        match = item.urlDetails.host.toLowerCase().includes(value);
>        break;
> +    case "remoteip":
> +      match = (item.remoteAddress + item.remotePort + "").toLowerCase().includes(value);

shouldn't it be:

`${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value); ?

(you were missing ":")

::: devtools/client/netmonitor/src/utils/sort-predicates.js
@@ +28,4 @@
>    return first > second ? 1 : -1;
>  }
>  
> +function ip2long(ip) {

I would put this function after compareValues() in sort-predicates.js

also: ipToLong

@@ +29,5 @@
>  }
>  
> +function ip2long(ip) {
> +  if (!ip) {
> +    return 0;

I would put -1, so 0.0.0.0 appears after no IP.

@@ +33,5 @@
> +    return 0;
> +  }
> +  let base;
> +  let octets = ip.split(".");
> +  if (octets.length === 4) {

Add comment indicator it's IPv4.

@@ +39,5 @@
> +  } else {
> +    // Probably IPv6
> +    octets = ip.split(":");
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::

I'm guessing this comment applies to the greater or equal to 6

In this case, I prefer doing:

octets = ip.replace(/\:+/g, ":").split(":");
if (octets.length === 6)

@@ +41,5 @@
> +    octets = ip.split(":");
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::
> +      base = 16;
> +    } else {

else {
  // Invalid IP

@@ +42,5 @@
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::
> +      base = 16;
> +    } else {
> +      return 0;

Same comment here. (-1)

@@ +83,5 @@
>  }
>  
> +function remoteip(first, second) {
> +  const firstIP = ip2long(first.remoteAddress);
> +  const secondIP = ip2long(second.rempteAddress);

typo: remoteAddress not rempteAdress

(might be why sorting wasn't working well)
Attachment #8856977 - Flags: review?(ntim.bugs) → feedback+
(Assignee)

Comment 34

a year ago
Created attachment 8857015 [details] [diff] [review]
fix-1344523-3.patch

- the remoteAddress typo was silly, yes, things worked post that
- returning -1 for invalid IP
Attachment #8856977 - Attachment is obsolete: true
Attachment #8857015 - Flags: review?(ntim.bugs)

Comment 35

a year ago
Comment on attachment 8857015 [details] [diff] [review]
fix-1344523-3.patch

Review of attachment 8857015 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: devtools/client/netmonitor/src/utils/sort-predicates.js
@@ +39,5 @@
> +    base = 10;
> +  } else {
> +    // Probably IPv6
> +    octets = ip.replace(/\:+/g, ":").split(":");
> +    if (octets.length >= 6) {

This condition is incorrect.

"::1" is a totally valid IPv6 address (it means " 0000:0000:0000:0000:0000:0000:0000:0001")

I think the condition can be omitted and the else above can be changed to:

else if (ip.includes(":")) {

@@ +40,5 @@
> +  } else {
> +    // Probably IPv6
> +    octets = ip.replace(/\:+/g, ":").split(":");
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::

I read up about IPv6, and the algorithm is actually a bit more complicated:

- :: represents series of zeros,
- leading zeros can be omitted. (though we don't need to handle this for this function)

So the right algorithm would be:

```
// Process IPv6 short notation
// See https://en.wikipedia.org/wiki/IPv6#Address_representation
let numberOfZeroSections = 8 - ip.replace(/^:+|:+$/g, "").split(/:+/g).length;
octets = ip
  .replace("::", `:${"0:".repeat(numberOfZeroSections)}`)
  .replace(/^:|:$/g, "")
  .split(":");

```

I apologize for not reading this earlier :)
Attachment #8857015 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 36

a year ago
Created attachment 8857333 [details] [diff] [review]
fix-1344523-4.patch

- used your IPv6 validation, thanks for that!!
- moved ipToLong function to request-utils.js; I think you meant that in one of your comments
Attachment #8857015 - Attachment is obsolete: true
Attachment #8857333 - Flags: review?(ntim.bugs)

Comment 37

a year ago
Comment on attachment 8857333 [details] [diff] [review]
fix-1344523-4.patch

Review of attachment 8857333 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome

::: package.json
@@ +1,2 @@
>  {
> +  "name": "mozilla-eslint-setup",

unrelated change.
Attachment #8857333 - Flags: review?(ntim.bugs) → review+

Comment 38

a year ago
Created attachment 8857366 [details] [diff] [review]
fix-1344523-5.patch (Patch for landing)

I've fixed a bug related to column showing.
I've also done some minor code style changes.

Comment 39

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8322b71c7df2
Add IP Address in Network Monitor. r=ntim.
(Assignee)

Comment 40

a year ago
Thanks a lot Tim. Could you please explain what was the change in `netmonitor/src/utils/create-store.js` ?

Comment 41

a year ago
(In reply to Ruturaj Vartak from comment #40)
> Thanks a lot Tim. Could you please explain what was the change in
> `netmonitor/src/utils/create-store.js` ?

There was a bug where if you enabled the Remote IP column then closed the toolbox and restarted the toolbox, the Remote IP column disappeared (the column state should persist across devtools sessions).

Comment 42

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8322b71c7df2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

a year ago
Blocks: 1356868
(Assignee)

Comment 44

a year ago
Hey Sebastian,

Thanks, Remote IP is disabled by default, users have to right click on Column header and then select the columns. If we could mention that somewhere as well.
Thank you for the note, Ruturaj! I have further clarified the description for how to toggle the hidden columns and added a note to each initially hidden column.

Sebastian

Comment 46

a year ago
I have reproduced this bug with Nightly 54.0a1 (2017-03-04) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build ID 	20170503030212
User Agent 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170503]
You need to log in before you can comment on or make changes to this bug.