diff --git a/packages/cli-kit/src/private/node/request-ids.test.ts b/packages/cli-kit/src/private/node/request-ids.test.ts new file mode 100644 index 00000000000..8cd403ac551 --- /dev/null +++ b/packages/cli-kit/src/private/node/request-ids.test.ts @@ -0,0 +1,68 @@ +import {MAX_REQUEST_IDS, requestIdsCollection} from './request-ids.js' +import {describe, test, expect, beforeEach} from 'vitest' + +describe('RequestIDCollection', () => { + beforeEach(() => { + requestIdsCollection.clear() + }) + + test('starts with an empty collection', () => { + expect(requestIdsCollection.getRequestIds()).toEqual([]) + }) + + test('adds request IDs to collection', () => { + // When + requestIdsCollection.addRequestId('request-1') + requestIdsCollection.addRequestId('request-2') + + // Then + expect(requestIdsCollection.getRequestIds()).toEqual(['request-1', 'request-2']) + }) + + test('ignores undefined or null request IDs', () => { + // When + requestIdsCollection.addRequestId(undefined) + requestIdsCollection.addRequestId(null) + requestIdsCollection.addRequestId('request-1') + + // Then + expect(requestIdsCollection.getRequestIds()).toEqual(['request-1']) + }) + + test('limits collection to MAX_REQUEST_IDS', () => { + // When + for (let i = 0; i < MAX_REQUEST_IDS + 20; i++) { + requestIdsCollection.addRequestId(`request-${i}`) + } + + // Then + expect(requestIdsCollection.getRequestIds()).toHaveLength(100) + expect(requestIdsCollection.getRequestIds()[0]).toBe('request-0') + expect(requestIdsCollection.getRequestIds()[99]).toBe('request-99') + }) + + test('clear() removes all request IDs', () => { + // Given + requestIdsCollection.addRequestId('request-1') + requestIdsCollection.addRequestId('request-2') + + // When + requestIdsCollection.clear() + + // Then + expect(requestIdsCollection.getRequestIds()).toEqual([]) + }) + + test('maintains singleton instance', () => { + // Given + requestIdsCollection.addRequestId('request-1') + + // When + const sameInstance = requestIdsCollection + sameInstance.addRequestId('request-2') + + // Then + expect(requestIdsCollection.getRequestIds()).toEqual(['request-1', 'request-2']) + expect(sameInstance.getRequestIds()).toEqual(['request-1', 'request-2']) + }) +}) diff --git a/packages/cli-kit/src/private/node/request-ids.ts b/packages/cli-kit/src/private/node/request-ids.ts new file mode 100644 index 00000000000..9f19e6ceb77 --- /dev/null +++ b/packages/cli-kit/src/private/node/request-ids.ts @@ -0,0 +1,43 @@ +export const MAX_REQUEST_IDS = 100 + +/** + * Manages collection of request IDs during command execution + */ +class RequestIDCollection { + private static instance: RequestIDCollection + + static getInstance(): RequestIDCollection { + if (!RequestIDCollection.instance) { + RequestIDCollection.instance = new RequestIDCollection() + } + return RequestIDCollection.instance + } + + private requestIds: string[] = [] + + /** + * Add a request ID to the collection + * We only report the first MAX_REQUEST_IDS request IDs. + */ + addRequestId(requestId: string | undefined | null) { + if (requestId && this.requestIds.length < MAX_REQUEST_IDS) { + this.requestIds.push(requestId) + } + } + + /** + * Get all collected request IDs + */ + getRequestIds(): string[] { + return this.requestIds + } + + /** + * Clear all stored request IDs + */ + clear() { + this.requestIds = [] + } +} + +export const requestIdsCollection = RequestIDCollection.getInstance() diff --git a/packages/cli-kit/src/public/node/analytics.ts b/packages/cli-kit/src/public/node/analytics.ts index 701e6117b0e..5fc253c88d0 100644 --- a/packages/cli-kit/src/public/node/analytics.ts +++ b/packages/cli-kit/src/public/node/analytics.ts @@ -10,6 +10,7 @@ import {recordMetrics} from '../../private/node/otel-metrics.js' import {runWithRateLimit} from '../../private/node/conf-store.js' import {reportingRateLimit} from '../../private/node/constants.js' import {getLastSeenUserIdAfterAuth} from '../../private/node/session.js' +import {requestIdsCollection} from '../../private/node/request-ids.js' import {Interfaces} from '@oclif/core' export type CommandExitMode = @@ -154,6 +155,7 @@ async function buildPayload({config, errorMessage, exitMode}: ReportAnalyticsEve cmd_all_timing_active_ms: totalTimeWithoutSubtimers, cmd_all_exit: exitMode, user_id: await getLastSeenUserIdAfterAuth(), + request_ids: requestIdsCollection.getRequestIds(), }, sensitive: { args: startArgs.join(' '), diff --git a/packages/cli-kit/src/public/node/api/graphql.test.ts b/packages/cli-kit/src/public/node/api/graphql.test.ts index 33db86c421e..9a1203d6d0c 100644 --- a/packages/cli-kit/src/public/node/api/graphql.test.ts +++ b/packages/cli-kit/src/public/node/api/graphql.test.ts @@ -1,17 +1,26 @@ import {graphqlRequest, graphqlRequestDoc} from './graphql.js' -import {retryAwareRequest} from '../../../private/node/api.js' +import * as api from '../../../private/node/api.js' import * as debugRequest from '../../../private/node/api/graphql.js' import {buildHeaders} from '../../../private/node/api/headers.js' +import {requestIdsCollection} from '../../../private/node/request-ids.js' +import * as metadata from '../metadata.js' import {GraphQLClient} from 'graphql-request' import {test, vi, describe, expect, beforeEach} from 'vitest' -import {Headers} from 'node-fetch' import {TypedDocumentNode} from '@graphql-typed-document-node/core' -vi.mock('../../../private/node/api.js') +let mockedRequestId = 'request-id-123' + vi.mock('graphql-request', async () => { const actual = await vi.importActual('graphql-request') const client = vi.fn() - client.prototype.rawRequest = vi.fn() + client.prototype.rawRequest = () => { + return { + status: 200, + headers: new Headers({ + 'x-request-id': mockedRequestId, + }), + } + } return { ...(actual as object), @@ -20,20 +29,18 @@ vi.mock('graphql-request', async () => { }) vi.spyOn(debugRequest, 'debugLogRequestInfo').mockResolvedValue(undefined) -const mockedAddress = 'mockedAddress' +const mockedAddress = 'http://localhost:3000' const mockVariables = {some: 'variables'} const mockToken = 'token' const mockedAddedHeaders = {some: 'header'} beforeEach(async () => { - vi.mocked(retryAwareRequest).mockResolvedValue({ - status: 200, - headers: {} as Headers, - }) + requestIdsCollection.clear() }) describe('graphqlRequest', () => { test('calls debugLogRequestInfo once', async () => { + // When await graphqlRequest({ query: 'query', api: 'mockApi', @@ -42,6 +49,8 @@ describe('graphqlRequest', () => { addedHeaders: mockedAddedHeaders, variables: mockVariables, }) + + // Then expect(GraphQLClient).toHaveBeenCalledWith(mockedAddress, { agent: expect.any(Object), headers: { @@ -50,16 +59,44 @@ describe('graphqlRequest', () => { }, }) expect(debugRequest.debugLogRequestInfo).toHaveBeenCalledOnce() - const receivedObject = { - request: expect.any(Function), + }) + + test('Logs the request ids to metadata and requestIdCollection', async () => { + // Given + const metadataSpyOn = vi.spyOn(metadata, 'addPublicMetadata').mockImplementation(async () => {}) + + // When + await graphqlRequest({ + query: 'query', + api: 'mockApi', url: mockedAddress, - } - expect(retryAwareRequest).toHaveBeenCalledWith(receivedObject, expect.any(Function), undefined) + token: mockToken, + addedHeaders: mockedAddedHeaders, + variables: mockVariables, + }) + + mockedRequestId = 'request-id-456' + + await graphqlRequest({ + query: 'query', + api: 'mockApi', + url: mockedAddress, + token: mockToken, + addedHeaders: mockedAddedHeaders, + variables: mockVariables, + }) + + // Then + expect(requestIdsCollection.getRequestIds()).toEqual(['request-id-123', 'request-id-456']) + expect(metadataSpyOn).toHaveBeenCalledTimes(2) + expect(metadataSpyOn.mock.calls[0]![0]()).toEqual({cmd_all_last_graphql_request_id: 'request-id-123'}) + expect(metadataSpyOn.mock.calls[1]![0]()).toEqual({cmd_all_last_graphql_request_id: 'request-id-456'}) }) }) describe('graphqlRequestDoc', () => { test('converts document before querying', async () => { + // Given const document = { kind: 'Document', definitions: [ @@ -80,6 +117,9 @@ describe('graphqlRequestDoc', () => { ], } as unknown as TypedDocumentNode + const retryAwareSpy = vi.spyOn(api, 'retryAwareRequest') + + // When await graphqlRequestDoc({ query: document, api: 'mockApi', @@ -89,7 +129,8 @@ describe('graphqlRequestDoc', () => { variables: mockVariables, }) - expect(retryAwareRequest).toHaveBeenCalledWith( + // Then + expect(retryAwareSpy).toHaveBeenCalledWith( { request: expect.any(Function), url: mockedAddress, @@ -102,7 +143,7 @@ describe('graphqlRequestDoc', () => { `query QueryName { example }`, - 'mockedAddress', + 'http://localhost:3000', mockVariables, expect.anything(), ) diff --git a/packages/cli-kit/src/public/node/api/graphql.ts b/packages/cli-kit/src/public/node/api/graphql.ts index 453481a7b99..e61eeeacc92 100644 --- a/packages/cli-kit/src/public/node/api/graphql.ts +++ b/packages/cli-kit/src/public/node/api/graphql.ts @@ -2,6 +2,7 @@ import {buildHeaders, httpsAgent} from '../../../private/node/api/headers.js' import {debugLogRequestInfo, errorHandler} from '../../../private/node/api/graphql.js' import {addPublicMetadata, runWithTimer} from '../metadata.js' import {retryAwareRequest} from '../../../private/node/api.js' +import {requestIdsCollection} from '../../../private/node/request-ids.js' import { GraphQLClient, rawRequest, @@ -105,11 +106,10 @@ async function performGraphQLRequest(options: PerformGraphQLRequestOpti async function logLastRequestIdFromResponse(response: GraphQLResponse) { try { const requestId = response.headers.get('x-request-id') - await addPublicMetadata(async () => { - return { - cmd_all_last_graphql_request_id: requestId ?? undefined, - } - }) + requestIdsCollection.addRequestId(requestId) + await addPublicMetadata(() => ({ + cmd_all_last_graphql_request_id: requestId ?? undefined, + })) // eslint-disable-next-line no-catch-all/no-catch-all } catch { // no problem if unable to get request ID. diff --git a/packages/cli-kit/src/public/node/api/http.ts b/packages/cli-kit/src/public/node/api/http.ts deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/packages/cli-kit/src/public/node/monorail.ts b/packages/cli-kit/src/public/node/monorail.ts index 45c1ae7db92..6f79d6b1289 100644 --- a/packages/cli-kit/src/public/node/monorail.ts +++ b/packages/cli-kit/src/public/node/monorail.ts @@ -10,7 +10,7 @@ const url = 'https://monorail-edge.shopifysvc.com/v1/produce' type Optional = 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.15' +export const MONORAIL_COMMAND_TOPIC = 'app_cli3_command/1.16' export interface Schemas { [MONORAIL_COMMAND_TOPIC]: {