Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send a list of request ids to monorail #5119

Merged
merged 7 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions packages/cli-kit/src/private/node/request-ids.test.ts
Original file line number Diff line number Diff line change
@@ -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'])
})
})
43 changes: 43 additions & 0 deletions packages/cli-kit/src/private/node/request-ids.ts
Original file line number Diff line number Diff line change
@@ -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()
2 changes: 2 additions & 0 deletions packages/cli-kit/src/public/node/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
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 =
Expand Down Expand Up @@ -70,14 +71,14 @@
}
const doOpenTelemetry = async () => {
const active = payload.public.cmd_all_timing_active_ms || 0
const network = payload.public.cmd_all_timing_network_ms || 0

Check warning on line 74 in packages/cli-kit/src/public/node/analytics.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/public/node/analytics.ts#L74

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const prompt = payload.public.cmd_all_timing_prompts_ms || 0

Check warning on line 75 in packages/cli-kit/src/public/node/analytics.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/public/node/analytics.ts#L75

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

return recordMetrics(
{
skipMetricAnalytics,
cliVersion: payload.public.cli_version,
owningPlugin: payload.public.cmd_all_plugin || '@shopify/cli',

Check warning on line 81 in packages/cli-kit/src/public/node/analytics.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/cli-kit/src/public/node/analytics.ts#L81

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
command: payload.public.command,
exitMode: options.exitMode,
},
Expand Down Expand Up @@ -154,6 +155,7 @@
cmd_all_timing_active_ms: totalTimeWithoutSubtimers,
cmd_all_exit: exitMode,
user_id: await getLastSeenUserIdAfterAuth(),
request_ids: requestIdsCollection.getRequestIds(),
},
sensitive: {
args: startArgs.join(' '),
Expand Down
71 changes: 56 additions & 15 deletions packages/cli-kit/src/public/node/api/graphql.test.ts
Original file line number Diff line number Diff line change
@@ -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),
Expand All @@ -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',
Expand All @@ -42,6 +49,8 @@ describe('graphqlRequest', () => {
addedHeaders: mockedAddedHeaders,
variables: mockVariables,
})

// Then
expect(GraphQLClient).toHaveBeenCalledWith(mockedAddress, {
agent: expect.any(Object),
headers: {
Expand All @@ -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: [
Expand All @@ -80,6 +117,9 @@ describe('graphqlRequestDoc', () => {
],
} as unknown as TypedDocumentNode<unknown, unknown>

const retryAwareSpy = vi.spyOn(api, 'retryAwareRequest')

// When
await graphqlRequestDoc({
query: document,
api: 'mockApi',
Expand All @@ -89,7 +129,8 @@ describe('graphqlRequestDoc', () => {
variables: mockVariables,
})

expect(retryAwareRequest).toHaveBeenCalledWith(
// Then
expect(retryAwareSpy).toHaveBeenCalledWith(
{
request: expect.any(Function),
url: mockedAddress,
Expand All @@ -102,7 +143,7 @@ describe('graphqlRequestDoc', () => {
`query QueryName {
example
}`,
'mockedAddress',
'http://localhost:3000',
mockVariables,
expect.anything(),
)
Expand Down
10 changes: 5 additions & 5 deletions packages/cli-kit/src/public/node/api/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -105,11 +106,10 @@ async function performGraphQLRequest<TResult>(options: PerformGraphQLRequestOpti
async function logLastRequestIdFromResponse(response: GraphQLResponse<unknown>) {
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.
Expand Down
Empty file.
2 changes: 1 addition & 1 deletion packages/cli-kit/src/public/node/monorail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.15'
export const MONORAIL_COMMAND_TOPIC = 'app_cli3_command/1.16'

export interface Schemas {
[MONORAIL_COMMAND_TOPIC]: {
Expand Down
Loading