Skip to content

core(network-recorder): handle QUIC requests #5256

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.

Sign up for GitHub

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

Merged
paulirish merged 3 commits into from
May 19, 2018
Merged

core(network-recorder): handle QUIC requests #5256

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
671b2af
core(network-recorder): handle QUIC requests
patrickhulce May 17, 2018
62115b0
fix
patrickhulce May 17, 2018
e753060
revert -Infinity, separate method
patrickhulce May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lighthouse-core/gather/computed/metrics/interactive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,14 @@ class Interactive extends MetricArtifact {
// Consider network records that had 4xx/5xx status code as "failed"
record.statusCode < 400;
});
return NetworkRecorder.findNetworkQuietPeriods(filteredNetworkRecords,

const periods = NetworkRecorder.findNetworkQuietPeriods(filteredNetworkRecords,
ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs);
periods.forEach(period => {
if (period.start === -Infinity) period.start = 0;
});

return periods;
}

/**
Expand Down
10 changes: 8 additions & 2 deletions lighthouse-core/lib/network-recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,15 @@ class NetworkRecorder extends EventEmitter {
return;
}

// QUIC network requests don't always "finish" even when they're done loading data
// Use receivedHeaders instead, see https://github.com/GoogleChrome/lighthouse/issues/5254
const isQUIC = record._responseHeaders && record._responseHeaders
Copy link
Contributor

@brendankenny brendankenny May 18, 2018

Choose a reason for hiding this comment

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

would be nice pulled out into a function to separate determining if request is QUIC vs what to do with QUIC requests

Copy link
Contributor

@brendankenny brendankenny May 18, 2018

Choose a reason for hiding this comment

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

it could also be a function that's like isQuicAndFinished(record) to absorb logic below, too :)

Copy link
Collaborator Author

@patrickhulce patrickhulce May 18, 2018

Choose a reason for hiding this comment

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

sure

.find(header => header.name.toLowerCase() === 'alt-svc' && /quic/.test(header.value));
Copy link
Contributor

@brendankenny brendankenny May 18, 2018

Choose a reason for hiding this comment

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

how certain is the 'alt-svc' check? Any other way to tell?

Copy link
Collaborator Author

@patrickhulce patrickhulce May 18, 2018

Choose a reason for hiding this comment

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

seems to be fairly certain, in the spec it's not technically required to announce it buuuuuut...

While HTTP/QUIC doesn't formally require a client to implement Alt-Svc, there's no discovery mechanism other than Alt-Svc provided, so you're not going to get very far without
it.

from quicwg/base-drafts#253

const receivedHeaders = record._timing && record._timing.receiveHeadersEnd > 0;

// convert the network record timestamp to ms
timeBoundaries.push({time: record.startTime * 1000, isStart: true});
if (record.finished) {
if (record.finished || (isQUIC && receivedHeaders && record.endTime)) {
timeBoundaries.push({time: record.endTime * 1000, isStart: false});
}
});
Expand All @@ -119,7 +125,7 @@ class NetworkRecorder extends EventEmitter {
.sort((a, b) => a.time - b.time);

let numInflightRequests = 0;
let quietPeriodStart = 0;
let quietPeriodStart = -Infinity;
Copy link
Contributor

@brendankenny brendankenny May 18, 2018

Choose a reason for hiding this comment

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

what's the advantage of a [-Infinity, 0] period? Time starting at 0 also seems like a reasonable state of things

Copy link
Collaborator Author

@patrickhulce patrickhulce May 18, 2018

Choose a reason for hiding this comment

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

that's fine just made tests have [0, 0] quiet period so we either handle the empty period case, move test cases to past 0, or -Infinity, -Infinity seemed like most reasonable conceptually but I do not have strong feelings

Copy link
Contributor

@brendankenny brendankenny May 18, 2018

Choose a reason for hiding this comment

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

it may be fine, and I haven't spent enough time here recently to have a good gut instinct, but I think I would expect the earliest quiet period to start at 0.

I guess I don't understand what this is addressing. Seems independent of the QUIC change?

/** @type {Array<{start: number, end: number}>} */
const quietPeriods = [];
timeBoundaries.forEach(boundary => {
Expand Down
97 changes: 97 additions & 0 deletions lighthouse-core/test/gather/network-recorder-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,101 @@ describe('network recorder', function() {
const records = NetworkRecorder.recordsFromLogs(devtoolsLogItems);
assert.equal(records.length, 76);
});

describe('#findNetworkQuietPeriods', () => {
function record(data) {
const url = data.url || 'https://example.com';
const scheme = url.split(':')[0];
return Object.assign({
url,
finished: !!data.endTime,
parsedURL: {scheme},
}, data);
}

it('should find the 0-quiet periods', () => {
const records = [
record({startTime: 0, endTime: 1}),
record({startTime: 2, endTime: 3}),
record({startTime: 4, endTime: 5}),
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0);
assert.deepStrictEqual(periods, [
{start: -Infinity, end: 0},
{start: 1000, end: 2000},
{start: 3000, end: 4000},
{start: 5000, end: Infinity},
]);
});

it('should find the 2-quiet periods', () => {
const records = [
record({startTime: 0, endTime: 1.5}),
record({startTime: 0, endTime: 2}),
record({startTime: 0, endTime: 2.5}),
record({startTime: 2, endTime: 3}),
record({startTime: 4, endTime: 5}),
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 2);
assert.deepStrictEqual(periods, [
{start: -Infinity, end: 0},
{start: 1500, end: Infinity},
]);
});

it('should handle unfinished requests', () => {
const records = [
record({startTime: 0, endTime: 1.5}),
record({startTime: 0, endTime: 2}),
record({startTime: 0, endTime: 2.5}),
record({startTime: 2, endTime: 3}),
record({startTime: 2}),
record({startTime: 2}),
record({startTime: 4, endTime: 5}),
record({startTime: 5.5}),
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 2);
assert.deepStrictEqual(periods, [
{start: -Infinity, end: 0},
{start: 1500, end: 2000},
{start: 3000, end: 4000},
{start: 5000, end: 5500},
]);
});

it('should ignore data URIs', () => {
const records = [
record({startTime: 0, endTime: 1}),
record({startTime: 0, endTime: 2, url: 'data:image/png;base64,'}),
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0);
assert.deepStrictEqual(periods, [
{start: -Infinity, end: 0},
{start: 1000, end: Infinity},
]);
});

it('should handle QUIC requests', () => {
const quicRequest = {
finished: false,
_responseHeaders: [{name: 'ALT-SVC', value: 'hq=":49288";quic="1,1abadaba,51303334,0"'}],
_timing: {receiveHeadersEnd: 1.28},
};

const records = [
record({startTime: 0, endTime: 1}),
record({startTime: 0, endTime: 2, ...quicRequest}),
];

const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0);
assert.deepStrictEqual(periods, [
{start: -Infinity, end: 0},
{start: 2000, end: Infinity},
]);
});
});
});