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

clients(lightrider): always get transferSize from X-TotalFetchedSize header #7478

Merged
merged 11 commits into from
Mar 14, 2019
Merged

clients(lightrider): always get transferSize from X-TotalFetchedSize header #7478

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8406bc9
lightrider: always get transferSize from X-TotalFetchedSize header
connorjclark Mar 12, 2019
e8cbbe2
comments and more explicit handling
connorjclark Mar 13, 2019
49c70ca
parse headers before using them
connorjclark Mar 13, 2019
a208c69
add test
connorjclark Mar 13, 2019
fd7d187
force add devtools log
connorjclark Mar 13, 2019
17e6bd8
revert some changes
connorjclark Mar 13, 2019
9ff988b
trim devtools log
connorjclark Mar 13, 2019
852ed20
move
connorjclark Mar 13, 2019
e669481
move
connorjclark Mar 13, 2019
779fcfa
revise comment
connorjclark Mar 13, 2019
e50eb65
//
connorjclark Mar 13, 2019
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
2 changes: 1 addition & 1 deletion clients/lightrider-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
const {lrDevice, categoryIDs, logAssets, keepRawValues} = lrOpts;

// Certain fixes need to kick in under LR, see https://github.com/GoogleChrome/lighthouse/issues/5839
global.isLightRider = true;
global.isLightrider = true;

// disableStorageReset because it causes render server hang
flags.disableStorageReset = true;
Expand Down
27 changes: 17 additions & 10 deletions lighthouse-core/lib/network-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ module.exports = class NetworkRequest {
}

this._updateResponseReceivedTimeIfNecessary();
this._updateTransferSizeForLightRiderIfNecessary();
this._updateTransferSizeForLightrider();
}

/**
Expand All @@ -209,6 +209,7 @@ module.exports = class NetworkRequest {
this.localizedFailDescription = data.errorText;

this._updateResponseReceivedTimeIfNecessary();
this._updateTransferSizeForLightrider();
}

/**
Expand Down Expand Up @@ -246,6 +247,18 @@ module.exports = class NetworkRequest {

this.responseReceivedTime = timestamp;

this.responseHeadersText = response.headersText || '';
Copy link
Member

Choose a reason for hiding this comment

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

move these lines back?

this.responseHeaders = NetworkRequest._headersDictToHeadersArray(response.headers);

// The total length of the encoded data is spread out among multiple events. The sum of the
// values in onResponseReceived and all the onDataReceived events will equal the value
// seen on the onLoadingFinished event. As we process onResonseReceived and onDataReceived
Copy link
Member

Choose a reason for hiding this comment

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

The sum of the
// values in onResponseReceived and all the onDataReceived events will equal the value
// seen on the onLoadingFinished event.

I don't know exactly what leads to a situation when the loadingFinished total is different than the sum of the parts, but apparently it does happen.

I just tried against 6 saved devtoolslogs i have. (Disclaimer: the artifacts were collected months ago definitely without networkservice, though I wouldn't expect it to matter)... There's, 320 network requests in total and nearly all of them have a sum that matches (or the sum was 0).. But I found 3 requests where this wasn't the case.

encodedSum encodedTotalInLoadingFinished url
108 12640 http://localhost:10200/dobetterweb/dbw_tester.html
139 144 http://localhost:10200/dobetterweb/empty_module.js?delay=500
650 714 https://s.amazon-adsystem.com/x/da2e6c890e6e3636

Again.. don't know what leads to this situation, but it happens. I'm happy with admitting that in the comment or we can ask caseq/network people about the circumstances..

Copy link
Collaborator Author

@connorjclark connorjclark Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

ooh that is funky. With NS off, it's good. But on it was off by 5 for "empty_module?delay=500". And there was no dataReceived event!

// we accumulate the total encodedDataLength. When we process onLoadingFinished, we override
// the accumulated total. We do this so that if the request is aborted or fails, we still get
// a value via the accumulation.
// In Lightrider, we do not have true value for encodedDataLength, and we get the actual size
Copy link
Member

Choose a reason for hiding this comment

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

we do no accumulation.

Looks like we do, as of now.. We just override the transferSize in loadingFinished

Thank you VERY MUCH for writing all this shit out. These comments are so Good.

here's a very small rewrite of this second para for clarity:

In Lightrider, due to instrumentation limitations, our values for encodedDataLength are bogus and not valid. However the resource's true encodedDataLength/transferSize is shared via a special response header, X-TotalFetchedSize. In this situation, we read this value from responseReceived, use it for the transferSize and ignore the encodedDataLength values in both dataReceived and loadingFinished.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops thanks for the revisions ❤️

// of the encoded data via a special response header. Because the values are totally bogus,
// we do no accumulation.
Copy link
Member

Choose a reason for hiding this comment

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

move comment down to _updateTransferSizeForLightrider?

this.transferSize = response.encodedDataLength;
if (typeof response.fromDiskCache === 'boolean') this.fromDiskCache = response.fromDiskCache;

Expand All @@ -254,15 +267,11 @@ module.exports = class NetworkRequest {
this.timing = response.timing;
if (resourceType) this.resourceType = RESOURCE_TYPES[resourceType];
this.mimeType = response.mimeType;
this.responseHeadersText = response.headersText || '';
this.responseHeaders = NetworkRequest._headersDictToHeadersArray(response.headers);

this.fetchedViaServiceWorker = !!response.fromServiceWorker;

if (this.fromMemoryCache) this.timing = undefined;
if (this.timing) this._recomputeTimesWithResourceTiming(this.timing);

this._updateTransferSizeForLightRiderIfNecessary();
}

/**
Expand Down Expand Up @@ -297,12 +306,10 @@ module.exports = class NetworkRequest {

/**
* LR loses transfer size information, but passes it in the 'X-TotalFetchedSize' header.
* 'X-TotalFetchedSize' is the canonical transfer size in LR. Nothing should supersede it.
*/
_updateTransferSizeForLightRiderIfNecessary() {
// Bail if we're not in LightRider, this only applies there.
if (!global.isLightRider) return;
// Bail if we somehow already have transfer size data.
if (this.transferSize) return;
_updateTransferSizeForLightrider() {
Copy link
Member

Choose a reason for hiding this comment

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

can you also leave a comment on L70 that says go read the _updateTransferSizeForLightrider comment?

if (!global.isLightrider) return;

const totalFetchedSize = this.responseHeaders.find(item => item.name === 'X-TotalFetchedSize');
// Bail if the header was missing.
Expand Down
Loading