Skip to content
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

misc: lantern trace collection script #9662

Merged
merged 109 commits into from
Dec 19, 2019
Merged

misc: lantern trace collection script #9662

merged 109 commits into from
Dec 19, 2019

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Sep 12, 2019

I only ran it with a few urls @ 3 samples. See example output summary file below. I don't think the format is terribly important, I just wanted a nice formatted way to query the data during later analysis.

Seeking feedback on the data being collected and the options for LH / WPT being used before running the full suite.

Just collecting the traces, since the devtoolsLog can be generated from a trace.

Next steps would be to democratize the analysis of these traces :)

[
  {
    "url": "http://www.hatena.ne.jp/",
    "wpt": [
      {
        "lhr": "http---www-hatena-ne-jp--mobile-wpt-1-lhr.json",
        "trace": "http---www-hatena-ne-jp--mobile-wpt-1-trace.json"
      }
    ],
    "unthrottled": [
      {
        "devtoolsLog": "http---www-hatena-ne-jp--mobile-unthrottled-1-devtoolsLog.json",
        "lhr": "http---www-hatena-ne-jp--mobile-unthrottled-1-lhr.json",
        "trace": "http---www-hatena-ne-jp--mobile-unthrottled-1-trace.json"
      }
    ]
  },
  ...
]


## Lighthouse Version

Check what version of Lighthouse WPT is using. You should use the same version of lighthouse for the desktop collection.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this important? WPT uses LH 5.2.0 rn.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be a big deal as long as we're not turning on different trace categories or something between the versions. observed metric definitions haven't changed in forever.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I take it back, it'll be a big deal because we want LCP and layout stability to be computed for these that's the whole point of getting new ones. I guess we'll have to derive them after the fact from the trace if we want to avoid delaying this an entire release :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so: i'll collect the LHR, devtoolslog, and trace for local unthrottled desktop runs. And the LHR and trace for WPT runs. put them all in a giant zip and upload somewhere.

then make the new golden lantern set by taking the median runs for the local runs. and just run trace-processor on the WPT traces to get the metrics.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right-o, but since we don't have LCP metric yet, I would triple check that the traces you're getting have the LCP and layout stability events in them at least and we don't need to enable some other category to get them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup they do


## Lighthouse Version

Check what version of Lighthouse WPT is using. You should use the same version of lighthouse for the desktop collection.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be a big deal as long as we're not turning on different trace categories or something between the versions. observed metric definitions haven't changed in forever.

apiUrl.searchParams.set('k', WPT_KEY);
apiUrl.searchParams.set('f', 'json');
// Keep the location constant. Use Chrome and 3G network conditions.
apiUrl.searchParams.set('location', 'Dulles:Chrome.3G');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the id for "3G Fast"? They have a separate "3G" profile that is slower than what we target. Dulles_MotoG4:Moto G4 - Chrome.3GFast is the location value I think we want

// Disables some things that WPT does, such as a "repeat view" analysis.
apiUrl.searchParams.set('type', 'lighthouse');
apiUrl.searchParams.set('mobile', '1');
// apiUrl.searchParams.set('mobileDevice', '1');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the parameters that are sent with the request made through the UI

lighthouseTrace: 1
vo: 8508357c0fc2a8119c04362b28b5bf8778ec1110
vd: 2019-09-12T20:59:51 00:00
vh: 4dc4641070f0dccf7187f77ce883f504246a3fec
url: https://patrickhulce.com
where: Mobile_Dulles_MotoG4
browser: MotoG4 - Chrome
location: Dulles_MotoG4:Moto G4 - Chrome.3GFast
bwDown: 1600
bwUp: 768
latency: 150
plr: 0
runs: 1
fvonly: 1
lighthouse: on
mobileDevice: MotoG4
traceCategories: blink,v8,cc,gpu,blink.net,netlog,disabled-by-default-v8.runtime_stats

not sure which of those are necessary to get the job done but perhaps bwDown, bwUp, latency are all necessary too not just location?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bwDown / bwUp sets the network stuff ... doesn't location = 3G do the same?

lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
urlIndex++;
}

log.log('done! archiving ...');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw I think I had to break up the entire trace set into 3-4 archives or things like google drive wouldn't take it. that could probably just be done manually later, but just a heads up

@vercel vercel bot temporarily deployed to staging September 13, 2019 01:57 Inactive
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments to clarify a few things. This is going to be really nice to have!

lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/urls.json Outdated Show resolved Hide resolved
lighthouse-core/scripts/lantern/collect/collect.js Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator Author

Collection script is good and is running in GCP right now. When done, will follow up with a new PR updating the baseline + adding LCP.

@@ -24,7 +24,7 @@ rm -rf lantern-data/
mkdir -p lantern-data/ && cd lantern-data
echo $VERSION > version

gsutil cp gs://lh-lantern-data/lantern-traces-$VERSION.tar.gz lantern-traces.tar.gz
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

DIRNAME="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
LH_ROOT_PATH="$DIRNAME/../../.."
cd $LH_ROOT_PATH

if [[ -f lantern-data/version ]] && [[ "$VERSION" != "$(cat lantern-data/version)" ]]; then
echo "Deleting old lantern data."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nice to give the user a chance to cancel this before deleting, I keep some debug stuff in that folder :D

Suggested change
echo "Deleting old lantern data."
echo "Version out of date. About to delete ./lantern-data..."
echo "Press any key to continue, Ctrl+C to exit"
read -n 1 -r unused_variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would hang travis ci tho?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could throw a $CI in there then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants