core(network-request): exclude dns from rtt estimates

Author: connorjclark

core/lib/dependency-graph/simulator/network-analyzer.js

@@ -204,11 +204,15 @@ class NetworkAnalyzer {
     if (!Number.isFinite(timing.sendStart) || timing.sendStart < 0) return;
 
     // Assume everything before sendStart was just DNS + (SSL)? + TCP handshake
-    // 1 RT for DNS, 1 RT (maybe) for SSL, 1 RT for TCP
-    let roundTrips = 1;
+    // 1 RT (maybe) for SSL, 1 RT for TCP
+    // DNS is not included because
+    // 1) it is very variable in terms of how many hops it could be (as little as 0, if cached locally)
+    // 2) it doesn't even communicate with the server, so it is useless for RTT calculation.
+    let roundTrips = 0;
     if (!record.protocol.startsWith('h3')) roundTrips += 1; // TCP
     if (record.parsedURL.scheme === 'https') roundTrips += 1;
-    return timing.sendStart / roundTrips;
+    const dnsTime = timing.dnsStart >= 0 ? timing.dnsEnd - timing.dnsStart : 0;
+    return (timing.sendStart - dnsTime) / roundTrips;
   }
 
   /**
@@ -237,13 +241,15 @@ class NetworkAnalyzer {
     // When connection was fresh...
     // TTFB = DNS + (SSL)? + TCP handshake + 1 RT for request + server response time
     if (!connectionReused) {
-      roundTrips += 1; // DNS
+      // We purposely exclude DNS from RTT estimate, as it is unrelated to RTT to the server.
       if (!record.protocol.startsWith('h3')) roundTrips += 1; // TCP
       if (record.parsedURL.scheme === 'https') roundTrips += 1; // SSL
     }
 
-    // subtract out our estimated server response time
-    return Math.max((timing.receiveHeadersEnd - estimatedServerResponseTime) / roundTrips, 3);
+    // subtract out our estimated server response time and dns time
+    const dnsTime = timing.dnsStart >= 0 ? timing.dnsEnd - timing.dnsStart : 0;
+    const duration = timing.receiveHeadersEnd - estimatedServerResponseTime - dnsTime;
+    return Math.max(duration / roundTrips, 3);
   }
 
   /**

core/test/audits/largest-contentful-paint-element-test.js

@@ -114,11 +114,11 @@ describe('Performance: largest-contentful-paint-element audit', () => {
     expect(auditResult.details.items[1].items[0].phase).toBeDisplayString('TTFB');
     expect(auditResult.details.items[1].items[0].timing).toBeCloseTo(800, 0.1);
     expect(auditResult.details.items[1].items[1].phase).toBeDisplayString('Load Delay');
-    expect(auditResult.details.items[1].items[1].timing).toBeCloseTo(651, 0.1);
+    expect(auditResult.details.items[1].items[1].timing).toBeCloseTo(650.25, 0.1);
     expect(auditResult.details.items[1].items[2].phase).toBeDisplayString('Load Time');
-    expect(auditResult.details.items[1].items[2].timing).toBeCloseTo(1813.7, 0.1);
+    expect(auditResult.details.items[1].items[2].timing).toBeCloseTo(1812.8, 0.1);
     expect(auditResult.details.items[1].items[3].phase).toBeDisplayString('Render Delay');
-    expect(auditResult.details.items[1].items[3].timing).toBeCloseTo(2539.2, 0.1);
+    expect(auditResult.details.items[1].items[3].timing).toBeCloseTo(2537.9, 0.1);
   });
 
   it('doesn\'t throw an error when there is nothing to show', async () => {

core/test/computed/metrics/lantern-largest-contentful-paint-test.js

@@ -24,14 +24,14 @@ describe('Metrics: Lantern LCP', () => {
     );
 
     expect({
-      timing: Math.round(result.timing),
-      optimistic: Math.round(result.optimisticEstimate.timeInMs),
-      pessimistic: Math.round(result.pessimisticEstimate.timeInMs)}).
+  timing: Math.round(result.timing),
+  optimistic: Math.round(result.optimisticEstimate.timeInMs),
+  pessimistic: Math.round(result.pessimisticEstimate.timeInMs) }).
 toMatchInlineSnapshot(`
 Object {
-  "optimistic": 2294,
-  "pessimistic": 3233,
-  "timing": 2764,
+  "optimistic": 2293,
+  "pessimistic": 3232,
+  "timing": 2763,
 }
 `);
     assert.equal(result.optimisticEstimate.nodeTimings.size, 12);

core/test/computed/metrics/largest-contentful-paint-test.js

@@ -25,14 +25,14 @@ describe('Metrics: LCP', () => {
       settings, URL}, context);
 
     expect({
-      timing: Math.round(result.timing),
-      optimistic: Math.round(result.optimisticEstimate.timeInMs),
-      pessimistic: Math.round(result.pessimisticEstimate.timeInMs)}).
+  timing: Math.round(result.timing),
+  optimistic: Math.round(result.optimisticEstimate.timeInMs),
+  pessimistic: Math.round(result.pessimisticEstimate.timeInMs) }).
 toMatchInlineSnapshot(`
 Object {
-  "optimistic": 2294,
-  "pessimistic": 3233,
-  "timing": 2764,
+  "optimistic": 2293,
+  "pessimistic": 3232,
+  "timing": 2763,
 }
 `);
   });

core/test/computed/metrics/lcp-breakdown-test.js

@@ -129,8 +129,8 @@ describe('LCPBreakdown', () => {
     const result = await LCPBreakdown.request(data, {computedCache: new Map()});
 
     expect(result.ttfb).toBeCloseTo(800, 0.1);
-    expect(result.loadStart).toBeCloseTo(2579.5, 0.1);
-    expect(result.loadEnd).toBeCloseTo(5804, 0.1);
+    expect(result.loadStart).toBeCloseTo(2578.2, 0.1);
+    expect(result.loadEnd).toBeCloseTo(5801, 0.1);
   });
 
   it('returns observed for image LCP', async () => {

core/test/computed/metrics/time-to-first-byte-test.js

@@ -91,7 +91,7 @@ describe('Metrics: TTFB', () => {
 
     // 3 * 150 RTT + 99 server response time
     // 99 Comes from (100ms observed TTFB - 1ms observed RTT)
-    expect(result.timing).toEqual(549);
+    expect(result.timing).toEqual(548.5);
     expect(result.timestamp).toBeUndefined();
   });
 
@@ -105,7 +105,7 @@ describe('Metrics: TTFB', () => {
 
     // 4 * 150 RTT + 99.1 server response time
     // 99.1 Comes from (100ms observed TTFB - 0.9ms observed RTT)
-    expect(result.timing).toEqual(699.1);
+    expect(result.timing).toEqual(699);
     expect(result.timestamp).toBeUndefined();
   });
 

core/test/computed/network-analysis-test.js

@@ -46,9 +46,24 @@ Map {
     });
 
     const result = await NetworkAnalysis.request(mutatedLog, {computedCache: new Map()});
-    // If the connection timings were not removed, this would be the 1.045 estimate as seen in
+    // If the connection timings were not removed, these estimates would be the same as in
     // the test above. However, without connection timings we fall back to a coarse estimate and
-    // get this instead.
-    expect(result.additionalRttByOrigin.get('https://www.google-analytics.com')).toBeCloseTo(2.86);
+    // get slightly different results.
+    expect(result.additionalRttByOrigin).toMatchInlineSnapshot(`
+Map {
+  "https://pwa.rocks" => 0.8417000135522703,
+  "https://www.googletagmanager.com" => 0.4456999959075678,
+  "https://www.google-analytics.com" => 0,
+  "__SUMMARY__" => 0,
+}
+`);
+    expect(result.serverResponseTimeByOrigin).toMatchInlineSnapshot(`
+Map {
+  "https://pwa.rocks" => 159.70249997917608,
+  "https://www.googletagmanager.com" => 153.03200000198592,
+  "https://www.google-analytics.com" => 161.04569999879467,
+  "__SUMMARY__" => 159.70249997917608,
+}
+`);
   });
 });