From c017181e307d5c725a9bc5ea110fece9622de000 Mon Sep 17 00:00:00 2001
From: Abhijeet Prasad <aprasad@sentry.io>
Date: Tue, 23 Jan 2024 11:04:21 -0500
Subject: [PATCH] fix(tracing): Don't send negative ttfb (#10286)

As per
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/responseStart,
`responseStart` can be 0 if the request is coming straight from the
cache. This might lead us to calculate a negative ttfb.

To account for these scenarios, use `Math.max` to make sure we always
set to 0 in the case of a negative value.
---
 .../src/browser/metrics/index.ts              | 57 +++++++++++++------
 .../test/browser/metrics/index.test.ts        | 29 ++++++++++
 2 files changed, 68 insertions(+), 18 deletions(-)

diff --git a/packages/tracing-internal/src/browser/metrics/index.ts b/packages/tracing-internal/src/browser/metrics/index.ts
index f5652ff2a052..d0eb73b945d2 100644
--- a/packages/tracing-internal/src/browser/metrics/index.ts
+++ b/packages/tracing-internal/src/browser/metrics/index.ts
@@ -240,24 +240,7 @@ export function addPerformanceEntries(transaction: Transaction): void {
 
   // Measurements are only available for pageload transactions
   if (op === 'pageload') {
-    // Generate TTFB (Time to First Byte), which measured as the time between the beginning of the transaction and the
-    // start of the response in milliseconds
-    if (typeof responseStartTimestamp === 'number' && transactionStartTime) {
-      DEBUG_BUILD && logger.log('[Measurements] Adding TTFB');
-      _measurements['ttfb'] = {
-        value: (responseStartTimestamp - transactionStartTime) * 1000,
-        unit: 'millisecond',
-      };
-
-      if (typeof requestStartTimestamp === 'number' && requestStartTimestamp <= responseStartTimestamp) {
-        // Capture the time spent making the request and receiving the first byte of the response.
-        // This is the time between the start of the request and the start of the response in milliseconds.
-        _measurements['ttfb.requestTime'] = {
-          value: (responseStartTimestamp - requestStartTimestamp) * 1000,
-          unit: 'millisecond',
-        };
-      }
-    }
+    _addTtfbToMeasurements(_measurements, responseStartTimestamp, requestStartTimestamp, transactionStartTime);
 
     ['fcp', 'fp', 'lcp'].forEach(name => {
       if (!_measurements[name] || !transactionStartTime || timeOrigin >= transactionStartTime) {
@@ -547,3 +530,41 @@ function setResourceEntrySizeData(
     data[dataKey] = entryVal;
   }
 }
+
+/**
+ * Add ttfb information to measurements
+ *
+ * Exported for tests
+ */
+export function _addTtfbToMeasurements(
+  _measurements: Measurements,
+  responseStartTimestamp: number | undefined,
+  requestStartTimestamp: number | undefined,
+  transactionStartTime: number | undefined,
+): void {
+  // Generate TTFB (Time to First Byte), which measured as the time between the beginning of the transaction and the
+  // start of the response in milliseconds
+  if (typeof responseStartTimestamp === 'number' && transactionStartTime) {
+    DEBUG_BUILD && logger.log('[Measurements] Adding TTFB');
+    _measurements['ttfb'] = {
+      // As per https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/responseStart,
+      // responseStart can be 0 if the request is coming straight from the cache.
+      // This might lead us to calculate a negative ttfb if we don't use Math.max here.
+      //
+      // This logic is the same as what is in the web-vitals library to calculate ttfb
+      // https://github.com/GoogleChrome/web-vitals/blob/2301de5015e82b09925238a228a0893635854587/src/onTTFB.ts#L92
+      // TODO(abhi): We should use the web-vitals library instead of this custom calculation.
+      value: Math.max(responseStartTimestamp - transactionStartTime, 0) * 1000,
+      unit: 'millisecond',
+    };
+
+    if (typeof requestStartTimestamp === 'number' && requestStartTimestamp <= responseStartTimestamp) {
+      // Capture the time spent making the request and receiving the first byte of the response.
+      // This is the time between the start of the request and the start of the response in milliseconds.
+      _measurements['ttfb.requestTime'] = {
+        value: (responseStartTimestamp - requestStartTimestamp) * 1000,
+        unit: 'millisecond',
+      };
+    }
+  }
+}
diff --git a/packages/tracing-internal/test/browser/metrics/index.test.ts b/packages/tracing-internal/test/browser/metrics/index.test.ts
index ce6060497ef7..e405760fbcc7 100644
--- a/packages/tracing-internal/test/browser/metrics/index.test.ts
+++ b/packages/tracing-internal/test/browser/metrics/index.test.ts
@@ -1,5 +1,6 @@
 import { Transaction } from '../../../src';
 import type { ResourceEntry } from '../../../src/browser/metrics';
+import { _addTtfbToMeasurements } from '../../../src/browser/metrics';
 import { _addMeasureSpans, _addResourceSpans } from '../../../src/browser/metrics';
 import { WINDOW } from '../../../src/browser/types';
 
@@ -261,6 +262,34 @@ describe('_addResourceSpans', () => {
   });
 });
 
+describe('_addTtfbToMeasurements', () => {
+  it('adds ttfb to measurements', () => {
+    const measurements = {};
+    _addTtfbToMeasurements(measurements, 300, 200, 100);
+    expect(measurements).toEqual({
+      ttfb: {
+        unit: 'millisecond',
+        value: 200000,
+      },
+      'ttfb.requestTime': {
+        unit: 'millisecond',
+        value: 100000,
+      },
+    });
+  });
+
+  it('does not add negative ttfb', () => {
+    const measurements = {};
+    _addTtfbToMeasurements(measurements, 100, 200, 300);
+    expect(measurements).toEqual({
+      ttfb: {
+        unit: 'millisecond',
+        value: 0,
+      },
+    });
+  });
+});
+
 const setGlobalLocation = (location: Location) => {
   // @ts-expect-error need to delete this in order to set to new value
   delete WINDOW.location;