859044 - Add more information about request timings in the details pane

Status: NEW

(DevTools :: Netmonitor, enhancement, P3)

Description

[Victor Porof [:vporof][:vp]]

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

Comment 1

[Victor Porof [:vporof][:vp]]

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 2

[Vangelis Katsikaros]

>- 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?)

Comment 3

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

Attachment: 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

Comment 4

[Vangelis Katsikaros]

Attachment: Bug 859044 - Add more information about request timings in the details pane.

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

Comment 5

[Vangelis Katsikaros]

Attachment: timings_start_at.png

Comment 6

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

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

Comment 7

[Vangelis Katsikaros]

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

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

Comment 8

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

Looks good thanks!

Honza

Comment 9

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

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

Comment 10

[Vangelis Katsikaros]

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.

Comment 11

[Vangelis Katsikaros]

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

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

Comment 12

[Vangelis Katsikaros]

Attachment: request_timings_details.png

Comment 13

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

(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 14

[Vangelis Katsikaros]

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

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

Comment 15

[Vangelis Katsikaros]

Attachment: 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

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

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.

Comment 17

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

(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

Comment 18

[Vangelis Katsikaros]

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)

Comment 19

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

(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

[Tim Nguyen :ntim]

Vangelis, are you planning to finish this ? You're quite close from landing :)

Comment 21

[Vangelis Katsikaros]

Tim, Honza I apologize for the terrible delay. I am rebasing the branch and will take care of the comments.

Comment 22

[Vangelis Katsikaros]

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

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

Comment 23

[Vangelis Katsikaros]

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 24

[Vangelis Katsikaros]

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

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

Comment 25

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

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.

Comment 26

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

(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

Comment 27

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

Attachment: timings.png

See the DOMContentLoaded and load events

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

Honza

Comment 28

[Vangelis Katsikaros]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143616/diff/6-7/

Comment 29

[Vangelis Katsikaros]

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?

Comment 30

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

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

Comment 31

[Vangelis Katsikaros]

Thanks, Honza. I can replicate the issue with the STR. I'll further look into it

Comment 32

[Vangelis Katsikaros]

Unassigned myself from the bug as I couldn't allocate time to work on it :( All work so far is pushed in hg, however it's not rebased.

Comment 33

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

The bug is inactive, moving to the backlog (P3)
Honza