Merge pull request #6470 from Shopify/record-retries-theme-events

Record retryable requests to monorail for theme commands

Author: EvilGenius13

packages/cli-kit/src/private/node/api.test.ts

@@ -1,7 +1,12 @@
 import {retryAwareRequest, isNetworkError, isTransientNetworkError} from './api.js'
+import {recordRetry} from '../../public/node/analytics.js'
 import {ClientError} from 'graphql-request'
 import {describe, test, vi, expect, beforeEach, afterEach} from 'vitest'
 
+vi.mock('../../public/node/analytics.js', () => ({
+  recordRetry: vi.fn(),
+}))
+
 describe('retryAwareRequest', () => {
   beforeEach(() => {
     vi.useFakeTimers()
@@ -374,6 +379,229 @@ describe('retryAwareRequest', () => {
       }),
     )
   })
+
+  test('records retry events when recordRetries is enabled', async () => {
+    const rateLimitedResponse = {
+      status: 200,
+      errors: [
+        {
+          extensions: {
+            code: '429',
+          },
+        } as any,
+      ],
+      headers: new Headers({'retry-after': '100'}),
+    }
+
+    const successResponse = {
+      status: 200,
+      data: {hello: 'world!'},
+      headers: new Headers(),
+    }
+
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError(rateLimitedResponse, {query: ''})
+      })
+      .mockImplementation(() => {
+        return Promise.resolve(successResponse)
+      })
+
+    const mockScheduleDelayFn = vi.fn((fn) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/api',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+        recordCommandRetries: true,
+      },
+      undefined,
+      {
+        scheduleDelay: mockScheduleDelayFn,
+      },
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {hello: 'world!'},
+    })
+
+    expect(recordRetry).toHaveBeenCalledTimes(1)
+    expect(recordRetry).toHaveBeenCalledWith('https://themes.example.com/api', 'http-retry-1:can-retry:')
+  })
+
+  test('does not record retry events when recordRetries is disabled', async () => {
+    const rateLimitedResponse = {
+      status: 200,
+      errors: [
+        {
+          extensions: {
+            code: '429',
+          },
+        } as any,
+      ],
+      headers: new Headers(),
+    }
+
+    const successResponse = {
+      status: 200,
+      data: {hello: 'world!'},
+      headers: new Headers(),
+    }
+
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError(rateLimitedResponse, {query: ''})
+      })
+      .mockImplementation(() => {
+        return Promise.resolve(successResponse)
+      })
+
+    const mockScheduleDelayFn = vi.fn((fn) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://app.example.com/api',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+        recordCommandRetries: false,
+      },
+      undefined,
+      {
+        scheduleDelay: mockScheduleDelayFn,
+      },
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {hello: 'world!'},
+    })
+
+    expect(recordRetry).not.toHaveBeenCalled()
+  })
+
+  test('records multiple retry events with correct attempt numbers', async () => {
+    const rateLimitedResponse = {
+      status: 200,
+      errors: [
+        {
+          extensions: {
+            code: '429',
+          },
+        } as any,
+      ],
+      headers: new Headers(),
+    }
+
+    const successResponse = {
+      status: 200,
+      data: {success: true},
+      headers: new Headers(),
+    }
+
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError(rateLimitedResponse, {query: ''})
+      })
+      .mockImplementationOnce(() => {
+        throw new ClientError(rateLimitedResponse, {query: ''})
+      })
+      .mockImplementation(() => {
+        return Promise.resolve(successResponse)
+      })
+
+    const mockScheduleDelayFn = vi.fn((fn) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/upload',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+        recordCommandRetries: true,
+      },
+      undefined,
+      {
+        scheduleDelay: mockScheduleDelayFn,
+      },
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {success: true},
+    })
+
+    expect(recordRetry).toHaveBeenCalledTimes(2)
+    expect(recordRetry).toHaveBeenNthCalledWith(1, 'https://themes.example.com/upload', 'http-retry-1:can-retry:')
+    expect(recordRetry).toHaveBeenNthCalledWith(2, 'https://themes.example.com/upload', 'http-retry-2:can-retry:')
+  })
+
+  test('records retry events for too many requests status', async () => {
+    const rateLimitedResponse = {
+      status: 200,
+      errors: [
+        {
+          extensions: {
+            code: '429',
+          },
+        } as any,
+      ],
+      headers: new Headers(),
+    }
+
+    const successResponse = {
+      status: 200,
+      data: {authenticated: true},
+      headers: new Headers(),
+    }
+
+    const mockRequestFn = vi
+      .fn()
+      .mockImplementationOnce(() => {
+        throw new ClientError(rateLimitedResponse, {query: ''})
+      })
+      .mockImplementation(() => {
+        return Promise.resolve(successResponse)
+      })
+
+    const mockScheduleDelayFn = vi.fn((fn) => fn())
+
+    const result = retryAwareRequest(
+      {
+        request: mockRequestFn,
+        url: 'https://themes.example.com/auth',
+        useNetworkLevelRetry: true,
+        maxRetryTimeMs: 10000,
+        recordCommandRetries: true,
+      },
+      undefined,
+      {
+        scheduleDelay: mockScheduleDelayFn,
+      },
+    )
+    await vi.runAllTimersAsync()
+
+    await expect(result).resolves.toEqual({
+      headers: expect.anything(),
+      status: 200,
+      data: {authenticated: true},
+    })
+
+    expect(recordRetry).toHaveBeenCalledTimes(1)
+    expect(recordRetry).toHaveBeenCalledWith('https://themes.example.com/auth', 'http-retry-1:can-retry:')
+  })
 })
 
 describe('isTransientNetworkError', () => {

packages/cli-kit/src/private/node/api.ts

@@ -2,6 +2,7 @@ import {sanitizedHeadersOutput} from './api/headers.js'
 import {sanitizeURL} from './api/urls.js'
 import {sleepWithBackoffUntil} from './sleep-with-backoff.js'
 import {outputDebug} from '../../public/node/output.js'
+import {recordRetry} from '../../public/node/analytics.js'
 import {Headers} from 'form-data'
 import {ClientError} from 'graphql-request'
 import {performance} from 'perf_hooks'
@@ -17,9 +18,11 @@ export type NetworkRetryBehaviour =
   | {
       useNetworkLevelRetry: true
       maxRetryTimeMs: number
+      recordCommandRetries?: boolean
     }
   | {
       useNetworkLevelRetry: false
+      recordCommandRetries?: boolean
     }
 
 type RequestOptions<T> = {
@@ -149,6 +152,12 @@ async function runRequestWithNetworkLevelRetry<T extends {headers: Headers; stat
       if (!isTransientNetworkError(err)) {
         throw err
       }
+
+      // Record command retries
+      if (requestOptions.recordCommandRetries) {
+        recordRetry(requestOptions.url, `network-retry:${(err as Error).message}`)
+      }
+
       outputDebug(`Retrying request to ${requestOptions.url} due to network error ${err}`)
     }
   }
@@ -371,6 +380,11 @@ ${result.sanitizedHeaders}
     }
     retriesUsed += 1
 
+    // Record command retries
+    if (requestOptions.recordCommandRetries) {
+      recordRetry(requestOptions.url, `http-retry-${retriesUsed}:${result.status}:`)
+    }
+
     // prefer to wait based on a header if given; the caller's preference if not; and a default if neither.
     const retryDelayMs = result.delayMs ?? retryOptions.defaultDelayMs ?? DEFAULT_RETRY_DELAY_MS
     outputDebug(`Scheduling retry request #${retriesUsed} to ${result.sanitizedUrl} in ${retryDelayMs} ms`)

packages/cli-kit/src/public/node/themes/api.ts

@@ -37,6 +37,7 @@ const THEME_API_NETWORK_BEHAVIOUR: RequestModeInput = {
   useNetworkLevelRetry: true,
   useAbortSignal: false,
   maxRetryTimeMs: 90 * 1000,
+  recordCommandRetries: true,
 }
 
 export async function fetchTheme(id: number, session: AdminSession): Promise<Theme | undefined> {