Merge pull request #7478 from Shopify/dlm-store-command-metrics-followup

Avoid metrics-only store analytics lookups

Author: dmerand

packages/cli-kit/src/public/node/metadata.ts

@@ -177,8 +177,7 @@ export function createRuntimeMetadataContainer<
 
 // We want to track anything that ends up getting sent to monorail as `cmd_all_*`,
 // `cmd_app_*`, `cmd_theme_*`, `store_*`, and `env_auto_upgrade_*`
-type CmdFieldsFromMonorail = Pick<MonorailEventPublic, 'shop_id'> &
-  PickByPrefix<MonorailEventPublic, 'cmd_all_'> &
+type CmdFieldsFromMonorail = PickByPrefix<MonorailEventPublic, 'cmd_all_'> &
   PickByPrefix<MonorailEventPublic, 'cmd_app_'> &
   PickByPrefix<MonorailEventPublic, 'cmd_create_app_'> &
   PickByPrefix<MonorailEventPublic, 'cmd_theme_'> &

packages/cli-kit/src/public/node/monorail.test.ts

@@ -1,5 +1,5 @@
 import * as http from './http.js'
-import {publishMonorailEvent} from './monorail.js'
+import {MONORAIL_COMMAND_TOPIC, publishMonorailEvent} from './monorail.js'
 import {mockAndCaptureOutput} from './testing/output.js'
 import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
 
@@ -52,4 +52,59 @@ describe('monorail', () => {
     expect(outputMock.debug()).toContain('"api_key": "****"')
     expect(outputMock.debug()).not.toContain('some-api-key')
   })
+
+  test('builds a request for the real command topic including validated store attribution', async () => {
+    const res = await publishMonorailEvent(
+      MONORAIL_COMMAND_TOPIC,
+      {
+        command: 'shopify store auth',
+        time_start: 1643709600000,
+        time_end: 1643709601000,
+        total_time: 1000,
+        success: true,
+        cli_version: '3.94.0',
+        uname: 'Darwin test',
+        ruby_version: '3.3.0',
+        node_version: '22.0.0',
+        is_employee: false,
+        user_id: '42',
+        store_fqdn_hash: 'hashed-store',
+        store_fqdn_validated: true,
+      },
+      {
+        args: '--store shop.myshopify.com',
+        store_fqdn: 'shop.myshopify.com',
+      },
+    )
+
+    expect(res.type).toEqual('ok')
+    expect(http.fetch).toHaveBeenCalledWith(
+      expectedURL,
+      {
+        method: 'POST',
+        body: JSON.stringify({
+          schema_id: MONORAIL_COMMAND_TOPIC,
+          payload: {
+            command: 'shopify store auth',
+            time_start: 1643709600000,
+            time_end: 1643709601000,
+            total_time: 1000,
+            success: true,
+            cli_version: '3.94.0',
+            uname: 'Darwin test',
+            ruby_version: '3.3.0',
+            node_version: '22.0.0',
+            is_employee: false,
+            user_id: '42',
+            store_fqdn_hash: 'hashed-store',
+            store_fqdn_validated: true,
+            args: '--store shop.myshopify.com',
+            store_fqdn: 'shop.myshopify.com',
+          },
+        }),
+        headers: expectedHeaders,
+      },
+      'non-blocking',
+    )
+  })
 })

packages/cli-kit/src/public/node/monorail.ts

@@ -10,7 +10,7 @@ const url = 'https://monorail-edge.shopifysvc.com/v1/produce'
 type Optional<T> = T | null
 
 // This is the topic name of the main event we log to Monorail, the command tracker
-export const MONORAIL_COMMAND_TOPIC = 'app_cli3_command/1.23'
+export const MONORAIL_COMMAND_TOPIC = 'app_cli3_command/1.24'
 
 export interface Schemas {
   [MONORAIL_COMMAND_TOPIC]: {
@@ -45,7 +45,7 @@ export interface Schemas {
       node_version: string
       is_employee: boolean
       store_fqdn_hash?: Optional<string>
-      shop_id?: Optional<number>
+      store_fqdn_validated?: Optional<boolean>
       user_id: string
 
       // Any and all commands

packages/store/src/cli/commands/store/auth.test.ts

@@ -4,7 +4,7 @@ import {createStoreAuthPresenter} from '../../services/store/auth/result.js'
 import {describe, expect, test, vi} from 'vitest'
 
 vi.mock('../../services/store/auth/index.js')
-vi.mock('../../services/store/metrics.js')
+vi.mock('../../services/store/attribution.js')
 vi.mock('../../services/store/auth/result.js', () => ({
   createStoreAuthPresenter: vi.fn((format: 'text' | 'json') => ({format})),
 }))

packages/store/src/cli/commands/store/execute.test.ts

@@ -5,7 +5,7 @@ import {beforeEach, describe, expect, test, vi} from 'vitest'
 
 vi.mock('../../services/store/execute/index.js')
 vi.mock('../../services/store/execute/result.js')
-vi.mock('../../services/store/metrics.js')
+vi.mock('../../services/store/attribution.js')
 
 describe('store execute command', () => {
   beforeEach(() => {

packages/store/src/cli/services/store/attribution.test.ts

@@ -0,0 +1,27 @@
+import {recordStoreFqdnMetadata} from './attribution.js'
+import {hashString} from '@shopify/cli-kit/node/crypto'
+import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata'
+import {beforeEach, describe, expect, test, vi} from 'vitest'
+
+vi.mock('@shopify/cli-kit/node/crypto')
+vi.mock('@shopify/cli-kit/node/metadata')
+
+describe('store command attribution', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    vi.mocked(hashString).mockReturnValue('hashed-store')
+  })
+
+  test('records the sensitive, hashed, and validation state for a store fqdn', async () => {
+    await recordStoreFqdnMetadata('shop.myshopify.com', true)
+
+    expect(addSensitiveMetadata).toHaveBeenCalledWith(expect.any(Function))
+    expect(vi.mocked(addSensitiveMetadata).mock.calls[0]![0]()).toEqual({store_fqdn: 'shop.myshopify.com'})
+    expect(addPublicMetadata).toHaveBeenCalledWith(expect.any(Function))
+    expect(vi.mocked(addPublicMetadata).mock.calls[0]![0]()).toEqual({
+      store_fqdn_hash: 'hashed-store',
+      store_fqdn_validated: true,
+    })
+    expect(hashString).toHaveBeenCalledWith('shop.myshopify.com')
+  })
+})

packages/store/src/cli/services/store/attribution.ts

@@ -0,0 +1,7 @@
+import {hashString} from '@shopify/cli-kit/node/crypto'
+import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata'
+
+export async function recordStoreFqdnMetadata(storeFqdn: string, validated: boolean): Promise<void> {
+  await addSensitiveMetadata(() => ({store_fqdn: storeFqdn}))
+  await addPublicMetadata(() => ({store_fqdn_hash: hashString(storeFqdn), store_fqdn_validated: validated}))
+}

packages/store/src/cli/services/store/auth/index.test.ts

@@ -1,12 +1,12 @@
 import {authenticateStoreWithApp} from './index.js'
 import {setStoredStoreAppSession} from './session-store.js'
 import {STORE_AUTH_APP_CLIENT_ID} from './config.js'
-import {recordStoreCommandShopIdFromAdminApi} from '../metrics.js'
+import {recordStoreFqdnMetadata} from '../attribution.js'
 import {setLastSeenUserId} from '@shopify/cli-kit/node/session'
 import {describe, expect, test, vi} from 'vitest'
 
 vi.mock('./session-store.js')
-vi.mock('../metrics.js')
+vi.mock('../attribution.js')
 vi.mock('@shopify/cli-kit/node/session')
 vi.mock('@shopify/cli-kit/node/system', () => ({openURL: vi.fn().mockResolvedValue(true)}))
 vi.mock('@shopify/cli-kit/node/crypto', () => ({randomUUID: vi.fn().mockReturnValue('state-123')}))
@@ -56,8 +56,9 @@ describe('store auth service', () => {
       }),
     )
     expect(presenter.success).toHaveBeenCalledWith(result)
+    expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, 'shop.myshopify.com', false)
+    expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'shop.myshopify.com', true)
     expect(setLastSeenUserId).toHaveBeenCalledWith('42')
-    expect(recordStoreCommandShopIdFromAdminApi).toHaveBeenCalledWith({store: 'shop.myshopify.com', accessToken: 'token'})
 
     const storedSession = vi.mocked(setStoredStoreAppSession).mock.calls[0]![0]
     expect(storedSession.store).toBe('shop.myshopify.com')
@@ -271,6 +272,123 @@ describe('store auth service', () => {
     expect(presenter.success).toHaveBeenCalledWith(result)
   })
 
+  test('authenticateStoreWithApp records fqdn metadata before resolving existing scopes', async () => {
+    await expect(
+      authenticateStoreWithApp(
+        {
+          store: 'shop.myshopify.com',
+          scopes: 'read_products',
+        },
+        {
+          resolveExistingScopes: vi.fn().mockRejectedValue(new Error('scope lookup failed')),
+          presenter: {
+            openingBrowser: vi.fn(),
+            manualAuthUrl: vi.fn(),
+            success: vi.fn(),
+          },
+        },
+      ),
+    ).rejects.toThrow('scope lookup failed')
+
+    expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
+    expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true)
+    expect(setLastSeenUserId).not.toHaveBeenCalled()
+    expect(setStoredStoreAppSession).not.toHaveBeenCalled()
+  })
+
+  test('authenticateStoreWithApp records fqdn metadata before waiting for the auth callback', async () => {
+    const waitForStoreAuthCodeMock = vi.fn().mockRejectedValue(new Error('callback failed'))
+
+    await expect(
+      authenticateStoreWithApp(
+        {
+          store: 'shop.myshopify.com',
+          scopes: 'read_products',
+        },
+        {
+          openURL: vi.fn().mockResolvedValue(true),
+          waitForStoreAuthCode: waitForStoreAuthCodeMock,
+          exchangeStoreAuthCodeForToken: vi.fn(),
+          presenter: {
+            openingBrowser: vi.fn(),
+            manualAuthUrl: vi.fn(),
+            success: vi.fn(),
+          },
+        },
+      ),
+    ).rejects.toThrow('callback failed')
+
+    expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
+    expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true)
+    expect(setStoredStoreAppSession).not.toHaveBeenCalled()
+  })
+
+  test('authenticateStoreWithApp does not mark the store validated when token exchange fails', async () => {
+    const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => {
+      await options.onListening?.()
+      return 'abc123'
+    })
+
+    await expect(
+      authenticateStoreWithApp(
+        {
+          store: 'shop.myshopify.com',
+          scopes: 'read_products',
+        },
+        {
+          openURL: vi.fn().mockResolvedValue(true),
+          waitForStoreAuthCode: waitForStoreAuthCodeMock,
+          exchangeStoreAuthCodeForToken: vi.fn().mockRejectedValue(new Error('token exchange failed')),
+          presenter: {
+            openingBrowser: vi.fn(),
+            manualAuthUrl: vi.fn(),
+            success: vi.fn(),
+          },
+        },
+      ),
+    ).rejects.toThrow('token exchange failed')
+
+    expect(recordStoreFqdnMetadata).toHaveBeenCalledWith('shop.myshopify.com', false)
+    expect(recordStoreFqdnMetadata).not.toHaveBeenCalledWith('shop.myshopify.com', true)
+    expect(setLastSeenUserId).not.toHaveBeenCalled()
+    expect(setStoredStoreAppSession).not.toHaveBeenCalled()
+  })
+
+  test('authenticateStoreWithApp marks the store validated after token exchange but before rejecting missing associated user data', async () => {
+    const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => {
+      await options.onListening?.()
+      return 'abc123'
+    })
+
+    await expect(
+      authenticateStoreWithApp(
+        {
+          store: 'shop.myshopify.com',
+          scopes: 'read_products',
+        },
+        {
+          openURL: vi.fn().mockResolvedValue(true),
+          waitForStoreAuthCode: waitForStoreAuthCodeMock,
+          exchangeStoreAuthCodeForToken: vi.fn().mockResolvedValue({
+            access_token: 'token',
+            scope: 'read_products',
+            expires_in: 86400,
+          }),
+          presenter: {
+            openingBrowser: vi.fn(),
+            manualAuthUrl: vi.fn(),
+            success: vi.fn(),
+          },
+        },
+      ),
+    ).rejects.toThrow('Shopify did not return associated user information for the online access token.')
+
+    expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(1, 'shop.myshopify.com', false)
+    expect(recordStoreFqdnMetadata).toHaveBeenNthCalledWith(2, 'shop.myshopify.com', true)
+    expect(setLastSeenUserId).not.toHaveBeenCalled()
+    expect(setStoredStoreAppSession).not.toHaveBeenCalled()
+  })
+
   test('authenticateStoreWithApp rejects when Shopify grants fewer scopes than requested', async () => {
     const waitForStoreAuthCodeMock = vi.fn().mockImplementation(async (options) => {
       await options.onListening?.()

packages/store/src/cli/services/store/auth/index.ts

@@ -6,7 +6,7 @@ import {createPkceBootstrap} from './pkce.js'
 import {mergeRequestedAndStoredScopes, parseStoreAuthScopes, resolveGrantedScopes} from './scopes.js'
 import {resolveExistingStoreAuthScopes, type ResolvedStoreAuthScopes} from './existing-scopes.js'
 import {createStoreAuthPresenter, type StoreAuthPresenter, type StoreAuthResult} from './result.js'
-import {recordStoreCommandShopIdFromAdminApi} from '../metrics.js'
+import {recordStoreFqdnMetadata} from '../attribution.js'
 import {setLastSeenUserId} from '@shopify/cli-kit/node/session'
 import {openURL} from '@shopify/cli-kit/node/system'
 import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output'
@@ -40,6 +40,7 @@ export async function authenticateStoreWithApp(
 ): Promise<StoreAuthResult> {
   const resolvedDependencies: StoreAuthDependencies = {...defaultStoreAuthDependencies, ...dependencies}
   const store = normalizeStoreFqdn(input.store)
+  await recordStoreFqdnMetadata(store, false)
   const requestedScopes = parseStoreAuthScopes(input.scopes)
   const existingScopeResolution = await resolvedDependencies.resolveExistingScopes(store)
   const scopes = mergeRequestedAndStoredScopes(requestedScopes, existingScopeResolution.scopes)
@@ -70,13 +71,13 @@ export async function authenticateStoreWithApp(
     },
   })
   const tokenResponse = await bootstrap.exchangeCodeForToken(code)
+  await recordStoreFqdnMetadata(store, true)
 
   const userId = tokenResponse.associated_user?.id?.toString()
   if (!userId) {
     throw new AbortError('Shopify did not return associated user information for the online access token.')
   }
   setLastSeenUserId(userId)
-  await recordStoreCommandShopIdFromAdminApi({store, accessToken: tokenResponse.access_token})
 
   const now = Date.now()
   const expiresAt = tokenResponse.expires_in ? new Date(now + tokenResponse.expires_in * 1000).toISOString() : undefined