1174091 - Output 'load' and 'DOMContentLoaded' event to HAR

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Sebastian Zartner [:sebo]]

Copying or saving the network requests as HAR currently sets the 'onContentLoad' and 'onLoad' timings to -1, meaning they are not available.

These properties should get the correct timings of the 'load' and 'DOMContentLoaded' events set.

Sebastian

Comment 1

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

Victor, I'd like to fix this issue (since you've just fixed bug 859042).

The basic support for HAR export is in devtools/client/netmonitor/har
Manual HAR export is using data already collected in the Net panel and automated HAR export (net panel not needed) is based on 'networkEvent' and 'networkEventUpdate' (fired by debuggerClient).

How can I get the timing for 'load' and 'DOMContentLoad' events?

Honza

Comment 2

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

You'll have to go through the timeline actor. Might want to wait for bug 1218078 to land to get a proper example, but I can give you an overview on IRC if this is urgent.

Comment 3

[Arjun Talwar]

Above 2 bugs seem to be fixed already. Can somebody take this bug up ... it would be of great help.

Comment 4

[grmoktan]

Also waiting to see real values in the 'onContentLoad' and 'onLoad' timings field.

Comment 5

[Dragana Damjanovic [:dragana]]

I want to make a patch for this bug, but I need help.
I thought that devtools/client/netmonitor/har/har-collector.js is the right place, but it looks like we do not call it at all. 
I would appreciate a short explanation, where to implement this :)
Thanks!

Comment 6

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

There is a patch attached to bug 1218078 (also mentioned in comment #2) that introduces new event sent from the back-end: "doc-loading" [1]

This event sends timing related to both: `DOMContentLoaded` and `Load` events and it should be reused by HARCollector.

See how a listener for the event is created in netmonitor-controller.js => see `_onDocLoadingMarker` method representing 'doc-loading' event handler [2]

HARCollector is very similar to netmonitor-controller in terms of registering net listeners and listening to net-events (they are both collecting HTTP data from the backend). The job here is to look how netmonitor-controller is collecting the 'doc-loading' timings and do the same in HARCollector.

Honza

[1] https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/devtools/server/performance/timeline.js#146

[2] https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/devtools/client/netmonitor/netmonitor-controller.js#569

Comment 7

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

Clear NI

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1174091_draft.patch

Comment 9

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #8)
> Created attachment 8788502 [details] [diff] [review]
> bug_1174091_draft.patch

Hi Jan, 
sorry for a  delay, I was busy with other things.
I do not know devtools at all, so I will need help with this. The patch does not work properly.
I am calling isRecording() just to see if the server is recording, which it does because it is started by netmonitor. The problem is that response to isRecording is going to a wrong place and I get error:
Message: Error: Unexpected packet server1.conn0.child1/timelineActor19, {"value":true,"from":"server1.conn0.child1/timelineActor19"}

I do not know how this work, why the response is sent to a wrong place. Please give me some hints before I start digging into devtool conns :) Thanks.

Comment 10

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

