feat: Retry initial connect call

docs/supportability-metrics.md

@@ -440,6 +440,14 @@ EventBuffer/soft_navigations/Dropped/Bytes
 <!-- node type 4 = Meta -->
 * rrweb/node/4/bytes
 
+### Harvester
+<!-- Harvester retried a harvest -->
+* 'Harvester/Retry/Attempted/<feature_name>'
+<!-- Retry failed codes (dynamic) -->
+* 'Harvester/Retry/Failed/<code>'
+<!-- Retry succeeded codes (dynamic) -->
+* 'Harvester/Retry/Succeeded/<code>'
+
 ### Browser Connect Response Metrics
 <!--- HTTP status code of failed browser connect response --->
 * 'Browser/Supportability/BCS/Error/<code>'

src/common/harvest/harvester.js

@@ -18,8 +18,10 @@ import { getSubmitMethod, xhr as xhrMethod, xhrFetch as fetchMethod } from '../u
 import { activatedFeatures } from '../util/feature-flags'
 import { dispatchGlobalEvent } from '../dispatch/global-event'
 
-const RETRY_FAILED = 'Harvester/Retry/Failed/'
-const RETRY_SUCCEEDED = 'Harvester/Retry/Succeeded/'
+const RETRY = 'Harvester/Retry/'
+const RETRY_ATTEMPTED = RETRY + 'Attempted/'
+const RETRY_FAILED = RETRY + 'Failed/'
+const RETRY_SUCCEEDED = RETRY + 'Succeeded/'
 
 export class Harvester {
   #started = false
@@ -62,7 +64,7 @@ export class Harvester {
     if (!submitMethod) return output
 
     const shouldRetryOnFail = !localOpts.isFinalHarvest && submitMethod === xhrMethod // always retry all features harvests except for final
-    output.payload = !localOpts.directSend ? aggregateInst.makeHarvestPayload(shouldRetryOnFail, localOpts) : localOpts.directSend?.payload // features like PVE can define the payload directly, bypassing the makeHarvestPayload logic
+    output.payload = aggregateInst.makeHarvestPayload(shouldRetryOnFail, localOpts)
 
     if (!output.payload) return output
 
@@ -86,7 +88,9 @@ export class Harvester {
      */
     function cbFinished (result) {
       if (aggregateInst.harvestOpts.prevAttemptCode) { // this means we just retried a harvest that last failed
-        handle(SUPPORTABILITY_METRIC_CHANNEL, [(result.retry ? RETRY_FAILED : RETRY_SUCCEEDED) + aggregateInst.harvestOpts.prevAttemptCode], undefined, FEATURE_NAMES.metrics, aggregateInst.ee)
+        const reportSM = (message) => handle(SUPPORTABILITY_METRIC_CHANNEL, [message], undefined, FEATURE_NAMES.metrics, aggregateInst.ee)
+        reportSM(RETRY_ATTEMPTED + aggregateInst.featureName)
+        reportSM((result.retry ? RETRY_FAILED : RETRY_SUCCEEDED) + aggregateInst.harvestOpts.prevAttemptCode)
         delete aggregateInst.harvestOpts.prevAttemptCode // always reset last observation so we don't falsely report again next harvest
         // In case this re-attempt failed again, that'll be handled (re-marked again) next.
       }
@@ -155,8 +159,7 @@ export function send (agentRef, { endpoint, payload, localOpts = {}, submitMetho
       result.addEventListener('loadend', function () {
         // `this` here in block refers to the XHR object in this scope, do not change the anon function to an arrow function
         // status 0 refers to a local error, such as CORS or network failure, or a blocked request by the browser (e.g. adblocker)
-        const cbResult = { sent: this.status !== 0, status: this.status, retry: shouldRetry(this.status), fullUrl, xhr: this }
-        if (localOpts.needResponse) cbResult.responseText = this.responseText
+        const cbResult = { sent: this.status !== 0, status: this.status, retry: shouldRetry(this.status), fullUrl, xhr: this, responseText: this.responseText }
         cbFinished(cbResult)
 
         /** temporary audit of consistency of harvest metadata flags */
@@ -165,8 +168,7 @@ export function send (agentRef, { endpoint, payload, localOpts = {}, submitMetho
     } else if (submitMethod === fetchMethod) {
       result.then(async function (response) {
         const status = response.status
-        const cbResult = { sent: true, status, retry: shouldRetry(status), fullUrl, fetchResponse: response }
-        if (localOpts.needResponse) cbResult.responseText = await response.text()
+        const cbResult = { sent: true, status, retry: shouldRetry(status), fullUrl, fetchResponse: response, responseText: await response.text() }
         cbFinished(cbResult)
         /** temporary audit of consistency of harvest metadata flags */
         if (!shouldRetry(status)) trackHarvestMetadata()

src/common/harvest/types.js

@@ -24,7 +24,6 @@
  * @property {HarvestEndpointIdentifier} endpoint The endpoint to use (jserrors, events, resources etc.)
  * @property {HarvestPayload} payload Object representing payload.
  * @property {object} localOpts Additional options for sending data
- * @property {boolean} localOpts.needResponse Specify whether the caller expects a response data.
  * @property {boolean} localOpts.isFinalHarvest Specify whether the call is a final harvest during page unload.
  * @property {boolean} localOpts.sendEmptyBody Specify whether the call should be made even if the body is empty. Useful for rum calls.
  * @property {boolean} localOpts.forceNoRetry Don't save the buffered data in the case of a need to retry the transmission.

src/features/page_view_event/aggregate/index.js

@@ -29,6 +29,7 @@ export class Aggregate extends AggregateBase {
     this.timeToFirstByte = 0
     this.firstByteToWindowLoad = 0 // our "frontend" duration
     this.firstByteToDomContent = 0 // our "dom processing" duration
+    this.retries = 0
 
     if (!isValid(agentRef.info)) {
       this.ee.abort()
@@ -55,9 +56,8 @@ export class Aggregate extends AggregateBase {
    *
    * @param {Function} cb A function to run once the RUM call has finished - Defaults to activateFeatures
    * @param {*} customAttributes custom attributes to attach to the RUM call - Defaults to info.js
-   * @param {*} target The target to harvest to
    */
-  sendRum (customAttributes = this.agentRef.info.jsAttributes, target = { licenseKey: this.agentRef.info.licenseKey, applicationID: this.agentRef.info.applicationID }) {
+  sendRum (customAttributes = this.agentRef.info.jsAttributes) {
     const info = this.agentRef.info
     const measures = {}
 
@@ -110,24 +110,26 @@ export class Aggregate extends AggregateBase {
     queryParameters.fp = firstPaint.current.value
     queryParameters.fcp = firstContentfulPaint.current.value
 
-    const timeKeeper = this.agentRef.runtime.timeKeeper
-    if (timeKeeper?.ready) {
-      queryParameters.timestamp = Math.floor(timeKeeper.correctRelativeTimestamp(now()))
+    this.queryStringsBuilder = () => { // this will be called by AggregateBase.makeHarvestPayload every time harvest is triggered to be qs
+      this.rumStartTime = now() // this should be reset at the beginning of each RUM call for proper timeKeeper calculation in coordination with postHarvestCleanup
+      const timeKeeper = this.agentRef.runtime.timeKeeper
+      if (timeKeeper?.ready) {
+        queryParameters.timestamp = Math.floor(timeKeeper.correctRelativeTimestamp(this.rumStartTime))
+      }
+      return queryParameters
     }
-
-    this.rumStartTime = now()
+    this.events.add(body)
 
     this.agentRef.runtime.harvester.triggerHarvestFor(this, {
-      directSend: {
-        target,
-        payload: { qs: queryParameters, body }
-      },
-      needResponse: true,
       sendEmptyBody: true
     })
   }
 
-  postHarvestCleanup ({ status, responseText, xhr }) {
+  serializer (eventBuffer) { // this is necessary because PVE sends a single item rather than an array; in the case of undefined, this prevents sending [null] as body
+    return eventBuffer[0]
+  }
+
+  postHarvestCleanup ({ sent, status, responseText, xhr, retry }) {
     const rumEndTime = now()
     let app, flags
     try {
@@ -137,8 +139,16 @@ export class Aggregate extends AggregateBase {
       warn(53, error)
     }
 
+    super.postHarvestCleanup({ sent, retry }) // this will set isRetrying & re-buffer the body if request is to be retried
+    if (this.isRetrying && this.retries++ < 1) { // Only retry once
+      setTimeout(() => this.agentRef.runtime.harvester.triggerHarvestFor(this, {
+        sendEmptyBody: true
+      }), 5000) // Retry sending the RUM event after 5 seconds
+      return
+    }
     if (status >= 400 || status === 0) {
       warn(18, status)
+      this.blocked = true
 
       // Get estimated payload size of our backlog
       const textEncoder = new TextEncoder()
@@ -205,6 +215,7 @@ export class Aggregate extends AggregateBase {
       }
     } catch (error) {
       this.ee.abort()
+      this.blocked = true
       warn(17, error)
       return
     }

tests/components/page_view_event/aggregate.test.js

@@ -21,11 +21,8 @@ test('PageViewEvent does not throw on Harvester driven processes', () => {
   expect(mainAgent.runtime.harvester.triggerHarvestFor(pveAggregate).ranSend).toEqual(false) // mimics what the harvester does on interval
   expect(mainAgent.runtime.harvester.triggerHarvestFor(pveAggregate, { isFinalHarvest: true }).ranSend).toEqual(false) // mimics what the harvester does on EoL
 
+  pveAggregate.events.add(undefined) // ensure request even if sendRum() puts empty body into buffer
   expect(mainAgent.runtime.harvester.triggerHarvestFor(pveAggregate, {
-    directSend: {
-      payload: 'blah'
-    },
-    needResponse: true,
     sendEmptyBody: true
   }).ranSend).toEqual(true) // mimics the manual trigger in PVE `sendRum`; this should return true as it actually tries to "send"
 })

tests/components/soft_navigations/aggregate.test.js

@@ -272,7 +272,9 @@ describe('popstate interactions', () => {
   test('do become route change if first interaction on page', () => {
     const origUrl = window.location.href
     const newUrl = 'http://myurl.com'
-    window.location.href = newUrl // location is normally updated by the time popstate event occurs
+    // Because JSDOM doesn't support modifying the location directly, we need to do this:
+    delete window.location
+    window.location = new URL(newUrl) // location is normally updated by the time popstate event occurs
 
     softNavAggregate.ee.emit('newUIEvent', [{ type: POPSTATE_TRIGGER, timeStamp: 100 }])
     const ixn = softNavAggregate.interactionInProgress

tests/specs/rum/retry-harvesting.e2e.js

@@ -26,10 +26,11 @@ describe('rum retry harvesting', () => {
   })
 
   afterEach(async () => {
+    await browser.testHandle.clearScheduledReplies('bamServer')
     await browser.destroyAgentSession(browser.testHandle)
   })
 
-  ;[400, 404, 408, 429, 500, 502, 503, 504, 512].forEach(statusCode => {
+  ;[400, 404].forEach(statusCode => {
     it(`should not retry rum and should not continue harvesting when request statusCode is ${statusCode}`, async () => {
       await browser.testHandle.scheduleReply('bamServer', {
         test: testRumRequest,
@@ -82,45 +83,30 @@ describe('rum retry harvesting', () => {
       expect(traceHarvests.length).toEqual(0)
       expect(errorMetricsHarvests.length).toEqual(0)
       expect(replaysHarvests.length).toEqual(0)
+    })
+  })
 
-      ;[
-        rumHarvests,
-        timingEventsHarvests,
-        ajaxEventsHarvests,
-        ajaxMetricsHarvests,
-        insightsHarvests,
-        interactionEventsHarvests,
-        traceHarvests,
-        errorMetricsHarvests,
-        replaysHarvests
-      ] = await Promise.all([
-        rumCapture.waitForResult({ timeout: 10000 }),
-        timingEventsCapture.waitForResult({ timeout: 10000 }),
-        ajaxEventsCapture.waitForResult({ timeout: 10000 }),
-        ajaxMetricsCapture.waitForResult({ timeout: 10000 }),
-        insightsCapture.waitForResult({ timeout: 10000 }),
-        interactionEventsCapture.waitForResult({ timeout: 10000 }),
-        traceCapture.waitForResult({ timeout: 10000 }),
-        errorMetricsCapture.waitForResult({ timeout: 10000 }),
-        replaysCapture.waitForResult({ timeout: 10000 }),
-        browser.url(await browser.testHandle.assetURL('/'))
-      ])
-
-      if (statusCode === 408) {
-        // Browsers automatically retry requests with status code 408
-        expect(rumHarvests.length).toBeLessThan(5)
-      } else {
-        expect(rumHarvests.length).toEqual(1)
-      }
+  ;[408, 429, 500, 502, 503, 504, 512].forEach(statusCode => {
+    it(`should retry rum and subsequent features should harvest when request statusCode is ${statusCode}`, async () => {
+      await browser.testHandle.scheduleReply('bamServer', {
+        test: testRumRequest,
+        permanent: false, // should only fail the first time
+        statusCode,
+        body: ''
+      })
 
-      expect(timingEventsHarvests.length).toEqual(0)
-      expect(ajaxEventsHarvests.length).toEqual(0)
-      expect(ajaxMetricsHarvests.length).toEqual(0)
-      expect(insightsHarvests.length).toEqual(0)
-      expect(interactionEventsHarvests.length).toEqual(0)
-      expect(traceHarvests.length).toEqual(0)
-      expect(errorMetricsHarvests.length).toEqual(0)
-      expect(replaysHarvests.length).toEqual(0)
+      const [[rumHarvest1, rumHarvest2]] = await Promise.all([
+        rumCapture.waitForResult({ totalCount: 2 }), // retry happens in 5 seconds after failure
+        timingEventsCapture.waitForResult({ totalCount: 1 }), // a subsequent feature should then still harvest here after first retry
+        browser.url(await browser.testHandle.assetURL('obfuscate-pii.html'))
+      ])
+      const { rst: rst1, ...query1 } = rumHarvest1.request.query
+      const { rst: rst2, ...query2 } = rumHarvest2.request.query
+      expect(query1).toEqual(query2)
+      expect(Number(rst2)).toBeGreaterThanOrEqual(Number(rst1))
+      expect(rumHarvest1.request.body).toEqual(rumHarvest2.request.body)
+      expect(rumHarvest1.reply.statusCode).toEqual(statusCode)
+      expect(rumHarvest2.reply.statusCode).toEqual(200)
     })
   })
 })

tests/specs/rum/supportability-metrics.e2e.js

@@ -15,7 +15,8 @@ describe('basic pve capturing', () => {
     // will reply with http status 500 to fake error response from browser connect service
     await browser.testHandle.scheduleReply('bamServer', {
       test: testRumRequest,
-      statusCode: 500
+      statusCode: 500,
+      permanent: true
     })
 
     // visit the webpage, not waiting for agent load since we don't expect the rum feature to load properly

tests/unit/common/harvest/harvester.test.js

@@ -125,20 +125,11 @@ describe('triggerHarvestFor', () => {
   test('fails if aggregate is blocked', () => {
     expect(harvester.triggerHarvestFor({ blocked: true })).toEqual({ payload: undefined, ranSend: false, endpointVersion: 1 })
   })
-  test('does nothing if no payload is returned from makeHarvestPayload if directSend unspecified', () => {
+  test('does nothing if no payload is returned from makeHarvestPayload (without sendEmptyBody)', () => {
     const fakeAggregate = { makeHarvestPayload: jest.fn(), agentRef: fakeAgent }
-    expect(harvester.triggerHarvestFor(fakeAggregate, { directSend: undefined })).toEqual({ payload: undefined, ranSend: false, endpointVersion: 1 })
+    expect(harvester.triggerHarvestFor(fakeAggregate)).toEqual({ payload: undefined, ranSend: false, endpointVersion: 1 })
     expect(fakeAggregate.makeHarvestPayload).toHaveBeenCalledTimes(1)
   })
-  test('allows directSend to provide the payload without makeHarvestPayload', () => {
-    const fakeAggregate = { makeHarvestPayload: jest.fn(), harvestOpts: {}, agentRef: fakeAgent }
-    expect(harvester.triggerHarvestFor(fakeAggregate, { directSend: { payload: 'fakePayload' } })).toEqual({ payload: 'fakePayload', ranSend: true, endpointVersion: 1 })
-    expect(fakeAggregate.makeHarvestPayload).not.toHaveBeenCalled()
-  })
-  test('disallow directSend to send if no payload is defined', () => {
-    const fakeAggregate = { makeHarvestPayload: jest.fn(), harvestOpts: {}, agentRef: fakeAgent }
-    expect(harvester.triggerHarvestFor(fakeAggregate, { directSend: { payload: undefined } })).toEqual({ payload: undefined, ranSend: false, endpointVersion: 1 })
-  })
   test('sends if payload is returned from makeHarvestPayload', () => {
     const fakeAggregate = { makeHarvestPayload: jest.fn().mockReturnValue('fakePayload'), harvestOpts: {}, agentRef: fakeAgent }
     expect(harvester.triggerHarvestFor(fakeAggregate, { })).toEqual({ payload: 'fakePayload', ranSend: true, endpointVersion: 1 })