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

Add time to first byte audit #2126

Merged
merged 4 commits into from
May 9, 2017
Merged

Add time to first byte audit #2126

merged 4 commits into from
May 9, 2017

Conversation

wardpeet
Copy link
Collaborator

@wardpeet wardpeet commented May 2, 2017

Succes:
image

Error:
image

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

this is awesome! we've been sorely missing more server-side-ish audits and love the direction 🎉

const Formatter = require('../report/formatter');
const URL = require('../lib/url-shim');

const TTFB_THRESHOLD = 200;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably use a variable threshold since throttling could easily make every request look like a "failure", maybe we determine a threshold based on the minimum TTFB?

either way we should probably just mark as informative: true in the meta and give the top X requests sorted by slowness

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean to go through all requests? Or just sort the critical ones?
🤔 throttling shouldn't mess up the ttfb as it's a server side metric? So what variable are we thinking of? 10%?

TIL: informative: true , didn't know we had these options 😛

Copy link
Collaborator

Choose a reason for hiding this comment

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

well kinda receiveHeadersEnd - sendEnd will be at least RTT/2 + server response time, right? if we're emulating RTT to 400ms for example everything would start to be flagged

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems right indeed, I believe we're not able to calculate real TTFB. I could do an estimate of a roundtrip as you calculate download time

static audit(artifacts) {
const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
const results = [];
const walk = (node) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

unfortunate that all consumers have to do the walk, I wonder if we should offer a flattened list as a computed artifact too?

Copy link
Collaborator Author

@wardpeet wardpeet May 3, 2017

Choose a reason for hiding this comment

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

would be easier though as we only need the tree for the critical request chain.

should I create an artifact named requestCriticalRequest or requestFlattenedCriticalRequestChains

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking just expose as a flattened property of the existing computed artifact so we don't bother with the computation twice although if you're feeling fancy we could refactor the existing one to call requestCriticalRequests and then build the tree :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good to me

name: 'time-to-firstbyte',
description: 'Time To FirstByte',
helpText: 'Time To First Byte identifies the time at which your server sends a response.',
requiredArtifacts: ['URL', 'networkRecords']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we actually use the URL artifact do we? Just the url-shim?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no sorry forgot to remove it, at first I used this to calculate ttfb of the main document but moved to critical assets later on

@@ -615,6 +620,7 @@ module.exports = {
{"id": "first-meaningful-paint", "weight": 5},
{"id": "speed-index-metric", "weight": 1},
{"id": "estimated-input-latency", "weight": 1},
{"id": "time-to-firstbyte", "weight": 1},
Copy link
Collaborator

Choose a reason for hiding this comment

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

going forward we're only going to be scoring the actual metrics in performance, so we can set this weight to 0

nit: could you move this down below with the other 'informative' audits so we don't split the metrics

artifacts.requestCriticalRequestChains = () => {
return Promise.resolve(
{
'0': {
Copy link
Collaborator

Choose a reason for hiding this comment

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

offering a flattened version would make the tests prettier too :)

category: 'Performance',
name: 'time-to-firstbyte',
description: 'Time To FirstByte',
helpText: 'Time To First Byte identifies the time at which your server sends a response.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for this! Was looking for one but didn't find one immediately

return {
category: 'Performance',
name: 'time-to-firstbyte',
description: 'Time To FirstByte',
Copy link
Contributor

@ebidel ebidel May 3, 2017

Choose a reason for hiding this comment

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

'Time To First Byte (TTFB)'

displayValue = recordsOverBudget.length +
` critical request(s) went over the ${TTFB_THRESHOLD}ms threshold`;
} else {
displayValue = 'All critical server requests are fast.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just omit this if the audit is passing? We usually provide extra context only when the audit is failing.

static caclulateTTFB(record) {
const timing = record._timing;

return timing.receiveHeadersEnd - timing.sendEnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious...do we want sendEnd - sendStart? Seeing Souders and other articles calculate TTFB that way.

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've copied these headers from devtools. Not sure which to follow

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah this is measuring more like response time than TTFB responseStart - startTime is true TTFB but that doesn't seem to be the primary thing we want to be auditing, perhaps we should rename the audit?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be sweet if DT had the same names as resource timing. Alas. Do we have access to responseStart or is that the same as sendEnd? https://chromedevtools.github.io/devtools-protocol/tot/Network/#type-ResourceTiming

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 believe we don't have true TTFB in devtools.

const child = node[id];

const networkRecord = networkRecords.find(record => {
return record._requestId === id;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use one line.

const ttfb = TTFBMetric.caclulateTTFB(networkRecord);
results.push({
url: URL.getDisplayName(networkRecord._url),
ttfb: `${Math.round(ttfb)}ms`,
Copy link
Contributor

Choose a reason for hiding this comment

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

${Math.round(ttfb)} ms

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: and maybe throw in .toLocaleString() for good measure :)

walk(tree);

const recordsOverBudget = results.filter(row =>
row.rawTTFB > TTFB_THRESHOLD + TTFB_THRESHOLD_BUFFER);
Copy link
Contributor

Choose a reason for hiding this comment

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

4 space indent


if (recordsOverBudget.length) {
displayValue = recordsOverBudget.length +
` critical request(s) went over the ${TTFB_THRESHOLD}ms threshold`;
Copy link
Contributor

Choose a reason for hiding this comment

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

4 space indent

Copy link
Contributor

Choose a reason for hiding this comment

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

space before "ms": ${TTFB_THRESHOLD} ms

formatter: Formatter.SUPPORTED_FORMATS.TABLE,
value: {
results,
tableHeadings: {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the v2 report, you'll want to put this in a details table. @paulirish is working on that in #2019

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh i'll have a look

@@ -0,0 +1,89 @@
/**
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

approving early for extra testing.

i'm open to disabling if its not ready.

@paulirish paulirish merged commit 0e80b61 into master May 9, 2017
@paulirish paulirish deleted the feature/audit-ttfb branch May 9, 2017 19:28
@wardpeet
Copy link
Collaborator Author

wardpeet commented May 9, 2017

I had a few changes ready to go, was fixing the details export of audit and cleaning up the criticalRequestChains but I can open an extra PR just for that 😄

@paulirish
Copy link
Member

@wardpeet sounds good. let's do that in a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants