-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Plots: A/B screenshot viewer #2026
Conversation
const ssJSONFilename = 'the_file-0.screenshots.json'; | ||
const ssFileContents = fs.readFileSync(ssJSONFilename, 'utf8'); | ||
const expectedScreenshotContent = '"timestamp": 674089419.919,'; | ||
assert.ok(ssFileContents.includes(expectedScreenshotContent), 'unexpected screenshot json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can be a bit stronger with our assertions here, maybe that it parses and it has the timestamp property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
benim sitem çok yavaş : (( bana yardım edermisiniz http://ankaraotokurtarici.com
plots/ab-screenshot/analyze.js
Outdated
|
||
const fcpTiming = getTiming('ttfcp'); | ||
const fmpTiming = getTiming('ttfmp'); | ||
const vc100Timing = getTiming('vc100'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get 85% too? not sure if it's in pwmetrics yet but we should since we'll be switching up TTI soon enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
} | ||
|
||
/** | ||
* Analyzes the screenshots for the first run of a particular site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I guess the order doesn't really matter then if it's not an aggregation of any sort? I guess we could just pull any random folder huh...sorting just for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it doesn't really matter but it's nice to know it's always the first run when you're looking up the artifacts for a particular run (e.g. open the trace file for a fishy run).
love being able to compare, and the screenshots mode makes this amazing for a very visual look at what's going on. For a major switch (like a site going non-PWA to PWA) or
this will be awesome. Do you have plans for comparing two distributions of runs, too? I think for more minor changes (as opposed to switching to a totally new TTI) that's going to give a better picture of what's going on than comparing just two runs at a time. Also did you see #2011? Pretty please :) |
@brendankenny I have it on my to-do list to create a box plot which will show distributions of runs for a particular site over time (e.g. different lighthouse versions). My sense is that this will be more valuable for the Lighthouse team in the longer-term (i.e. post I/O) when the metrics have settled - but if this is a high priority for the team, I can work on it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take another look.
const ssJSONFilename = 'the_file-0.screenshots.json'; | ||
const ssFileContents = fs.readFileSync(ssJSONFilename, 'utf8'); | ||
const expectedScreenshotContent = '"timestamp": 674089419.919,'; | ||
assert.ok(ssFileContents.includes(expectedScreenshotContent), 'unexpected screenshot json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
/** | ||
* Analyzes the screenshots for the first run of a particular site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it doesn't really matter but it's nice to know it's always the first run when you're looking up the artifacts for a particular run (e.g. open the trace file for a fishy run).
plots/ab-screenshot/analyze.js
Outdated
|
||
const fcpTiming = getTiming('ttfcp'); | ||
const fmpTiming = getTiming('ttfmp'); | ||
const vc100Timing = getTiming('vc100'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a-ok by me
fs.writeFileSync(screenshotsHTMLFilename, data.screenshotsHTML); | ||
log.log('screenshots saved to disk', screenshotsHTMLFilename); | ||
|
||
const screenshotsJSONFilename = `${pathWithBasename}-${index}.screenshots.json`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure to add this to gitignore too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
plots/ab-screenshot/README.md
Outdated
$ node analyze.js -a ../out-first -b ../out-second | ||
|
||
# Use the screenshot viewer | ||
# Open index.html in browser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be dope to use opn
and have analyze just open the graphs once it's done :)
Re-opening PR to retrigger Travis job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome stuff. I gave you a PR with a few more goodies that really makes this feature pretty hot:
wwwillchen#1
with that and the other stuff in here sorted, i'm feelin p good!
* lighthouse can affect perf results in real-world sites. | ||
* | ||
* Example usage: | ||
* node analyze.js -a ./out-first -b ./out-second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm thinking there is also the usecase of...
i want to compare two traces that were captured in the same
measure
run.
so like:
node analyze -a ./out/www.espn.com-./0/ -b ./out/www.espn.com-./2/
however I suppose your approach here is to show multiple sites across these two runs.
should we support both ways?
i'm down with whichever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally didn't think about this use case because then you're comparing two traces with the same configuration (e.g. lighthouse, chrome version) and in that case, I think the end goal could be to either A) visualize the variance between runs when all factors are held constant or B) compare multiple sites (e.g. a PM wants to compare their site vs. 3 competitors).
These seem like nice to have cases but not essential. Let's defer them until later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's defer them until later?
k
plots/ab-screenshot/analyze.js
Outdated
* @return {!SingleRunScreenshots} | ||
*/ | ||
function analyzeSingleRunScreenshots(sitePath) { | ||
const runDir = sortRunFolders(fs.readdirSync(sitePath))[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oy - fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arkadaşlar ben siteme bunu nasıl ekleyebilirim ? http://ankaraotokurtarici.com
Lütfen bana yardım edermisiniz
Help me pls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulirish ptal.
fs.writeFileSync(screenshotsHTMLFilename, data.screenshotsHTML); | ||
log.log('screenshots saved to disk', screenshotsHTMLFilename); | ||
|
||
const screenshotsJSONFilename = `${pathWithBasename}-${index}.screenshots.json`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* lighthouse can affect perf results in real-world sites. | ||
* | ||
* Example usage: | ||
* node analyze.js -a ./out-first -b ./out-second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally didn't think about this use case because then you're comparing two traces with the same configuration (e.g. lighthouse, chrome version) and in that case, I think the end goal could be to either A) visualize the variance between runs when all factors are held constant or B) compare multiple sites (e.g. a PM wants to compare their site vs. 3 competitors).
These seem like nice to have cases but not essential. Let's defer them until later?
plots/ab-screenshot/analyze.js
Outdated
* @return {!SingleRunScreenshots} | ||
*/ | ||
function analyzeSingleRunScreenshots(sitePath) { | ||
const runDir = sortRunFolders(fs.readdirSync(sitePath))[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oy - fixed!
@wwwillchen wdyt about wwwillchen#1 ? |
plots: add kbd nav, more persistent popover.
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@paulirish nice! merged it. Dunno why I didn't see it the first time 😎 |
Purpose
This enables the lighthouse team to better understand how changes to perf metrics (e.g. the algorithm for FMP) affect the perf results of real world sites.
Mode A: Aligned Screenshot Timeline
This enables easy head-to-head comparison between runs for the same site or even runs across different sites. To make the timeline less cluttered, I only show some of the screenshots.
Example:
Mode B: Non-Aligned Screenshot Timeline
This shows more detail than the other mode and shows every screenshot collected.
Example:
If this tool proves to be useful - we can work on making it easy to use for external users. For example, I can imagine a product manager be very interested in the Aligned Screenshot Timeline View for their company and top 3 competitors.
Closes part of #1980