-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(lhr): expose environment info #5871
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -374,7 +374,9 @@ class GatherRunner { | |
return { | ||
fetchTime: (new Date()).toJSON(), | ||
LighthouseRunWarnings: [], | ||
UserAgent: await options.driver.getUserAgent(), | ||
HostUserAgent: await options.driver.getUserAgent(), | ||
NetworkUserAgent: '', // updated later | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. sg, I also tried There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah. I still have nothings, so that's fine with me. If @paulirish has ideas we can do a follow up PR quickly before we ship it :) |
||
BenchmarkIndex: await options.driver.getBenchmarkIndex(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want to run the benchmark in the target page (vs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I think we'll want to experiment with when this gets evaluated, for example at the moment in CLI case I think it's being affected by being run while Chrome is still being setup. The values I'm seeing are noticably lower than when I run it after page load. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call moving it just after load blank which does the 300 ms waiting already much improves the estimate 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ha, I was going to come back later and say maybe it doesn't make a difference and isn't worth it, so good to hear :) I guess it's possible it'll cause more issues with moving off using |
||
traces: {}, | ||
devtoolsLogs: {}, | ||
settings: options.settings, | ||
|
@@ -420,6 +422,16 @@ class GatherRunner { | |
// Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. | ||
baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; | ||
|
||
const userAgentEntry = passData.devtoolsLog.find(entry => | ||
entry.method === 'Network.requestWillBeSent' && | ||
!!entry.params.request.headers['User-Agent'] | ||
); | ||
|
||
if (userAgentEntry && !baseArtifacts.NetworkUserAgent) { | ||
// @ts-ignore - guaranteed to exist by the find above | ||
baseArtifacts.NetworkUserAgent = userAgentEntry.params.request.headers['User-Agent']; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could also do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the number of ways that have popped up in issues as the way folks are trying to set the user agent string, I thought it was best to grab it directly from network records instead of assume it's done being manipulated after setupDriver There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SG. I guess |
||
|
||
// If requested by config, save pass's trace. | ||
if (passData.trace) { | ||
baseArtifacts.traces[passConfig.passName] = passData.trace; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,6 @@ function registerPerformanceObserverInPage() { | |
window.____lhPerformanceObserver = observer; | ||
} | ||
|
||
|
||
/** | ||
* Used by _waitForCPUIdle and executed in the context of the page, returns time since last long task. | ||
*/ | ||
|
@@ -114,7 +113,35 @@ function getElementsInDocument(selector) { | |
function getOuterHTMLSnippet(element) { | ||
const reOpeningTag = /^.*?>/; | ||
const match = element.outerHTML.match(reOpeningTag); | ||
return match && match[0] || ''; | ||
return (match && match[0]) || ''; | ||
} | ||
|
||
/** | ||
* Computes a memory/CPU performance benchmark index to determine rough device class. | ||
* @see https://docs.google.com/spreadsheets/d/1E0gZwKsxegudkjJl8Fki_sOwHKpqgXwt8aBAfuUaB8A/edit?usp=sharing | ||
* | ||
* The benchmark creates a string of length 100,000 in a loop. | ||
* The returned index is the number of numbers per second the string can be created. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. number of times per second? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. heh, yep fixed :) |
||
* | ||
* - 750+ is a desktop-class device, iPhone X, etc | ||
* - 300+ is a high-end Android phone, Galaxy S8, low-end Chromebook, etc | ||
* - 75+ is a mid-tier Android phone, Nexus 5X, etc | ||
* - <75 is a budget Android phone, Alcatel Ideal, Galaxy J2, etc | ||
*/ | ||
/* istanbul ignore next */ | ||
function ultradumbBenchmark() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we just had a whole thing with page-functions, but I kind of wish this was in its own file for easier running :) I guess we can always add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
const start = Date.now(); | ||
let iterations = 0; | ||
|
||
while (Date.now() - start < 500) { | ||
let s = ''; // eslint-disable-line no-unused-vars | ||
for (let j = 0; j < 100000; j++) s += 'a'; | ||
|
||
iterations++; | ||
} | ||
|
||
const durationInSeconds = (Date.now() - start) / 1000; | ||
return iterations / durationInSeconds; | ||
} | ||
|
||
module.exports = { | ||
|
@@ -123,4 +150,5 @@ module.exports = { | |
checkTimeSinceLastLongTask, | ||
getElementsInDocument, | ||
getOuterHTMLSnippet, | ||
ultradumbBenchmark, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,16 +106,37 @@ describe('GatherRunner', function() { | |
}); | ||
}); | ||
|
||
it('collects user agent as an artifact', () => { | ||
it('collects benchmark as an artifact', async () => { | ||
const url = 'https://example.com'; | ||
const driver = fakeDriver; | ||
const config = new Config({}); | ||
const settings = {}; | ||
const options = {url, driver, config, settings}; | ||
|
||
return GatherRunner.run([], options).then(results => { | ||
assert.equal(results.UserAgent, 'Fake user agent', 'did not find expected user agent string'); | ||
}); | ||
const results = await GatherRunner.run([], options); | ||
expect(Number.isFinite(results.BenchmarkIndex)).toBeTruthy(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like you must be trolling here... |
||
}); | ||
|
||
it('collects host user agent as an artifact', async () => { | ||
const url = 'https://example.com'; | ||
const driver = fakeDriver; | ||
const config = new Config({}); | ||
const settings = {}; | ||
const options = {url, driver, config, settings}; | ||
|
||
const results = await GatherRunner.run([], options); | ||
expect(results.HostUserAgent).toEqual('Fake user agent'); | ||
}); | ||
|
||
it('collects network user agent as an artifact', async () => { | ||
const url = 'https://example.com'; | ||
const driver = fakeDriver; | ||
const config = new Config({passes: [{}]}); | ||
const settings = {}; | ||
const options = {url, driver, config, settings}; | ||
|
||
const results = await GatherRunner.run(config.passes, options); | ||
expect(results.NetworkUserAgent).toContain('Mozilla'); | ||
}); | ||
|
||
it('collects requested and final URLs as an artifact', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,12 @@ declare global { | |
[varName: string]: string; | ||
} | ||
|
||
export interface Environment { | ||
hostUserAgent: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you copy the above doc strings down for these too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
networkUserAgent: string; | ||
benchmarkIndex: number; | ||
} | ||
|
||
/** | ||
* The full output of a Lighthouse run. | ||
*/ | ||
|
@@ -43,6 +49,8 @@ declare global { | |
runWarnings: string[]; | ||
/** The User-Agent string of the browser used run Lighthouse for these results. */ | ||
userAgent: string; | ||
/** Information about the environment in which Lighthouse was run. */ | ||
environment: Environment; | ||
/** Execution timings for the Lighthouse run */ | ||
timing: {total: number, [t: string]: number}; | ||
/** The record of all formatted string locations in the LHR and their corresponding source values. */ | ||
|
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.
will you add a simple docstring here?
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.
yep, done