1431912 - Response time and Latency are always shown to be 0min when waterfall is disabled

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Tim Nguyen :ntim]

STR:
- Disable "Timeline" column from netmonitor context menu
- Enable "Response Time" and "Latency" columns
- Reload the page

Latency and Response Time are always 0 minutes. The actual values are only shown when clicking on the network request.

Probably a regression of lazy loading timings: bug 1425273.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

We still can take a patch for this during beta 59. Jan, do you know who might work on it?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report Tim!

> Latency and Response Time are always 0 minutes. The actual values are only
> shown when clicking on the network request.
On my machine:
The actual values are fetched only if the request is clicked
and the "Timings" panel selected.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #1)
> We still can take a patch for this during beta 59. Jan, do you know who
> might work on it?
I'll try to analyze this ASAP.

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1431912 - Properly populate timing columns;

Review commit: https://reviewboard.mozilla.org/r/214816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214816/

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

https://reviewboard.mozilla.org/r/214816/#review220462

::: devtools/client/netmonitor/src/components/RequestListColumnResponseTime.js:17
(Diff revision 1)
> + * This component represents a column displaying selected
> + * timing value. There are following possible values this
> + * column can render:
> + * - Start Time
> + * - End Time
> + * - Response Time
> + * - Duration
> + * - Latency
> + */

This isn't true, Start Time, End Time, Response Time, Duration and Latency all have a different component.

(I'd totally be fine with unifying them in one component though)

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214816/diff/1-2/

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Tim Nguyen :ntim from comment #4)
> This isn't true, Start Time, End Time, Response Time, Duration and Latency
> all have a different component.
Ah, yes, good catch.

> (I'd totally be fine with unifying them in one component though)
Exactly, done.

Try push looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9baa8d3ed35c2ca11506f14e22b0942e52bb4e62

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1431912 - Test for timing columns;

Review commit: https://reviewboard.mozilla.org/r/215214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215214/

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214816/diff/2-3/

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

https://reviewboard.mozilla.org/r/214816/#review220790

Thanks for the fix, looks good.

::: devtools/client/netmonitor/src/components/RequestListColumnTime.js:14
(Diff revision 3)
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +const { getFormattedTime } = require("../utils/format-utils");
> +const { fetchNetworkUpdatePacket } = require("../utils/request-utils");
> +const { getResponseTime } = require("../utils/request-utils");
> +const { getStartTime } = require("../utils/request-utils");
> +const { getEndTime } = require("../utils/request-utils");

You could merge all these lines into:
const { fetchNetworkUpdatePacket, getResponseTime, getStartTime, getEndTime } = require("../utils/request-utils");

::: devtools/client/netmonitor/src/components/RequestListItem.js:34
(Diff revision 3)
> -  RequestListColumnStartTime,
>    RequestListColumnStatus,
>    RequestListColumnTransferredSize,
>    RequestListColumnType,
>    RequestListColumnWaterfall
>  */

Shouldn't you mention "RequestListColumnTime" in this list?

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

https://reviewboard.mozilla.org/r/215214/#review220800

::: devtools/client/netmonitor/test/browser_net_columns_time.js:44
(Diff revision 1)
> +
> +  let promises = [];
> +  types.forEach(type => {
> +    promises.push(waitUntil(() => {
> +      let node = item.querySelector(".requests-list-" + type + "-time");
> +      let value = parseInt(node.textContent, 10);

This is a bit surprising but it seems to work.
node.textContent will be the formated time here, so something like 100ms or 1.3s.
parseInt seems to ignore characters,
parseInt("100ms") returns 100...

::: devtools/client/netmonitor/test/browser_net_columns_time.js:49
(Diff revision 1)
> +      let value = parseInt(node.textContent, 10);
> +      return value > 0;
> +    }));
> +  });
> +
> +  await Promise.all(promises);

I think you do not need a promise list here.
You could do:
for (let type of types) {
  await waitUntil(() => {
    ...
  });
}

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

https://reviewboard.mozilla.org/r/215214/#review220804

::: devtools/client/netmonitor/test/head.js:709
(Diff revision 1)
>         "Headers for columns number " + i + " are aligned."
>      );
>    }
>  }
>  
> +function* hideColumn(monitor, column) {

It would be nice if you can change hideColumn and showColumn to async functions.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214816/diff/3-4/

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215214/diff/1-2/

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214816/diff/4-5/

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215214/diff/2-3/

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #9 and #10)
All fixed

(In reply to Tim Nguyen :ntim from comment #11)
> It would be nice if you can change hideColumn and showColumn to async
> functions.
I reported bug 1432865 for this.

Honza

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b0e37f74f15
Properly populate timing columns; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c1c7615bd593
Test for timing columns; r=ochameau

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0b0e37f74f15
https://hg.mozilla.org/mozilla-central/rev/c1c7615bd593

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1425273
[User impact if declined]: Some columns in the Network panel (DevTools) are not populated with data.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: both patches in this bug
[Is the change risky?]: low risk, only developers
[Why is the change risky/not risky?]: only developers
[String changes made/needed]: n/a

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1425273
[User impact if declined]: Some columns in the Network panel (DevTools) are not populated with data.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: both patches in this bug
[Is the change risky?]: low risk, only developers
[Why is the change risky/not risky?]: only developers
[String changes made/needed]: n/a

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8944663 [details]
Bug 1431912 - Properly populate timing columns;

Recent regression (in 59), has test coverage, let's uplift this for beta 6.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

Thanks for the extra tests.

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/175697dceadf
https://hg.mozilla.org/releases/mozilla-beta/rev/9be143123781