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.

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

Jump to bottom

core(network-request): loosen lightrider timing checksum #15330

Merged
merged 1 commit into from
Aug 3, 2023
Merged

core(network-request): loosen lightrider timing checksum #15330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 12 additions & 3 deletions core/lib/network-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@
// Bail if there was no totalTime.
if (!totalHeader) return;

const totalMs = parseInt(totalHeader.value);
let totalMs = parseInt(totalHeader.value);

Check warning on line 492 in core/lib/network-request.js

View check run for this annotation

Codecov / codecov/patch

core/lib/network-request.js#L492 

Added line #L492 was not covered by tests
const TCPMsHeader = this.responseHeaders.find(item => item.name === HEADER_TCP);
const SSLMsHeader = this.responseHeaders.find(item => item.name === HEADER_SSL);
const requestMsHeader = this.responseHeaders.find(item => item.name === HEADER_REQ);
Expand All @@ -502,11 +502,20 @@
const requestMs = requestMsHeader ? Math.max(0, parseInt(requestMsHeader.value)) : 0;
const responseMs = responseMsHeader ? Math.max(0, parseInt(responseMsHeader.value)) : 0;

// Bail if the timings don't add up.
if (TCPMs + requestMs + responseMs !== totalMs) {
if (Number.isNaN(TCPMs + requestMs + responseMs + totalMs)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a new requirement, but now checking for NaNs explicitly because the below code no longer does that implicitly.

Copy link
Member

Choose a reason for hiding this comment

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

nit: What if this just checked Number.isNaN(totalMs)? Then the next check would verify the rest. Up to you on which way is more readable. I just thought it isn't obvious that the isNaN gets distributed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just thought it isn't obvious that the isNaN gets distributed.

It's the only property of NaN! :)

Any operation on NaN is still NaN.

Copy link
Collaborator Author

@connorjclark connorjclark Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Except for NaN ** 0 apparently lol https://stackoverflow.com/a/17865241

return;
}

// If things don't add up, tweak the total a bit.
if (TCPMs + requestMs + responseMs !== totalMs) {
const delta = Math.abs(TCPMs + requestMs + responseMs - totalMs);
// We didn't see total being more than 5ms less than the total of the components.
// Allow some discrepancy in the timing, but not too much.
if (delta >= 25) return;

totalMs = TCPMs + requestMs + responseMs;
}

Check warning on line 518 in core/lib/network-request.js

View check run for this annotation

Codecov / codecov/patch

core/lib/network-request.js#L509-L518 

Added lines #L509 - L518 were not covered by tests
// Bail if SSL time is > TCP time.
if (SSLMs > TCPMs) {
return;
Expand Down
16 changes: 15 additions & 1 deletion core/test/lib/network-request-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ describe('NetworkRequest', () => {
expect(record.lrStatistics).toStrictEqual(undefined);
});

it('does nothing if header timings do not add up', () => {
it('accepts if header timings only kinda do not add up', () => {
const req = getRequest();
const tcpHeader = req.responseHeaders[1];
expect(tcpHeader.name).toStrictEqual(NetworkRequest.HEADER_TCP);
Expand All @@ -193,6 +193,20 @@ describe('NetworkRequest', () => {
global.isLightrider = true;
const record = NetworkRecorder.recordsFromLogs(devtoolsLog)[0];

expect(record).toMatchObject(req);
expect(record.lrStatistics).not.toStrictEqual(undefined);
});

it('does nothing if header timings _really_ do not add up', () => {
const req = getRequest();
const tcpHeader = req.responseHeaders[1];
expect(tcpHeader.name).toStrictEqual(NetworkRequest.HEADER_TCP);
tcpHeader.value = '8000';

const devtoolsLog = networkRecordsToDevtoolsLog([req]);
global.isLightrider = true;
const record = NetworkRecorder.recordsFromLogs(devtoolsLog)[0];

expect(record).toMatchObject(req);
expect(record.lrStatistics).toStrictEqual(undefined);
});
Expand Down
Loading