Add more information about request timings in the details pane

ASSIGNED

Status

()

Firefox
Developer Tools: Netmonitor
P2
normal
ASSIGNED
5 years ago
12 days ago

People

(Reporter: vporof, Assigned: Vangelis Katsikaros)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
More details that can/should be shown:
- Request start time since the beginning
- Event timing relative to the request start (vs. DOMContentLoaded and onload)
(Reporter)

Comment 1

5 years ago
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Component: Developer Tools → Developer Tools: Netmonitor
Summary: [netmonitor] Add more information about request timings in the details pane → Add more information about request timings in the details pane
(Reporter)

Updated

5 years ago
Priority: -- → P3
(Reporter)

Updated

5 years ago
Priority: P3 → P2
(Assignee)

Comment 2

6 months ago
>- Request start time since the beginning
>- Event timing relative to the request start (vs. DOMContentLoaded and onload)

If I understand the request correctly:

The 1st one is now available in the new columns added in https://bugzilla.mozilla.org/show_bug.cgi?id=1356871 , although it could be argued that this info is a bit buried in there.

The 2nd one is now available in the details panel -> Timings.

I would like to ask if there is still interest of adding the 1st one somewhere in the details pane (guessing in the Timings tab?)
Flags: needinfo?(odvarko)
Created attachment 8871704 [details]
timings.png

(In reply to Vangelis Katsikaros from comment #2)
> >- Request start time since the beginning
> >- Event timing relative to the request start (vs. DOMContentLoaded and onload)
> 
> If I understand the request correctly:
> 
> The 1st one is now available in the new columns added in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1356871 , although it could be
> argued that this info is a bit buried in there.
> 
> The 2nd one is now available in the details panel -> Timings.
> 
> I would like to ask if there is still interest of adding the 1st one
> somewhere in the details pane (guessing in the Timings tab?)

Yes, this report is still valid and it make sense to have the timing in the Timings panel. I am attaching a screenshot from Chrome DevTools (see: "Started at" time) for inspiration.

Honza
Flags: needinfo?(odvarko)
(Assignee)

Updated

6 months ago
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 5

6 months ago
Created attachment 8872127 [details]
timings_start_at.png

Comment 6

6 months ago
mozreview-review
Comment on attachment 8872126 [details]
Bug 859044 - Add more information about request timings in the details pane.

https://reviewboard.mozilla.org/r/143616/#review147602

Looks good to me, just one inline comment.

R+ assuming Try is green.

Thanks for working on this!


Honza

::: devtools/client/locales/en-US/netmonitor.properties:730
(Diff revision 1)
>  netmonitor.timings.receive=Receiving:
>  
> +# LOCALIZATION NOTE (netmonitor.timings.receive): This is the label displayed
> +# in the network details timings tab identifying when the current request started
> +# compared to the first request
> +netmonitor.timings.startedAt=Started at:

> LOCALIZATION NOTE (netmonitor.timings.receive)

The localization note needs to refer to the right string ID, i.e.: netmonitor.timings.startedAt
Attachment #8872126 - Flags: review?(odvarko) → review+
(Assignee)

Updated

6 months ago
Keywords: dev-doc-needed
Comment hidden (mozreview-request)
Looks good thanks!

Honza
Btw. the comment #0 mentions also event timing relative to the request start
(vs. DOMContentLoaded and onload)

Do you want to make another patch for it or file a follow up?

Honza
Flags: needinfo?(vkatsikaros)
(Assignee)

Comment 10

6 months ago
Honza I apologize for missing the "vs. DOMContentLoaded and onload" part. I'll see if it's easy to add and if so I'll add send an updated patch.
Flags: needinfo?(vkatsikaros)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

6 months ago
Created attachment 8874143 [details]
request_timings_details.png
Attachment #8872127 - Attachment is obsolete: true
(In reply to Vangelis Katsikaros from comment #12)
> Created attachment 8874143 [details]
> request_timings_details.png

Thanks for the screenshot/mockup, looks good. Just one thing, can you create a little space between the new Timings (Started, DOMContentLoaded and load events) and the rest of the original timings. Just to visually separate these things. The same might be done for the [Learn More] link.

Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 15

6 months ago
Created attachment 8874511 [details]
started_at_vertical_space.png

Honza, I added the vertical spacing. However, CSS is not my strong point so I am not sure if this could be done in an easier/less bloated way or if follows the proper CSS conventions.

Comment 16

6 months ago
mozreview-review
Comment on attachment 8872126 [details]
Bug 859044 - Add more information about request timings in the details pane.

https://reviewboard.mozilla.org/r/143616/#review150012

::: devtools/client/netmonitor/src/components/timings-panel.js:116
(Diff revision 4)
>  TimingsPanel.displayName = "TimingsPanel";
>  
>  TimingsPanel.propTypes = {
> +  firstRequestStartedMillis: PropTypes.number.isRequired,
>    request: PropTypes.object.isRequired,
> +  timingMarkers: PropTypes.object.isRequired,

Please create an explanation comment for every prop.
(In reply to Vangelis Katsikaros from comment #15)
> Honza, I added the vertical spacing. However, CSS is not my strong point so
> I am not sure if this could be done in an easier/less bloated way or if
> follows the proper CSS conventions.

Looks good to me, thanks!
Honza
(Assignee)

Comment 18

5 months ago
mozreview-review-reply
Comment on attachment 8872126 [details]
Bug 859044 - Add more information about request timings in the details pane.

https://reviewboard.mozilla.org/r/143616/#review150012

> Please create an explanation comment for every prop.

Thanks for the review, I'd like to ask a clarification. Should the comment be about why the prop is needed for this React element or what the prop describes? 

I am a bit concerned: the first option could easily bit rot if the prop is used for something else in the future. The second option looks a bit out of place and perhaps this should be documented in the place originally introduced (http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/devtools/client/netmonitor/src/reducers/requests.js#67)
(In reply to Vangelis Katsikaros from comment #18)
> Comment on attachment 8872126 [details]
> Bug 859044 - Add more information about request timings in the details pane.
No big deal, I was thinking about a comment that help people
to understand the component/props a bit better/faster.

In any case, this should not block the patch from landing.

Thanks!

Honza

Comment 20

4 months ago
Vangelis, are you planning to finish this ? You're quite close from landing :)
Flags: needinfo?(vkatsikaros)
(Assignee)

Comment 21

2 months ago
Tim, Honza I apologize for the terrible delay. I am rebasing the branch and will take care of the comments.
Flags: needinfo?(vkatsikaros)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 months ago
mozreview-review
Comment on attachment 8872126 [details]
Bug 859044 - Add more information about request timings in the details pane.

https://reviewboard.mozilla.org/r/143616/#review184854

::: devtools/client/netmonitor/src/components/tabbox-panel.js:134
(Diff revision 5)
> +      load: getDisplayedTimingMarker(state, "firstDocumentLoadTimestamp"),
> +    }
> +  }),
> +  (dispatch) => ({
> +  }),
> +)(TabboxPanel);

I see this was removed in https://hg.mozilla.org/mozilla-unified/rev/fb65bb381a43 so I wonder if there is a better way to handle this now.
Comment hidden (mozreview-request)

Comment 25

2 months ago
mozreview-review
Comment on attachment 8872126 [details]
Bug 859044 - Add more information about request timings in the details pane.

https://reviewboard.mozilla.org/r/143616/#review185032

Thanks for the update!

I am seeing weird values for DOMContentLoaded and load (I'll attach a screenshot)

Honza

::: devtools/client/netmonitor/src/components/tabbox-panel.js:133
(Diff revision 6)
> +      getDisplayedTimingMarker(state, "firstDocumentDOMContentLoadedTimestamp"),
> +      load: getDisplayedTimingMarker(state, "firstDocumentLoadTimestamp"),
> +    }
> +  }),
> +  (dispatch) => ({
> +  }),

