Closed
Bug 1174091
Opened 9 years ago
Closed 7 years ago
Output 'load' and 'DOMContentLoaded' event to HAR
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: sebo, Assigned: Honza)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 2 obsolete files)
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
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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•9 years ago
|
||
Above 2 bugs seem to be fixed already. Can somebody take this bug up ... it would be of great help.
Also waiting to see real values in the 'onContentLoad' and 'onLoad' timings field.
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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
Updated•8 years ago
|
Assignee: nobody → dd.mozilla
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
(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)
Assignee | ||
Comment 10•8 years ago
|
||
(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)
Comment 11•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
Attachment #8788502 -
Attachment is obsolete: true
Attachment #8800867 -
Flags: review?(poirot.alex)
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
(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•7 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)
Comment 18•7 years ago
|
||
(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•7 years ago
|
||
Thanks for the quick response. So, I'm forwarding the request to Honza. Sebastian
Flags: needinfo?(odvarko)
Assignee | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Blocks: netmonitor-har
Assignee | ||
Comment 21•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8800867 -
Attachment is obsolete: true
Attachment #8800867 -
Flags: review?(odvarko)
Comment 23•7 years 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)
Assignee | ||
Comment 24•7 years ago
|
||
(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•7 years 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) |
Assignee | ||
Comment 28•7 years ago
|
||
Comments, resolved, thanks for the review Alex! Try push green, https://treeherder.mozilla.org/#/jobs?repo=try&revision=884a304f84dc0f26b1954de958a59e6f5a096276 Honza
Comment 29•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f58798142e33
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•