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

core(network-analyzer): estimate from lrStatistics #15158

Merged
merged 1 commit into from
Jun 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions core/lib/dependency-graph/simulator/network-analyzer.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,15 @@ class NetworkAnalyzer {
return NetworkAnalyzer._estimateValueByOrigin(records, ({timing, connectionReused, record}) => {
if (connectionReused) return;

// In LR, network records are missing connection timing, but we've smuggled it in via headers.
if (global.isLightrider && record.lrStatistics) {
if (record.protocol.startsWith('h3')) {
return record.lrStatistics.TCPMs;
} else {
return [record.lrStatistics.TCPMs / 2, record.lrStatistics.TCPMs / 2];
}
}

const {connectStart, sslStart, sslEnd, connectEnd} = timing;
if (connectEnd >= 0 && connectStart >= 0 && record.protocol.startsWith('h3')) {
// These values are equal to sslStart and sslEnd for h3.
Expand Down Expand Up @@ -247,6 +256,10 @@ class NetworkAnalyzer {
*/
static _estimateResponseTimeByOrigin(records, rttByOrigin) {
return NetworkAnalyzer._estimateValueByOrigin(records, ({record, timing}) => {
// Lightrider does not have timings for sendEnd, but we do have this timing which should be
// close to the response time.
if (global.isLightrider && record.lrStatistics) return record.lrStatistics.requestMs;

if (!Number.isFinite(timing.receiveHeadersEnd) || timing.receiveHeadersEnd < 0) return;
if (!Number.isFinite(timing.sendEnd) || timing.sendEnd < 0) return;

Expand Down
15 changes: 6 additions & 9 deletions core/lib/network-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import UrlUtils from './url-utils.js';

// Lightrider X-Header names for timing information.
// See: _updateTransferSizeForLightrider and _updateTimingsForLightrider.
const HEADER_TCP = 'X-TCPMs';
const HEADER_TCP = 'X-TCPMs'; // Note: this should have been called something like ConnectMs, as it includes SSL.
const HEADER_SSL = 'X-SSLMs';
const HEADER_REQ = 'X-RequestMs';
const HEADER_RES = 'X-ResponseMs';
Expand All @@ -80,14 +80,10 @@ const HEADER_PROTOCOL_IS_H2 = 'X-ProtocolIsH2';

/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clarified these docstrings, and formatted such that they apply to the correct variables (it was off...)

* @typedef LightriderStatistics
* The difference in networkEndTime between the observed Lighthouse networkEndTime and Lightrider's derived networkEndTime.
* @property {number} endTimeDeltaMs
* The time spent making a TCP connection (connect + SSL).
* @property {number} TCPMs
* The time spent requesting a resource from a remote server, we use this to approx RTT.
* @property {number} requestMs
* The time spent transferring a resource from a remote server.
* @property {number} responseMs
* @property {number} endTimeDeltaMs The difference in networkEndTime between the observed Lighthouse networkEndTime and Lightrider's derived networkEndTime.
* @property {number} TCPMs The time spent making a TCP connection (connect + SSL). Note: this is poorly named.
* @property {number} requestMs The time spent requesting a resource from a remote server, we use this to approx RTT. Note: this is poorly names, it really should be "server response time".
* @property {number} responseMs Time to receive the entire response payload starting the clock on receiving the first fragment (first non-header byte).
*/

/** @type {LH.Util.SelfMap<LH.Crdp.Network.ResourceType>} */
Expand Down Expand Up @@ -501,6 +497,7 @@ class NetworkRequest {

// Make sure all times are initialized and are non-negative.
const TCPMs = TCPMsHeader ? Math.max(0, parseInt(TCPMsHeader.value)) : 0;
// This is missing for h2 requests, but present for h1. See b/283843975
const SSLMs = SSLMsHeader ? Math.max(0, parseInt(SSLMsHeader.value)) : 0;
const requestMs = requestMsHeader ? Math.max(0, parseInt(requestMsHeader.value)) : 0;
const responseMs = responseMsHeader ? Math.max(0, parseInt(responseMsHeader.value)) : 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ const devtoolsLog = readJson('../../../fixtures/traces/progressive-app-m60.devto
const devtoolsLogWithRedirect = readJson('../../../fixtures/artifacts/redirect/devtoolslog.json', import.meta);

describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
afterEach(() => {
global.isLightrider = undefined;
});

let recordId;

function createRecord(opts) {
Expand Down Expand Up @@ -248,6 +252,22 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
assertCloseEnough(result.median, resultApprox.median, 30);
});
});

it('should use lrStatistics when needed', () => {
global.isLightrider = true;
const record = createRecord({timing: {}, lrStatistics: {TCPMs: 100}});
const result = NetworkAnalyzer.estimateRTTByOrigin([record]);
const expected = {min: 50, max: 50, avg: 50, median: 50};
assert.deepStrictEqual(result.get('https://example.com'), expected);
});

it('should use lrStatistics when needed (h3)', () => {
global.isLightrider = true;
const record = createRecord({protocol: 'h3', timing: {}, lrStatistics: {TCPMs: 100}});
const result = NetworkAnalyzer.estimateRTTByOrigin([record]);
const expected = {min: 100, max: 100, avg: 100, median: 100};
assert.deepStrictEqual(result.get('https://example.com'), expected);
});
});

describe('#estimateServerResponseTimeByOrigin', () => {
Expand Down Expand Up @@ -299,6 +319,14 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
assertCloseEnough(result.median, resultApprox.median, 30);
});
});

it('should use lrStatistics when needed', () => {
global.isLightrider = true;
const record = createRecord({timing: {}, lrStatistics: {requestMs: 100}});
const result = NetworkAnalyzer.estimateServerResponseTimeByOrigin([record]);
const expected = {min: 100, max: 100, avg: 100, median: 100};
assert.deepStrictEqual(result.get('https://example.com'), expected);
});
});

describe('#estimateThroughput', () => {
Expand Down