Output 'load' and 'DOMContentLoaded' event to HAR

RESOLVED FIXED in Firefox 60

Status

enhancement
P2
normal
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: sebo, Assigned: Honza)

Tracking

(Blocks 1 bug)

unspecified
Firefox 60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
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
Flags: needinfo?(vporof)
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.
Flags: needinfo?(vporof)

Comment 3

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

Comment 4

3 years ago
Also waiting to see real values in the 'onContentLoad' and 'onLoad' timings field.
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!
Flags: needinfo?(odvarko)
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
Clear NI
Flags: needinfo?(odvarko)
Assignee: nobody → dd.mozilla
Priority: -- → P2
(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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko) → needinfo?(poirot.alex)
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
(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.
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
Flags: needinfo?(poirot.alex)
Posted patch bug_1174091_v1.patch (obsolete) — Splinter Review
Attachment #8788502 - Attachment is obsolete: true
Attachment #8800867 - Flags: review?(poirot.alex)
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?
Attachment #8800867 - Flags: review?(poirot.alex) → review?(odvarko)
(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.
(Reporter)

Comment 17

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

Sebastian
Flags: needinfo?(dd.mozilla)
(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.
Flags: needinfo?(dd.mozilla)
(Reporter)

Comment 19

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

Sebastian
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
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
Assignee: dd.mozilla → odvarko
Status: NEW → ASSIGNED
Depends on: 1432803
Attachment #8800867 - Attachment is obsolete: true
Attachment #8800867 - Flags: review?(odvarko)

Comment 23

a year ago
mozreview-review
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.
Attachment #8947076 - Flags: review?(poirot.alex)
(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 hidden (mozreview-request)

Comment 26

a year ago
mozreview-review
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.
Attachment #8947076 - Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request)

Comment 29

a year ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f58798142e33
Output 'load' and 'DOMContentLoaded' event to HAR; r=ochameau

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f58798142e33
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.