You can remove this empty callback. The connect() method doesn't have to have the second argument.
Attachment #8872126 - Flags: review+ → review-
(In reply to Vangelis Katsikaros from comment #23)
> I see this was removed in
> https://hg.mozilla.org/mozilla-unified/rev/fb65bb381a43 so I wonder if there
> is a better way to handle this now.
It's been removed since we didn't need it. It's fine to have it back.
(and I see that you have it in the patch)

Honza
Created attachment 8908127 [details]
timings.png

See the DOMContentLoaded and load events

Also, DOMContentLoaded label and its value should be at the same line (no wrap).

Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 29

14 days ago
Honza, a delayed rebase and fix according to your comments.

I tried several times, with different web pages (homepages of bugzilla, wikipedia, google), with/without cache but I couldn't replicate the issue where you get a 10 digit time in ms. Could you describe the STR, so that I take a look?

Updated

14 days ago
Flags: needinfo?(odvarko)

Comment 30

13 days ago
mozreview-review
Comment on attachment 8872126 [details]
Bug 859044 - Add more information about request timings in the details pane.

https://reviewboard.mozilla.org/r/143616/#review201906

I am still seeing an invalid value for DOMContentLoaded and load events.

Here is my STR:
1) Load google.com
2) Open the Toolbox and switch into the Net panel
3) Select the first HTTP request in the panel
4) Switch into the Timings side-panel
5) Check 'Persist Logs' and reload the page (the side panel should stay open)
6) The value of DOMContentLoad and loaded events is set to huge number for a little moment. It's replaced by valid value when the page finishes loading.

Can you repro this on your machine?

Honza
Attachment #8872126 - Flags: review?(odvarko) → review-
Flags: needinfo?(odvarko)
Flags: needinfo?(vkatsikaros)
(Assignee)

Comment 31

12 days ago
Thanks, Honza. I can replicate the issue with the STR. I'll further look into it
Flags: needinfo?(vkatsikaros)
You need to log in before you can comment on or make changes to this bug.