Get critical chains: pure js version #310
Conversation
| @@ -46,6 +46,7 @@ def main(): | |||
| with open('clovis-trace.log') as f: | |||
There was a problem hiding this comment.
Given this is pure JS, do we not want to remove these python files?
There was a problem hiding this comment.
(see the initial PR comment :)
There was a problem hiding this comment.
Yeah okay... I'm not sure. Feels like we should keep it around in a separate branch on origin, but I don't want to merge it in unless we know we need it.
There was a problem hiding this comment.
I'm not 200% confident about how correct my approach is or whether we'll end up needing the clovis graph for something else. Once we're sure clovis is never happening I'll purge all python from lighthouse.
There was a problem hiding this comment.
oh I didn't see your other comment. The python files were already on master. Do you want me to remove them and put them on other branch? There is still use for them in the sense you can do lighthouse --save-artifacts and then use the artifacts through clovis to generate the dependency graph png picture. If you want I can send out a separate PR removing all python/clovis code.
|
Reviewed. |
| const log = require('../lib/log.js'); | ||
|
|
||
| const flatten = arr => arr.reduce((a, b) => a.concat(b), []); | ||
| const contains = (arr, elm) => arr.indexOf(elm) > -1; |
There was a problem hiding this comment.
change to includes to match [].includes (coming in node 6)
| }, | ||
|
|
||
| _onRequestStarted: function(event) { | ||
| var request = (event.data); |
| const flatten = arr => arr.reduce((a, b) => a.concat(b), []); | ||
| const includes = (arr, elm) => arr.indexOf(elm) > -1; | ||
|
|
||
| class Node { |
There was a problem hiding this comment.
maybe something slightly more descriptive, RequestNode or something?
There was a problem hiding this comment.
also a comment describing what children and parent are in this context would be helpful
There was a problem hiding this comment.
the request graph is actually a tree, so these are tree nodes with one parent and a bunch of children. I'll rename and add a comment.
There was a problem hiding this comment.
right, so to be clear that's the relationship the tree represents, something like InitiatorNode then?
There was a problem hiding this comment.
I like RequestNode better, because it holds data about a request. The initiator relationship is captured by the edges, so I'll add a comment about what the parent-child relationship represents.
There was a problem hiding this comment.
the leaves are not initiators of anything so it would be weird to call them InitiatorNode.
| return `{ | ||
| id: ${this.requestId}, | ||
| parent: ${!!(this.parent) && this.parent.requestId}, | ||
| children: ${this.children.map(child => child.requestId)} |
There was a problem hiding this comment.
does this need to be surrounded by [] to make sure it's valid JSON?
There was a problem hiding this comment.
You're right. I call JSON.stringify on the array now.
| } | ||
| return {CriticalNetworkChains: chains}; | ||
| } | ||
|
|
There was a problem hiding this comment.
type docs would help here (and above) for clarity, even if not checked. Hard to wade through all the flattens :) @return {!Array<!Array<!RequestNode>>}?
There was a problem hiding this comment.
added some types. Did not annotate everything but I hope it makes it little more readable
|
looks great! |
Summary
Getting critical request chains from dev tools initiator info instead of clovis graph. I get the same results as with clovis graph, perhaps even somewhat more stable since clovis often ran into hard to reproduce timing errors.
How to run this
The new gatherer now runs by default, with or without flags. However, if you want to see the results of the gather you still have to use the
--use-net-dep-graphflag. For some websites, the urls can be crazy long and they flood the cli output. I didn't want that to happen by default.Testing
I would really appreciate input on I can test this better - the automated tests included in this PR only test the graph algorithm, not whether the initiator info is right or whether all critical requests are being captured. I ran in against some live sites and the output looked reasonable.
Extension builds and runs fine.
Meta
This is based on #300 (which is why it has unrelated python changes) but I believe this can be merged directly without merging #300 first. Sorry for the 17 commits in that case, pls squash before merging. Refs #292
Can we delete the python code
It's probably best to hold off on doing that until we can conclusively decide we won't be needing the clovis graph.