Closed Bug 1431912 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 + fixed
firefox60 --- fixed

People

(Reporter: ntim, Assigned: Honza)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1431912 - Properly populate timing columns;
59 bytes, text/x-review-board-request
ochameau
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1431912 - Test for timing columns;
59 bytes, text/x-review-board-request
ochameau
: review+
lizzard
: approval-mozilla-beta+
Details 
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.
Version: unspecified → Trunk
We still can take a patch for this during beta 59. Jan, do you know who might work on it?
tracking-firefox59: ? → +
Flags: needinfo?(odvarko)
Flags: needinfo?(odvarko)
Priority: -- → P2
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
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: P2 → P1
Has STR: --- → yes
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)
(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
status-firefox60: ? → affected
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?
Attachment #8944663 - Flags: review?(poirot.alex) → review+
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(() => {
    ...
  });
}
Attachment #8945019 - Flags: review?(poirot.alex) → review+
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.
(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
Pushed by jodvarko@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/0b0e37f74f15
https://hg.mozilla.org/mozilla-central/rev/c1c7615bd593
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Please request Beta approval on this when you get a chance.
status-firefox-esr52: --- → unaffected
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
Attachment #8944663 - Flags: approval-mozilla-beta?
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
Attachment #8945019 - Flags: approval-mozilla-beta?
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.
Attachment #8944663 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8945019 [details]
Bug 1431912 - Test for timing columns;

Thanks for the extra tests.
Attachment #8945019 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: