Prevent theme telemetry failures from replacing command results

Author: gonzaloriestra

packages/theme/src/cli/commands/theme/transfer-environments.test.ts

@@ -4,6 +4,7 @@ import Share from './share.js'
 import {executeThemePush} from '../../services/push.js'
 import {executeThemePull} from '../../services/pull.js'
 import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output'
+import {addPublicMetadata} from '@shopify/cli-kit/node/metadata'
 import {loadEnvironment} from '@shopify/cli-kit/node/environments'
 import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
@@ -20,17 +21,15 @@ vi.mock('@shopify/cli-kit/node/metadata')
 class TestPush extends Push {
   public parse = vi.fn()
 }
-
 class TestPull extends Pull {
   public parse = vi.fn()
 }
-
 class TestShare extends Share {
   public parse = vi.fn()
 }
 
 describe.each([TestPush, TestPull, TestShare])('%s', (Command) => {
-  test.each(['none', 'partial', 'total', 'cancelled'] as const)(
+  test.each(['none', 'partial', 'total', 'cancelled', 'analytics'] as const)(
     'retains all environments in request order with %s failures',
     async (failures) => {
       await inTemporaryDirectory(async (path) => {
@@ -45,6 +44,11 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => {
         const secondStarted = new Promise<void>((resolve) => {
           releaseFirst = resolve
         })
+        vi.mocked(addPublicMetadata).mockImplementation(async (collect) => {
+          const metadata = await collect()
+          if (failures === 'analytics' && metadata?.store_domain === 'second.myshopify.com')
+            throw new Error('analytics failed')
+        })
         const executionOrder: string[] = []
         const execute = async (flags: {environment?: string[]}, session?: {storeFqdn: string}) => {
           const environment = flags.environment![0]!
@@ -73,7 +77,6 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => {
         }
         vi.mocked(executeThemePush).mockImplementation(execute)
         vi.mocked(executeThemePull).mockImplementation(execute)
-
         const config = new Config({root: __dirname})
         await config.load()
         const command = new Command(
@@ -114,7 +117,7 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => {
                   .map((line) => JSON.parse(line))
               : []
             expect(events.filter((event) => event.level === 'error')).toHaveLength(
-              {none: 0, partial: 1, total: 3, cancelled: 0}[failures],
+              {none: 0, partial: 1, total: 3, cancelled: 0, analytics: 0}[failures],
             )
           })
         } finally {
@@ -151,7 +154,6 @@ describe.each([TestPush, TestPull, TestShare])('%s', (Command) => {
         ])
         expect(executeThemePush).not.toHaveBeenCalled()
         expect(executeThemePull).not.toHaveBeenCalled()
-
         expect(process.exitCode).toBe(1)
       })
     } finally {

packages/theme/src/cli/utilities/theme-command-analytics.test.ts

@@ -0,0 +1,51 @@
+import ThemeCommand from './theme-command.js'
+import {Config, Flags} from '@oclif/core'
+import {compileData} from '@shopify/cli-kit/node/analytics'
+import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
+import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata'
+import {expect, test, vi} from 'vitest'
+
+vi.mock('@shopify/cli-kit/node/analytics')
+vi.mock('@shopify/cli-kit/node/session')
+vi.mock('@shopify/cli-kit/node/metadata')
+
+test.each(
+  ['compile', 'public metadata', 'sensitive metadata'].flatMap((failure) =>
+    [false, true].map((commandFails) => ({failure, commandFails})),
+  ),
+)('preserves the command outcome when $failure fails, commandFails=$commandFails', async ({failure, commandFails}) => {
+  const commandError = new Error('Command failed')
+  const telemetryError = new Error('Telemetry failed')
+  class TestCommand extends ThemeCommand {
+    static flags = {
+      store: Flags.string({env: 'SHOPIFY_FLAG_STORE'}),
+      password: Flags.string({env: 'SHOPIFY_FLAG_PASSWORD'}),
+    }
+
+    static multiEnvironmentsFlags = ['store']
+
+    command = vi.fn(async () => {
+      if (commandFails) throw commandError
+    })
+  }
+
+  vi.mocked(ensureAuthenticatedThemes).mockResolvedValue({storeFqdn: 'example.myshopify.com', token: 'token'})
+  vi.mocked(compileData).mockImplementation(() => {
+    if (failure === 'compile') throw telemetryError
+    return {timings: {}, errors: {}, retries: {}, events: {}} as ReturnType<typeof compileData>
+  })
+  vi.mocked(addPublicMetadata).mockImplementation(async (metadata) => {
+    if (failure === 'public metadata' && 'cmd_theme_timings' in (await metadata())) throw telemetryError
+  })
+  if (failure === 'sensitive metadata') vi.mocked(addSensitiveMetadata).mockRejectedValue(telemetryError)
+  const config = new Config({root: __dirname})
+  await config.load()
+  const command = new TestCommand(['--store', 'example.myshopify.com', '--password', 'token'], config)
+
+  if (commandFails) {
+    await expect(command.run()).rejects.toBe(commandError)
+  } else {
+    await expect(command.run()).resolves.toBeUndefined()
+  }
+  expect(command.command).toHaveBeenCalledOnce()
+})

packages/theme/src/cli/utilities/theme-command.ts

@@ -741,18 +741,24 @@ export default abstract class ThemeCommand extends Command {
   private async logAnalyticsData(session?: AdminSession): Promise<void> {
     if (!session) return
 
-    const data = compileData()
-    await addPublicMetadata(() => ({
-      store_fqdn_hash: hashString(session.storeFqdn),
-      store_domain: session.storeFqdn,
-
-      cmd_theme_timings: JSON.stringify(data.timings),
-      cmd_theme_errors: JSON.stringify(data.errors),
-      cmd_theme_retries: JSON.stringify(data.retries),
-      cmd_theme_events: JSON.stringify(data.events),
-    }))
-    await addSensitiveMetadata(() => ({
-      store_fqdn: session.storeFqdn,
-    }))
+    try {
+      const data = compileData()
+      await addPublicMetadata(() => ({
+        store_fqdn_hash: hashString(session.storeFqdn),
+        store_domain: session.storeFqdn,
+
+        cmd_theme_timings: JSON.stringify(data.timings),
+        cmd_theme_errors: JSON.stringify(data.errors),
+        cmd_theme_retries: JSON.stringify(data.retries),
+        cmd_theme_events: JSON.stringify(data.events),
+      }))
+      await addSensitiveMetadata(() => ({
+        store_fqdn: session.storeFqdn,
+      }))
+      // eslint-disable-next-line no-catch-all/no-catch-all
+    } catch (error) {
+      // Telemetry must not discard completed work or replace the command's original error.
+      outputDebug(`Unable to record theme analytics: ${error instanceof Error ? error.message : String(error)}`)
+    }
   }
 }