core(tti): update ignorable network requests and start point #5021
Conversation
| @@ -28,8 +33,11 @@ class ConsistentlyInteractive extends MetricArtifact { | |||
| */ | |||
| static _findNetworkQuietPeriods(networkRecords, traceOfTab) { | |||
| const traceEndTsInMs = traceOfTab.timestamps.traceEnd / 1000; | |||
| /** @param {LH.WebInspector.NetworkRequest} record */ | |||
There was a problem hiding this comment.
a comment for the filter would be good here, even though it's in the linked doc
| @@ -28,8 +33,11 @@ class ConsistentlyInteractive extends MetricArtifact { | |||
| */ | |||
| static _findNetworkQuietPeriods(networkRecords, traceOfTab) { | |||
| const traceEndTsInMs = traceOfTab.timestamps.traceEnd / 1000; | |||
| /** @param {LH.WebInspector.NetworkRequest} record */ | |||
| const filter = record => record.finished && record.requestMethod === 'GET' && !record.failed && | |||
| Math.floor(record.statusCode / 100) < 4; | |||
There was a problem hiding this comment.
added a comment 😆
| @@ -97,7 +97,7 @@ const ERRORS = { | |||
|
|
|||
| Object.keys(ERRORS).forEach(code => ERRORS[code].code = code); | |||
|
|
|||
| /** @type {Object<string, LighthouseErrorDefinition>} */ | |||
| /** @type {Object<keyof ERRORS, LighthouseErrorDefinition>} */ | |||
| return NetworkRecorder.findNetworkQuietPeriods(networkRecords, | ||
| ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs); | ||
| ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs, filter); |
There was a problem hiding this comment.
making NetworkRecorder.findNetworkQuietPeriods polymorphic like this makes me nervous since we use it for monitoring network quiet during page load. In CLI usage it's no big deal since this always happens after gathering is finished, but wdyt about avoiding the question altogether and filter before passing in?
There was a problem hiding this comment.
filtering before we passing networkrecords seems reasonable.
There was a problem hiding this comment.
yeah sg, changed to filter in TTI
| @@ -28,8 +33,11 @@ class ConsistentlyInteractive extends MetricArtifact { | |||
| */ | |||
| static _findNetworkQuietPeriods(networkRecords, traceOfTab) { | |||
| const traceEndTsInMs = traceOfTab.timestamps.traceEnd / 1000; | |||
| /** @param {LH.WebInspector.NetworkRequest} record */ | |||
| const filter = record => record.finished && record.requestMethod === 'GET' && !record.failed && | |||
There was a problem hiding this comment.
don't need it anymore
| return NetworkRecorder.findNetworkQuietPeriods(networkRecords, | ||
| ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs); | ||
| ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs, filter); |
There was a problem hiding this comment.
filtering before we passing networkrecords seems reasonable.
| @@ -79,11 +87,11 @@ class ConsistentlyInteractive extends MetricArtifact { | |||
| * @return {{cpuQuietPeriod: TimePeriod, networkQuietPeriod: TimePeriod, cpuQuietPeriods: Array<TimePeriod>, networkQuietPeriods: Array<TimePeriod>}} | |||
| */ | |||
| static findOverlappingQuietPeriods(longTasks, networkRecords, traceOfTab) { | |||
| const FMPTsInMs = traceOfTab.timestamps.firstMeaningfulPaint / 1000; | |||
| const FCPTsInMs = traceOfTab.timestamps.firstContentfulPaint / 1000; | |||
updates TTI definition based on v1.0 in the doc
NOTE: this PR does not tweak our decision of when to stop tracing, just the computation for observed TTI
ref #4629 #5008 #4333