(In reply to Dragana Damjanovic [:dragana] from comment #9)
> (In reply to Dragana Damjanovic [:dragana] from comment #8)
> > Created attachment 8788502 [details] [diff] [review]
> > bug_1174091_draft.patch
> 
> Hi Jan, 
> sorry for a  delay, I was busy with other things.
> I do not know devtools at all, so I will need help with this. The patch does
> not work properly.
> I am calling isRecording() just to see if the server is recording, which it
> does because it is started by netmonitor. The problem is that response to
> isRecording is going to a wrong place and I get error:
> Message: Error: Unexpected packet server1.conn0.child1/timelineActor19,
> {"value":true,"from":"server1.conn0.child1/timelineActor19"}
> 
> I do not know how this work, why the response is sent to a wrong place.
> Please give me some hints before I start digging into devtool conns :)
> Thanks.

I tried manual HAR export and I am seeing some timing for 'onLoad' and 'onContentLoad'.

But the way you are trying to use the 'TimelineFront' seems to be wrong to me.

> this.timelineFront = new TimelineFront(this.debuggerClient, this.form);
> var a = yield this.timelineFront.isRecording();
> dump("DDDD START " + a + "\n");
> this.timelineFront.on("doc-loading", this.onDocLoadingMarker);

I think you need to do something like as follows:

let connectTimeline = () => {
  this.timelineFront = new TimelineFront(this.debuggerClient, this.form);
  return this.timelineFront.start({ withDocLoadingEvents: true });
};

yield connectTimeline();
this.timelineFront.on("doc-loading", this.onDocLoadingMarker);

I tried that, but surprisingly I am also getting the exception:

JSM error: Error: Unexpected packet server1.conn1.child1/timelineActor19, {"value":377805.3849208206,"from":"server1.conn1.child1/timelineActor19"}

@Alex, how the Timeline Actor should be properly reused?

Honza

Comment 11

[Alexandre Poirot [:ochameau]]

Hum. I applied the patch to a recent mozilla-central and it works fine for me without any error message?
By correct, I mean that I finally see onLoad, onContentLoad to something else than -1.

But I'm wondering if I'm testing it correctly, as I don't see this dump call:
  dump("DDDD START " + a + "\n");

Here is my str:
- open a webpage
- open the netmon
- select the html page request
- click on "Copy all as HAR" context menu item

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> Hum. I applied the patch to a recent mozilla-central and it works fine for
> me without any error message?
> By correct, I mean that I finally see onLoad, onContentLoad to something
> else than -1.
> 
> But I'm wondering if I'm testing it correctly, as I don't see this dump call:
>   dump("DDDD START " + a + "\n");
> 
> Here is my str:
> - open a webpage
> - open the netmon
> - select the html page request
> - click on "Copy all as HAR" context menu item

Copy all to HAR works but automatic HAR collection doesn't.

Comment 13

[Alexandre Poirot [:ochameau]]

Ah ok, that's quite a hidden feature. I was able to reproduce.
I imagine that's because you instanciate two fronts for the same actor.
You would need to memoize the front to be unique per client, like what we do for some other actors.

You would have to copy code like this:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/fronts/device.js#40-54
To: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/fronts/timeline.js
And then refactor: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-controller.js#207
to call the same memoization helper you are going to call from har-collector.js

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_1174091_v1.patch

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8800867 [details] [diff] [review]
bug_1174091_v1.patch

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

The actor/front work looks good, but I don't know the network monitor code much and even less the har component.

::: devtools/client/netmonitor/har/har-collector.js
@@ +48,5 @@
>      this.debuggerClient.addListener("networkEvent", this.onNetworkEvent);
>      this.debuggerClient.addListener("networkEventUpdate",
>        this.onNetworkEventUpdate);
> +
> +    this.timelineFront = getTimelineFront(this.debuggerClient, this.form, false);

Why wouldn't you create the actor here? Does that mess up with the network monitor if you initialize the actor before the tools are opened?

Comment 16

[Dragana Damjanovic [:dragana]]

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> Comment on attachment 8800867 [details] [diff] [review]
> bug_1174091_v1.patch
> 
> Review of attachment 8800867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The actor/front work looks good, but I don't know the network monitor code
> much and even less the har component.
> 
> ::: devtools/client/netmonitor/har/har-collector.js
> @@ +48,5 @@
> >      this.debuggerClient.addListener("networkEvent", this.onNetworkEvent);
> >      this.debuggerClient.addListener("networkEventUpdate",
> >        this.onNetworkEventUpdate);
> > +
> > +    this.timelineFront = getTimelineFront(this.debuggerClient, this.form, false);
> 
> Why wouldn't you create the actor here? Does that mess up with the network
> monitor if you initialize the actor before the tools are opened?

Looking at the code and debugging it har-collector is only created after network monitor has been created. But maybe I am wrong, so please correct me.

Comment 17

[Sebastian Zartner [:sebo]]

Looks like the patch is lying around for quite some time. Dragana, do you still plan to finish it?

Sebastian

Comment 18

[Dragana Damjanovic [:dragana]]

(In reply to Sebastian Zartner [:sebo] from comment #17)
> Looks like the patch is lying around for quite some time. Dragana, do you
> still plan to finish it?
> 
> Sebastian

I was waiting for a review for this patch. This was long time ago, but from my comment 16 I thought at the time that it is not necessary to change anything.

I do not know dev-tools at all, if Honza get to review the patch I would finish it.

Comment 19

[Sebastian Zartner [:sebo]]

Thanks for the quick response. So, I'm forwarding the request to Honza.

Sebastian

Comment 20

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

Sorry for huuuge delay on this! It's still on my todo list. But it isn't forgotten!
(I am clearing NI, but keeping the review request on)

Honza

Comment 21

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

Dragana, I finally got time to work on HAR related issues and new features (like e.g. import HAR bug 1434333 and HAR WebExtension APIs Bug 1311171 and Bug 1311177). The underlying architecture of the Net panel changed a lot recently and there are also upcoming changes related to how the timing markers are fetched from the backend (Bug 1432803)

Since all the related WIP bug reports, I am taking over this bug, hope it's ok!

Honza

Comment 22

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

Attachment: Bug 1174091 - Output 'load' and 'DOMContentLoaded' event to HAR;

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

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 8947076 [details]
Bug 1174091 - Output 'load' and 'DOMContentLoaded' event to HAR;

https://reviewboard.mozilla.org/r/216874/#review222736

Could you contribute to one existing test in devtools/client/netmonitor/src/har/test/ to cover this new feature?
Is it intended that these two fields are not going to be set when using devtools.netmonitor.har.enableAutoExportToFile?

Otherwise, HAR save to file and copy to clipboard features are really slow.
It takes about 10seconds to complete against duckduckgo, which isn't a big website!
Were you aware about this? Is there a plan to address this?
Here is a profile of this:
  https://perfht.ml/2npGY8N
It looks like we update redux store and do a tons of react updates, without necessary reflowing.
I filled bug 1434848 to fix StatusBar. But we should probably fix other components like RequestListContent.
And bug 1434849 to track HAR on DAMP.

Finally, I'm bit lost in the various HAR classes, would you mind writting a very small README
in devtools/client/netmonitor/src/har/ to briefly explain the purpose of each class and especially highlight the two different codepaths:
* the one using netmonitor connector/getData/requestData and triggered from netmonitor
* the other one that is not reusing connector. I think that's the one related to devtools.netmonitor.har.enableAutoExportToFile pref, right?

::: devtools/client/netmonitor/src/selectors/timing-markers.js:8
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  function getDisplayedTimingMarker(state, marker) {
> -  return state.timingMarkers.marker - state.requests.firstStartedMillis;
> +  return state.timingMarkers[marker] - state.requests.firstStartedMillis;

Could you write a test covering this?
At least check that the status bar get a load event timing being displayed.

Comment 24

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

(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Could you contribute to one existing test in
> devtools/client/netmonitor/src/har/test/ to cover this new feature?
Done

> Is it intended that these two fields are not going to be set when using
> devtools.netmonitor.har.enableAutoExportToFile?
Yes, the HARAutomation is using different code path and it should either:
1) be refactoring and reuse the existing Firefox Connector
2) or be removed entirely and replaced by WebExtension API + HARTriggerExport
   extension

For now I am ignoring bugs in HarAutomation (there are more issues)

> Otherwise, HAR save to file and copy to clipboard features are really slow.
> It takes about 10seconds to complete against duckduckgo, which isn't a big
> website!
> Were you aware about this? Is there a plan to address this?
Yes, I know about this, thanks for filing bug 1434855 to track this!

> And bug 1434849 to track HAR on DAMP.
Nice

> Finally, I'm bit lost in the various HAR classes, would you mind writting a
> very small README
Done 

> in devtools/client/netmonitor/src/har/ to briefly explain the purpose of
> each class and especially highlight the two different codepaths:
> * the one using netmonitor connector/getData/requestData and triggered from
> netmonitor
> * the other one that is not reusing connector. I think that's the one
> related to devtools.netmonitor.har.enableAutoExportToFile pref, right?
Correct

> Could you write a test covering this?
> At least check that the status bar get a load event timing being displayed.
Done

Thanks,
Honza

Comment 25

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

Comment on attachment 8947076 [details]
Bug 1174091 - Output 'load' and 'DOMContentLoaded' event to HAR;

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

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8947076 [details]
Bug 1174091 - Output 'load' and 'DOMContentLoaded' event to HAR;

https://reviewboard.mozilla.org/r/216874/#review222958

Thanks a lot for the tests and the doc!

::: devtools/client/netmonitor/src/har/README.md:3
(Diff revision 2)
> +# HAR
> +HAR stands for `HTTP Archive` format used by various HTTP monitoring tools
> +to export collected data. This format is based on JSON and supported by

and +is+ supported

::: devtools/client/netmonitor/test/browser_net_status-bar.js:37
(Diff revision 2)
> +  // All expected labels should be there
> +  ok(requestCount, "There must be request count label");
> +  ok(size, "There must be size label");
> +  ok(onContentLoad, "There must be DOMContentLoaded label");
> +  ok(onLoad, "There must be load label");
> +  ok(onLoad, "There must be load label");

This line is a duplicate.

::: devtools/client/netmonitor/test/browser_net_status-bar.js:44
(Diff revision 2)
> +  // The content should not be empty
> +  ok(requestCount.textContent, "There must be request count label text");
> +  ok(size.textContent, "There must be size label text");
> +  ok(onContentLoad.textContent, "There must be DOMContentLoaded label text");
> +  ok(onLoad.textContent, "There must be load label text");
> +  ok(onLoad.textContent, "There must be load label text");

Same, this line is also a duplicate.

Comment 27

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

Comment on attachment 8947076 [details]
Bug 1174091 - Output 'load' and 'DOMContentLoaded' event to HAR;

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

Comment 28

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

Comments, resolved, thanks for the review Alex!

Try push green,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=884a304f84dc0f26b1954de958a59e6f5a096276

Honza

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f58798142e33
Output 'load' and 'DOMContentLoaded' event to HAR; r=ochameau

Comment 30

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f58798142e33