From aa0b58890df62edb9e3dd2ac3a2aaf631aa343dd Mon Sep 17 00:00:00 2001 From: Kaung Zin Hein Date: Tue, 24 Sep 2024 22:33:49 -0400 Subject: [PATCH 1/6] feat(browser): Add `graphqlClientIntegration` Added support for graphql query with `xhr` with tests. Signed-off-by: Kaung Zin Hein --- .../suites/integrations/graphqlClient/init.js | 14 +++++ .../integrations/graphqlClient/xhr/subject.js | 16 ++++++ .../integrations/graphqlClient/xhr/test.ts | 51 +++++++++++++++++++ packages/browser/src/index.ts | 16 ++---- .../browser/src/integrations/graphqlClient.ts | 48 +++++++++++++++++ packages/browser/src/tracing/request.ts | 3 ++ packages/utils/src/graphql.ts | 26 ++++++++++ packages/utils/src/index.ts | 1 + packages/utils/test/graphql.test.ts | 41 +++++++++++++++ 9 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/integrations/graphqlClient/init.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts create mode 100644 packages/browser/src/integrations/graphqlClient.ts create mode 100644 packages/utils/src/graphql.ts create mode 100644 packages/utils/test/graphql.test.ts diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/init.js b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/init.js new file mode 100644 index 000000000000..7ca9df70b6c3 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/init.js @@ -0,0 +1,14 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [ + Sentry.browserTracingIntegration(), + Sentry.graphqlClientIntegration({ + endpoints: ['http://sentry-test.io/foo'], + }), + ], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js new file mode 100644 index 000000000000..d95cceeb8b7f --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js @@ -0,0 +1,16 @@ +const xhr = new XMLHttpRequest(); + +xhr.open('POST', 'http://sentry-test.io/foo'); +xhr.setRequestHeader('Accept', 'application/json'); +xhr.setRequestHeader('Content-Type', 'application/json'); + +const query = `query Test{ + + people { + name + pet + } +}`; + +const requestBody = JSON.stringify({ query }); +xhr.send(requestBody); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts new file mode 100644 index 000000000000..09dd02e3862b --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts @@ -0,0 +1,51 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; + +sentryTest.only('should create spans for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.route('**/foo', route => { + return route.fulfill({ + status: 200, + body: JSON.stringify({ + people: [ + { name: 'Amy', pet: 'dog' }, + { name: 'Jay', pet: 'cat' }, + ], + }), + headers: { + 'Content-Type': 'application/json', + }, + }); + }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + const requestSpans = eventData.spans?.filter(({ op }) => op === 'http.client'); + + expect(requestSpans).toHaveLength(1); + + expect(requestSpans![0]).toMatchObject({ + description: 'POST http://sentry-test.io/foo (query Test)', + parent_span_id: eventData.contexts?.trace?.span_id, + span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + trace_id: eventData.contexts?.trace?.trace_id, + data: { + type: 'xhr', + 'http.method': 'POST', + 'http.url': 'http://sentry-test.io/foo', + url: 'http://sentry-test.io/foo', + 'server.address': 'sentry-test.io', + 'sentry.op': 'http.client', + 'sentry.origin': 'auto.http.browser', + }, + }); +}); diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 10b974dc35e8..83d2c3aa3e8f 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -3,6 +3,7 @@ export * from './exports'; export { reportingObserverIntegration } from './integrations/reportingobserver'; export { httpClientIntegration } from './integrations/httpclient'; export { contextLinesIntegration } from './integrations/contextlines'; +export { graphqlClientIntegration } from './integrations/graphqlClient'; export { captureConsoleIntegration, @@ -13,10 +14,7 @@ export { captureFeedback, } from '@sentry/core'; -export { - replayIntegration, - getReplay, -} from '@sentry-internal/replay'; +export { replayIntegration, getReplay } from '@sentry-internal/replay'; export type { ReplayEventType, ReplayEventWithTime, @@ -34,17 +32,11 @@ export { replayCanvasIntegration } from '@sentry-internal/replay-canvas'; import { feedbackAsyncIntegration } from './feedbackAsync'; import { feedbackSyncIntegration } from './feedbackSync'; export { feedbackAsyncIntegration, feedbackSyncIntegration, feedbackSyncIntegration as feedbackIntegration }; -export { - getFeedback, - sendFeedback, -} from '@sentry-internal/feedback'; +export { getFeedback, sendFeedback } from '@sentry-internal/feedback'; export * from './metrics'; -export { - defaultRequestInstrumentationOptions, - instrumentOutgoingRequests, -} from './tracing/request'; +export { defaultRequestInstrumentationOptions, instrumentOutgoingRequests } from './tracing/request'; export { browserTracingIntegration, startBrowserTracingNavigationSpan, diff --git a/packages/browser/src/integrations/graphqlClient.ts b/packages/browser/src/integrations/graphqlClient.ts new file mode 100644 index 000000000000..fbe7caabd6cb --- /dev/null +++ b/packages/browser/src/integrations/graphqlClient.ts @@ -0,0 +1,48 @@ +import { SEMANTIC_ATTRIBUTE_SENTRY_OP, defineIntegration, spanToJSON } from '@sentry/core'; +import type { IntegrationFn } from '@sentry/types'; +import { parseGraphQLQuery } from '@sentry/utils'; + +interface GraphQLClientOptions { + endpoints: Array; +} + +const INTEGRATION_NAME = 'GraphQLClient'; + +const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { + return { + name: INTEGRATION_NAME, + setup(client) { + client.on('spanStart', span => { + const spanJSON = spanToJSON(span); + + const spanAttributes = spanJSON.data || {}; + + const spanOp = spanAttributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]; + const isHttpClientSpan = spanOp === 'http.client'; + + if (isHttpClientSpan) { + const httpUrl = spanAttributes['http.url']; + + const { endpoints } = options; + + const isTracedGraphqlEndpoint = endpoints.includes(httpUrl); + + if (isTracedGraphqlEndpoint) { + const httpMethod = spanAttributes['http.method']; + const graphqlQuery = spanAttributes['body']?.query as string; + + const { operationName, operationType } = parseGraphQLQuery(graphqlQuery); + const newOperation = operationName ? `${operationType} ${operationName}` : `${operationType}`; + + span.updateName(`${httpMethod} ${httpUrl} (${newOperation})`); + } + } + }); + }, + }; +}) satisfies IntegrationFn; + +/** + * GraphQL Client integration for the browser. + */ +export const graphqlClientIntegration = defineIntegration(_graphqlClientIntegration); diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts index e0b2e1a6d113..f302d6d50221 100644 --- a/packages/browser/src/tracing/request.ts +++ b/packages/browser/src/tracing/request.ts @@ -357,6 +357,8 @@ export function xhrCallback( return undefined; } + const requestBody = JSON.parse(sentryXhrData.body as string); + const fullUrl = getFullURL(sentryXhrData.url); const host = fullUrl ? parseUrl(fullUrl).host : undefined; @@ -374,6 +376,7 @@ export function xhrCallback( 'server.address': host, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', + body: requestBody, }, }) : new SentryNonRecordingSpan(); diff --git a/packages/utils/src/graphql.ts b/packages/utils/src/graphql.ts new file mode 100644 index 000000000000..2062643c7d00 --- /dev/null +++ b/packages/utils/src/graphql.ts @@ -0,0 +1,26 @@ +interface GraphQLOperation { + operationType: string | undefined; + operationName: string | undefined; +} + +/** + * Extract the name and type of the operation from the GraphQL query. + * @param query + * @returns + */ +export function parseGraphQLQuery(query: string): GraphQLOperation { + const queryRe = /^(?:\s*)(query|mutation|subscription)(?:\s*)(\w+)(?:\s*)[\{\(]/; + + const matched = query.match(queryRe); + + if (matched) { + return { + operationType: matched[1], + operationName: matched[2], + }; + } + return { + operationType: undefined, + operationName: undefined, + }; +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 245751b3e72c..2f35523be25a 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -39,3 +39,4 @@ export * from './lru'; export * from './buildPolyfills'; export * from './propagationContext'; export * from './version'; +export * from './graphql'; diff --git a/packages/utils/test/graphql.test.ts b/packages/utils/test/graphql.test.ts new file mode 100644 index 000000000000..59d5c0fadda8 --- /dev/null +++ b/packages/utils/test/graphql.test.ts @@ -0,0 +1,41 @@ +import { parseGraphQLQuery } from '../src'; + +describe('parseGraphQLQuery', () => { + const queryOne = `query Test { + items { + id + } + }`; + + const queryTwo = `mutation AddTestItem($input: TestItem!) { + addItem(input: $input) { + name + } + }`; + + const queryThree = `subscription OnTestItemAdded($itemID: ID!) { + itemAdded(itemID: $itemID) { + id + } + }`; + + // TODO: support name-less queries + // const queryFour = ` query { + // items { + // id + // } + // }`; + + test.each([ + ['should handle query type', queryOne, { operationName: 'Test', operationType: 'query' }], + ['should handle mutation type', queryTwo, { operationName: 'AddTestItem', operationType: 'mutation' }], + [ + 'should handle subscription type', + queryThree, + { operationName: 'OnTestItemAdded', operationType: 'subscription' }, + ], + // ['should handle query without name', queryFour, { operationName: undefined, operationType: 'query' }], + ])('%s', (_, input, output) => { + expect(parseGraphQLQuery(input)).toEqual(output); + }); +}); From 4614a55ec0bf166667395b10a3b7db24bfd03282 Mon Sep 17 00:00:00 2001 From: Kaung Zin Hein Date: Thu, 26 Sep 2024 10:44:44 -0400 Subject: [PATCH 2/6] feat(browser): Add support for fetch graphql request Added test for fetch graphql. Created new utility functions and added tests. Updated `instrumentFetch` to collect fetch request payload. Signed-off-by: Kaung Zin Hein --- .../graphqlClient/fetch/subject.js | 17 ++++ .../integrations/graphqlClient/fetch/test.ts | 55 ++++++++++++ .../integrations/graphqlClient/xhr/test.ts | 4 + .../browser/src/integrations/graphqlClient.ts | 24 ++++-- packages/browser/src/tracing/request.ts | 7 +- packages/core/src/baseclient.ts | 6 ++ packages/core/src/fetch.ts | 6 +- packages/types/src/client.ts | 11 +++ packages/types/src/instrument.ts | 3 + packages/utils/src/graphql.ts | 23 ++++- packages/utils/src/instrument/fetch.ts | 11 ++- packages/utils/test/graphql.test.ts | 86 +++++++++++-------- packages/utils/test/instrument/fetch.test.ts | 24 ++++-- 13 files changed, 219 insertions(+), 58 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/subject.js b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/subject.js new file mode 100644 index 000000000000..6a9398578b8b --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/subject.js @@ -0,0 +1,17 @@ +const query = `query Test{ + people { + name + pet + } +}`; + +const requestBody = JSON.stringify({ query }); + +fetch('http://sentry-test.io/foo', { + method: 'POST', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + }, + body: requestBody, +}); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts new file mode 100644 index 000000000000..ce8cbce4f8ce --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts @@ -0,0 +1,55 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; + +sentryTest.only('should create spans for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.route('**/foo', route => { + return route.fulfill({ + status: 200, + body: JSON.stringify({ + people: [ + { name: 'Amy', pet: 'dog' }, + { name: 'Jay', pet: 'cat' }, + ], + }), + headers: { + 'Content-Type': 'application/json', + }, + }); + }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + const requestSpans = eventData.spans?.filter(({ op }) => op === 'http.client'); + + expect(requestSpans).toHaveLength(1); + + expect(requestSpans![0]).toMatchObject({ + description: 'POST http://sentry-test.io/foo (query Test)', + parent_span_id: eventData.contexts?.trace?.span_id, + span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + trace_id: eventData.contexts?.trace?.trace_id, + status: 'ok', + data: expect.objectContaining({ + type: 'fetch', + 'http.method': 'POST', + 'http.url': 'http://sentry-test.io/foo', + url: 'http://sentry-test.io/foo', + 'server.address': 'sentry-test.io', + 'sentry.op': 'http.client', + 'sentry.origin': 'auto.http.browser', + body: { + query: expect.any(String), + }, + }), + }); +}); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts index 09dd02e3862b..0e8323f5ae17 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts @@ -38,6 +38,7 @@ sentryTest.only('should create spans for GraphQL XHR requests', async ({ getLoca start_timestamp: expect.any(Number), timestamp: expect.any(Number), trace_id: eventData.contexts?.trace?.trace_id, + status: 'ok', data: { type: 'xhr', 'http.method': 'POST', @@ -46,6 +47,9 @@ sentryTest.only('should create spans for GraphQL XHR requests', async ({ getLoca 'server.address': 'sentry-test.io', 'sentry.op': 'http.client', 'sentry.origin': 'auto.http.browser', + body: { + query: expect.any(String), + }, }, }); }); diff --git a/packages/browser/src/integrations/graphqlClient.ts b/packages/browser/src/integrations/graphqlClient.ts index fbe7caabd6cb..9f922854486d 100644 --- a/packages/browser/src/integrations/graphqlClient.ts +++ b/packages/browser/src/integrations/graphqlClient.ts @@ -1,4 +1,10 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_OP, defineIntegration, spanToJSON } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_URL_FULL, + defineIntegration, + spanToJSON, +} from '@sentry/core'; import type { IntegrationFn } from '@sentry/types'; import { parseGraphQLQuery } from '@sentry/utils'; @@ -13,6 +19,10 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { name: INTEGRATION_NAME, setup(client) { client.on('spanStart', span => { + client.emit('outgoingRequestSpanStart', span); + }); + + client.on('outgoingRequestSpanStart', span => { const spanJSON = spanToJSON(span); const spanAttributes = spanJSON.data || {}; @@ -21,17 +31,21 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { const isHttpClientSpan = spanOp === 'http.client'; if (isHttpClientSpan) { - const httpUrl = spanAttributes['http.url']; + const httpUrl = spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL] || spanAttributes['http.url']; const { endpoints } = options; const isTracedGraphqlEndpoint = endpoints.includes(httpUrl); if (isTracedGraphqlEndpoint) { - const httpMethod = spanAttributes['http.method']; - const graphqlQuery = spanAttributes['body']?.query as string; + const httpMethod = spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || spanAttributes['http.method']; + const graphqlBody = spanAttributes['body']; + + // Standard graphql request shape: https://graphql.org/learn/serving-over-http/#post-request + const graphqlQuery = graphqlBody && (graphqlBody['query'] as string); + const graphqlOperationName = graphqlBody && (graphqlBody['operationName'] as string); - const { operationName, operationType } = parseGraphQLQuery(graphqlQuery); + const { operationName = graphqlOperationName, operationType } = parseGraphQLQuery(graphqlQuery); const newOperation = operationName ? `${operationType} ${operationName}` : `${operationType}`; span.updateName(`${httpMethod} ${httpUrl} (${newOperation})`); diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts index f302d6d50221..c8db20f269ca 100644 --- a/packages/browser/src/tracing/request.ts +++ b/packages/browser/src/tracing/request.ts @@ -28,6 +28,7 @@ import { browserPerformanceTimeOrigin, dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader, + getGraphQLRequestPayload, parseUrl, stringMatchesSomePattern, } from '@sentry/utils'; @@ -357,13 +358,13 @@ export function xhrCallback( return undefined; } - const requestBody = JSON.parse(sentryXhrData.body as string); - const fullUrl = getFullURL(sentryXhrData.url); const host = fullUrl ? parseUrl(fullUrl).host : undefined; const hasParent = !!getActiveSpan(); + const graphqlRequest = getGraphQLRequestPayload(sentryXhrData.body as string); + const span = shouldCreateSpanResult && hasParent ? startInactiveSpan({ @@ -376,7 +377,7 @@ export function xhrCallback( 'server.address': host, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', - body: requestBody, + body: graphqlRequest, }, }) : new SentryNonRecordingSpan(); diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index c7a26f45ab70..d0c299aa7d1f 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -462,6 +462,9 @@ export abstract class BaseClient implements Client { /** @inheritdoc */ public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): () => void; + /** @inheritdoc */ + public on(hook: 'outgoingRequestSpanStart', callback: (span: Span) => void): () => void; + public on(hook: 'flush', callback: () => void): () => void; public on(hook: 'close', callback: () => void): () => void; @@ -540,6 +543,9 @@ export abstract class BaseClient implements Client { /** @inheritdoc */ public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void; + /** @inheritdoc */ + public emit(hook: 'outgoingRequestSpanStart', span: Span): void; + /** @inheritdoc */ public emit(hook: 'flush'): void; diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 9b9d2ece836b..438ea6234910 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -3,6 +3,7 @@ import { BAGGAGE_HEADER_NAME, dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader, + getGraphQLRequestPayload, isInstanceOf, parseUrl, } from '@sentry/utils'; @@ -65,13 +66,15 @@ export function instrumentFetchRequest( const scope = getCurrentScope(); const client = getClient(); - const { method, url } = handlerData.fetchData; + const { method, url, body } = handlerData.fetchData; const fullUrl = getFullURL(url); const host = fullUrl ? parseUrl(fullUrl).host : undefined; const hasParent = !!getActiveSpan(); + const graphqlRequest = getGraphQLRequestPayload(body as string); + const span = shouldCreateSpanResult && hasParent ? startInactiveSpan({ @@ -84,6 +87,7 @@ export function instrumentFetchRequest( 'server.address': host, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: spanOrigin, [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', + body: graphqlRequest, }, }) : new SentryNonRecordingSpan(); diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index 94f28f9157aa..d99018b3f772 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -291,6 +291,12 @@ export interface Client { */ on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): () => void; + /** + * A hook for GraphQL client integration to enhance a span and breadcrumbs with request data. + * @returns A function that, when executed, removes the registered callback. + */ + on(hook: 'outgoingRequestSpanStart', callback: (span: Span) => void): () => void; + /** * A hook that is called when the client is flushing * @returns A function that, when executed, removes the registered callback. @@ -387,6 +393,11 @@ export interface Client { */ emit(hook: 'startNavigationSpan', options: StartSpanOptions): void; + /** + * Emit a hook event for GraphQL client integration to enhance a span and breadcrumbs with request data. + */ + emit(hook: 'outgoingRequestSpanStart', span: Span): void; + /** * Emit a hook event for client flush */ diff --git a/packages/types/src/instrument.ts b/packages/types/src/instrument.ts index f0b239e86b14..dea6ce0f8d18 100644 --- a/packages/types/src/instrument.ts +++ b/packages/types/src/instrument.ts @@ -6,6 +6,8 @@ import type { WebFetchHeaders } from './webfetchapi'; // Make sure to cast it where needed! type XHRSendInput = unknown; +type FetchInput = unknown; + export type ConsoleLevel = 'debug' | 'info' | 'warn' | 'error' | 'log' | 'assert' | 'trace'; export interface SentryWrappedXMLHttpRequest { @@ -37,6 +39,7 @@ export interface HandlerDataXhr { interface SentryFetchData { method: string; url: string; + body?: FetchInput; request_body_size?: number; response_body_size?: number; // span_id for the fetch request diff --git a/packages/utils/src/graphql.ts b/packages/utils/src/graphql.ts index 2062643c7d00..5ffc2640ffb1 100644 --- a/packages/utils/src/graphql.ts +++ b/packages/utils/src/graphql.ts @@ -6,10 +6,9 @@ interface GraphQLOperation { /** * Extract the name and type of the operation from the GraphQL query. * @param query - * @returns */ export function parseGraphQLQuery(query: string): GraphQLOperation { - const queryRe = /^(?:\s*)(query|mutation|subscription)(?:\s*)(\w+)(?:\s*)[\{\(]/; + const queryRe = /^(?:\s*)(query|mutation|subscription)(?:\s*)(\w+)(?:\s*)[{(]/; const matched = query.match(queryRe); @@ -24,3 +23,23 @@ export function parseGraphQLQuery(query: string): GraphQLOperation { operationName: undefined, }; } + +/** + * Extract the payload of a request ONLY if it's GraphQL. + * @param payload - A valid JSON string + */ +export function getGraphQLRequestPayload(payload: string): any | undefined { + let graphqlBody = undefined; + try { + const requestBody = JSON.parse(payload); + const isGraphQLRequest = !!requestBody['query']; + if (isGraphQLRequest) { + graphqlBody = requestBody; + } + } finally { + // Fallback to undefined if payload is an invalid JSON (SyntaxError) + + /* eslint-disable no-unsafe-finally */ + return graphqlBody; + } +} diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index a161b8db79bb..4b067c98c786 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -48,12 +48,13 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat fill(GLOBAL_OBJ, 'fetch', function (originalFetch: () => void): () => void { return function (...args: any[]): void { - const { method, url } = parseFetchArgs(args); + const { method, url, body } = parseFetchArgs(args); const handlerData: HandlerDataFetch = { args, fetchData: { method, url, + body, }, startTimestamp: timestampInSeconds() * 1000, }; @@ -191,12 +192,12 @@ function getUrlFromResource(resource: FetchResource): string { } /** - * Parses the fetch arguments to find the used Http method and the url of the request. + * Parses the fetch arguments to find the used Http method, the url, and the payload of the request. * Exported for tests only. */ -export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string } { +export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string; body: string | null } { if (fetchArgs.length === 0) { - return { method: 'GET', url: '' }; + return { method: 'GET', url: '', body: null }; } if (fetchArgs.length === 2) { @@ -205,6 +206,7 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str return { url: getUrlFromResource(url), method: hasProp(options, 'method') ? String(options.method).toUpperCase() : 'GET', + body: hasProp(options, 'body') ? String(options.body) : null, }; } @@ -212,5 +214,6 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str return { url: getUrlFromResource(arg as FetchResource), method: hasProp(arg, 'method') ? String(arg.method).toUpperCase() : 'GET', + body: hasProp(arg, 'body') ? String(arg.body) : null, }; } diff --git a/packages/utils/test/graphql.test.ts b/packages/utils/test/graphql.test.ts index 59d5c0fadda8..a325e9c94bcc 100644 --- a/packages/utils/test/graphql.test.ts +++ b/packages/utils/test/graphql.test.ts @@ -1,41 +1,59 @@ -import { parseGraphQLQuery } from '../src'; +import { getGraphQLRequestPayload, parseGraphQLQuery } from '../src'; -describe('parseGraphQLQuery', () => { - const queryOne = `query Test { - items { - id - } - }`; +describe('graphql', () => { + describe('parseGraphQLQuery', () => { + const queryOne = `query Test { + items { + id + } + }`; - const queryTwo = `mutation AddTestItem($input: TestItem!) { - addItem(input: $input) { - name - } - }`; + const queryTwo = `mutation AddTestItem($input: TestItem!) { + addItem(input: $input) { + name + } + }`; - const queryThree = `subscription OnTestItemAdded($itemID: ID!) { - itemAdded(itemID: $itemID) { - id - } - }`; + const queryThree = `subscription OnTestItemAdded($itemID: ID!) { + itemAdded(itemID: $itemID) { + id + } + }`; - // TODO: support name-less queries - // const queryFour = ` query { - // items { - // id - // } - // }`; + // TODO: support name-less queries + // const queryFour = ` query { + // items { + // id + // } + // }`; - test.each([ - ['should handle query type', queryOne, { operationName: 'Test', operationType: 'query' }], - ['should handle mutation type', queryTwo, { operationName: 'AddTestItem', operationType: 'mutation' }], - [ - 'should handle subscription type', - queryThree, - { operationName: 'OnTestItemAdded', operationType: 'subscription' }, - ], - // ['should handle query without name', queryFour, { operationName: undefined, operationType: 'query' }], - ])('%s', (_, input, output) => { - expect(parseGraphQLQuery(input)).toEqual(output); + test.each([ + ['should handle query type', queryOne, { operationName: 'Test', operationType: 'query' }], + ['should handle mutation type', queryTwo, { operationName: 'AddTestItem', operationType: 'mutation' }], + [ + 'should handle subscription type', + queryThree, + { operationName: 'OnTestItemAdded', operationType: 'subscription' }, + ], + // ['should handle query without name', queryFour, { operationName: undefined, operationType: 'query' }], + ])('%s', (_, input, output) => { + expect(parseGraphQLQuery(input)).toEqual(output); + }); + }); + describe('getGraphQLRequestPayload', () => { + test('should return undefined for non-GraphQL request', () => { + const requestBody = { data: [1, 2, 3] }; + + expect(getGraphQLRequestPayload(JSON.stringify(requestBody))).toBeUndefined(); + }); + test('should return the payload object for GraphQL request', () => { + const requestBody = { + query: 'query Test {\r\n items {\r\n id\r\n }\r\n }', + operationName: 'Test', + variables: {}, + }; + + expect(getGraphQLRequestPayload(JSON.stringify(requestBody))).toEqual(requestBody); + }); }); }); diff --git a/packages/utils/test/instrument/fetch.test.ts b/packages/utils/test/instrument/fetch.test.ts index ea29e0c16c3e..c306e355258d 100644 --- a/packages/utils/test/instrument/fetch.test.ts +++ b/packages/utils/test/instrument/fetch.test.ts @@ -1,25 +1,31 @@ import { parseFetchArgs } from '../../src/instrument/fetch'; describe('instrument > parseFetchArgs', () => { + const data = { name: 'Test' }; + it.each([ - ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com' }], - ['URL object only', [new URL('http://example.com')], { method: 'GET', url: 'http://example.com/' }], - ['Request URL only', [{ url: 'http://example.com' }], { method: 'GET', url: 'http://example.com' }], + ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com', body: null }], + ['URL object only', [new URL('http://example.com')], { method: 'GET', url: 'http://example.com/', body: null }], + ['Request URL only', [{ url: 'http://example.com' }], { method: 'GET', url: 'http://example.com', body: null }], [ 'Request URL & method only', [{ url: 'http://example.com', method: 'post' }], - { method: 'POST', url: 'http://example.com' }, + { method: 'POST', url: 'http://example.com', body: null }, + ], + [ + 'string URL & options', + ['http://example.com', { method: 'post', body: JSON.stringify(data) }], + { method: 'POST', url: 'http://example.com', body: '{"name":"Test"}' }, ], - ['string URL & options', ['http://example.com', { method: 'post' }], { method: 'POST', url: 'http://example.com' }], [ 'URL object & options', - [new URL('http://example.com'), { method: 'post' }], - { method: 'POST', url: 'http://example.com/' }, + [new URL('http://example.com'), { method: 'post', body: JSON.stringify(data) }], + { method: 'POST', url: 'http://example.com/', body: '{"name":"Test"}' }, ], [ 'Request URL & options', - [{ url: 'http://example.com' }, { method: 'post' }], - { method: 'POST', url: 'http://example.com' }, + [{ url: 'http://example.com' }, { method: 'post', body: JSON.stringify(data) }], + { method: 'POST', url: 'http://example.com', body: '{"name":"Test"}' }, ], ])('%s', (_name, args, expected) => { const actual = parseFetchArgs(args as unknown[]); From 77c9fbcb93613d9be4448f093fe26fa3bce940cd Mon Sep 17 00:00:00 2001 From: Kaung Zin Hein Date: Thu, 26 Sep 2024 11:24:38 -0400 Subject: [PATCH 3/6] test(browser): Remove skip test Signed-off-by: Kaung Zin Hein --- .../suites/integrations/graphqlClient/fetch/test.ts | 8 ++------ .../suites/integrations/graphqlClient/xhr/test.ts | 8 ++------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts index ce8cbce4f8ce..dc7277989f82 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts @@ -2,13 +2,9 @@ import { expect } from '@playwright/test'; import type { Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; - -sentryTest.only('should create spans for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { - if (shouldSkipTracingTest()) { - sentryTest.skip(); - } +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; +sentryTest('should create spans for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); await page.route('**/foo', route => { diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts index 0e8323f5ae17..1efa45598a26 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts @@ -2,13 +2,9 @@ import { expect } from '@playwright/test'; import type { Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; -import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; - -sentryTest.only('should create spans for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { - if (shouldSkipTracingTest()) { - sentryTest.skip(); - } +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; +sentryTest('should create spans for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); await page.route('**/foo', route => { From 20b65165a64b98e1480c5804a927af3246291c66 Mon Sep 17 00:00:00 2001 From: Kaung Zin Hein Date: Thu, 26 Sep 2024 21:57:27 -0400 Subject: [PATCH 4/6] fix(browser): Attach request payload to fetch instrumentation only for graphql requests Signed-off-by: Kaung Zin Hein --- .../browser/src/integrations/graphqlClient.ts | 4 +- packages/utils/src/graphql.ts | 4 ++ packages/utils/src/instrument/fetch.ts | 35 +++++++++--- packages/utils/test/instrument/fetch.test.ts | 55 ++++++++++++++----- 4 files changed, 75 insertions(+), 23 deletions(-) diff --git a/packages/browser/src/integrations/graphqlClient.ts b/packages/browser/src/integrations/graphqlClient.ts index 9f922854486d..37bb159bea0a 100644 --- a/packages/browser/src/integrations/graphqlClient.ts +++ b/packages/browser/src/integrations/graphqlClient.ts @@ -34,7 +34,6 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { const httpUrl = spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL] || spanAttributes['http.url']; const { endpoints } = options; - const isTracedGraphqlEndpoint = endpoints.includes(httpUrl); if (isTracedGraphqlEndpoint) { @@ -42,7 +41,10 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { const graphqlBody = spanAttributes['body']; // Standard graphql request shape: https://graphql.org/learn/serving-over-http/#post-request + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const graphqlQuery = graphqlBody && (graphqlBody['query'] as string); + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const graphqlOperationName = graphqlBody && (graphqlBody['operationName'] as string); const { operationName = graphqlOperationName, operationType } = parseGraphQLQuery(graphqlQuery); diff --git a/packages/utils/src/graphql.ts b/packages/utils/src/graphql.ts index 5ffc2640ffb1..8b4265f4307c 100644 --- a/packages/utils/src/graphql.ts +++ b/packages/utils/src/graphql.ts @@ -27,12 +27,16 @@ export function parseGraphQLQuery(query: string): GraphQLOperation { /** * Extract the payload of a request ONLY if it's GraphQL. * @param payload - A valid JSON string + * @returns A POJO or undefined */ export function getGraphQLRequestPayload(payload: string): any | undefined { let graphqlBody = undefined; try { const requestBody = JSON.parse(payload); + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const isGraphQLRequest = !!requestBody['query']; + if (isGraphQLRequest) { graphqlBody = requestBody; } diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index 4b067c98c786..4212f32d05f6 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -1,6 +1,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import type { HandlerDataFetch } from '@sentry/types'; +import { getGraphQLRequestPayload } from '../graphql'; import { isError } from '../is'; import { addNonEnumerableProperty, fill } from '../object'; import { supportsNativeFetch } from '../supports'; @@ -48,17 +49,25 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat fill(GLOBAL_OBJ, 'fetch', function (originalFetch: () => void): () => void { return function (...args: any[]): void { - const { method, url, body } = parseFetchArgs(args); + const { method, url } = parseFetchArgs(args); const handlerData: HandlerDataFetch = { args, fetchData: { method, url, - body, }, startTimestamp: timestampInSeconds() * 1000, }; + const body = parseFetchPayload(args); + + if (body) { + const graphqlRequest = getGraphQLRequestPayload(body); + if (graphqlRequest) { + handlerData.fetchData.body = body; + } + } + // if there is no callback, fetch is instrumented directly if (!onFetchResolved) { triggerHandlers('fetch', { @@ -192,12 +201,12 @@ function getUrlFromResource(resource: FetchResource): string { } /** - * Parses the fetch arguments to find the used Http method, the url, and the payload of the request. + * Parses the fetch arguments to find the used Http method and the url of the request. * Exported for tests only. */ -export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string; body: string | null } { +export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string } { if (fetchArgs.length === 0) { - return { method: 'GET', url: '', body: null }; + return { method: 'GET', url: '' }; } if (fetchArgs.length === 2) { @@ -206,7 +215,6 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str return { url: getUrlFromResource(url), method: hasProp(options, 'method') ? String(options.method).toUpperCase() : 'GET', - body: hasProp(options, 'body') ? String(options.body) : null, }; } @@ -214,6 +222,19 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str return { url: getUrlFromResource(arg as FetchResource), method: hasProp(arg, 'method') ? String(arg.method).toUpperCase() : 'GET', - body: hasProp(arg, 'body') ? String(arg.body) : null, }; } + +/** + * Parses the fetch arguments to extract the request payload. + * Exported for tests only. + */ +export function parseFetchPayload(fetchArgs: unknown[]): string | undefined { + if (fetchArgs.length === 2) { + const options = fetchArgs[1]; + return hasProp(options, 'body') ? String(options.body) : undefined; + } + + const arg = fetchArgs[0]; + return hasProp(arg, 'body') ? String(arg.body) : undefined; +} diff --git a/packages/utils/test/instrument/fetch.test.ts b/packages/utils/test/instrument/fetch.test.ts index c306e355258d..04a4d3da7586 100644 --- a/packages/utils/test/instrument/fetch.test.ts +++ b/packages/utils/test/instrument/fetch.test.ts @@ -1,34 +1,59 @@ -import { parseFetchArgs } from '../../src/instrument/fetch'; +import { parseFetchArgs, parseFetchPayload } from '../../src/instrument/fetch'; describe('instrument > parseFetchArgs', () => { - const data = { name: 'Test' }; - it.each([ - ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com', body: null }], - ['URL object only', [new URL('http://example.com')], { method: 'GET', url: 'http://example.com/', body: null }], - ['Request URL only', [{ url: 'http://example.com' }], { method: 'GET', url: 'http://example.com', body: null }], + ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com' }], + ['URL object only', [new URL('http://example.com')], { method: 'GET', url: 'http://example.com/' }], + ['Request URL only', [{ url: 'http://example.com' }], { method: 'GET', url: 'http://example.com' }], [ 'Request URL & method only', [{ url: 'http://example.com', method: 'post' }], - { method: 'POST', url: 'http://example.com', body: null }, + { method: 'POST', url: 'http://example.com' }, ], + ['string URL & options', ['http://example.com', { method: 'post' }], { method: 'POST', url: 'http://example.com' }], + [ + 'URL object & options', + [new URL('http://example.com'), { method: 'post' }], + { method: 'POST', url: 'http://example.com/' }, + ], + [ + 'Request URL & options', + [{ url: 'http://example.com' }, { method: 'post' }], + { method: 'POST', url: 'http://example.com' }, + ], + ])('%s', (_name, args, expected) => { + const actual = parseFetchArgs(args as unknown[]); + + expect(actual).toEqual(expected); + }); +}); + +describe('instrument > parseFetchPayload', () => { + const data = [1, 2, 3]; + const jsonData = '{"data":[1,2,3]}'; + + it.each([ + ['string URL only', ['http://example.com'], undefined], + ['URL object only', [new URL('http://example.com')], undefined], + ['Request URL only', [{ url: 'http://example.com' }], undefined], [ - 'string URL & options', - ['http://example.com', { method: 'post', body: JSON.stringify(data) }], - { method: 'POST', url: 'http://example.com', body: '{"name":"Test"}' }, + 'Request URL & method only', + [{ url: 'http://example.com', method: 'post', body: JSON.stringify({ data }) }], + jsonData, ], + ['string URL & options', ['http://example.com', { method: 'post', body: JSON.stringify({ data }) }], jsonData], [ 'URL object & options', - [new URL('http://example.com'), { method: 'post', body: JSON.stringify(data) }], - { method: 'POST', url: 'http://example.com/', body: '{"name":"Test"}' }, + [new URL('http://example.com'), { method: 'post', body: JSON.stringify({ data }) }], + jsonData, ], [ 'Request URL & options', - [{ url: 'http://example.com' }, { method: 'post', body: JSON.stringify(data) }], - { method: 'POST', url: 'http://example.com', body: '{"name":"Test"}' }, + [{ url: 'http://example.com' }, { method: 'post', body: JSON.stringify({ data }) }], + jsonData, ], ])('%s', (_name, args, expected) => { - const actual = parseFetchArgs(args as unknown[]); + const actual = parseFetchPayload(args as unknown[]); expect(actual).toEqual(expected); }); From a3042182aab8be224cf611f4405b1a87098a15af Mon Sep 17 00:00:00 2001 From: Kaung Zin Hein Date: Fri, 27 Sep 2024 11:57:27 -0400 Subject: [PATCH 5/6] fix(browser): Emit the `outgoingRequestSpanStart` hook after the span has started Signed-off-by: Kaung Zin Hein --- .../integrations/graphqlClient/fetch/test.ts | 13 ++++++++--- .../integrations/graphqlClient/xhr/subject.js | 9 ++++---- .../integrations/graphqlClient/xhr/test.ts | 13 ++++++++--- .../browser/src/integrations/graphqlClient.ts | 22 +++++++++---------- packages/browser/src/tracing/request.ts | 7 +++--- packages/core/src/baseclient.ts | 4 ++-- packages/core/src/fetch.ts | 7 +++--- packages/types/src/client.ts | 4 ++-- 8 files changed, 47 insertions(+), 32 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts index dc7277989f82..17bdfa4b9215 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts @@ -4,6 +4,15 @@ import type { Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; +// Duplicate from subject.js +const query = `query Test{ + people { + name + pet + } +}`; +const queryPayload = JSON.stringify({ query }); + sentryTest('should create spans for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); @@ -43,9 +52,7 @@ sentryTest('should create spans for GraphQL Fetch requests', async ({ getLocalTe 'server.address': 'sentry-test.io', 'sentry.op': 'http.client', 'sentry.origin': 'auto.http.browser', - body: { - query: expect.any(String), - }, + body: queryPayload, }), }); }); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js index d95cceeb8b7f..85645f645635 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/subject.js @@ -5,11 +5,10 @@ xhr.setRequestHeader('Accept', 'application/json'); xhr.setRequestHeader('Content-Type', 'application/json'); const query = `query Test{ - - people { - name - pet - } + people { + name + pet + } }`; const requestBody = JSON.stringify({ query }); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts index 1efa45598a26..d1c78626d6c3 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts @@ -4,6 +4,15 @@ import type { Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; +// Duplicate from subject.js +const query = `query Test{ + people { + name + pet + } +}`; +const queryPayload = JSON.stringify({ query }); + sentryTest('should create spans for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); @@ -43,9 +52,7 @@ sentryTest('should create spans for GraphQL XHR requests', async ({ getLocalTest 'server.address': 'sentry-test.io', 'sentry.op': 'http.client', 'sentry.origin': 'auto.http.browser', - body: { - query: expect.any(String), - }, + body: queryPayload, }, }); }); diff --git a/packages/browser/src/integrations/graphqlClient.ts b/packages/browser/src/integrations/graphqlClient.ts index 37bb159bea0a..ee6b4cd1f8b8 100644 --- a/packages/browser/src/integrations/graphqlClient.ts +++ b/packages/browser/src/integrations/graphqlClient.ts @@ -12,17 +12,19 @@ interface GraphQLClientOptions { endpoints: Array; } +interface GraphQLRequestPayload { + query: string; + operationName?: string; + variables?: Record; +} + const INTEGRATION_NAME = 'GraphQLClient'; const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { return { name: INTEGRATION_NAME, setup(client) { - client.on('spanStart', span => { - client.emit('outgoingRequestSpanStart', span); - }); - - client.on('outgoingRequestSpanStart', span => { + client.on('outgoingRequestSpanStart', (span, { body }) => { const spanJSON = spanToJSON(span); const spanAttributes = spanJSON.data || {}; @@ -38,19 +40,17 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { if (isTracedGraphqlEndpoint) { const httpMethod = spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || spanAttributes['http.method']; - const graphqlBody = spanAttributes['body']; + const graphqlBody = body as GraphQLRequestPayload; // Standard graphql request shape: https://graphql.org/learn/serving-over-http/#post-request - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const graphqlQuery = graphqlBody && (graphqlBody['query'] as string); - - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const graphqlOperationName = graphqlBody && (graphqlBody['operationName'] as string); + const graphqlQuery = graphqlBody.query; + const graphqlOperationName = graphqlBody.operationName; const { operationName = graphqlOperationName, operationType } = parseGraphQLQuery(graphqlQuery); const newOperation = operationName ? `${operationType} ${operationName}` : `${operationType}`; span.updateName(`${httpMethod} ${httpUrl} (${newOperation})`); + span.setAttribute('body', JSON.stringify(graphqlBody)); } } }); diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts index c8db20f269ca..54bb23b47162 100644 --- a/packages/browser/src/tracing/request.ts +++ b/packages/browser/src/tracing/request.ts @@ -363,8 +363,6 @@ export function xhrCallback( const hasParent = !!getActiveSpan(); - const graphqlRequest = getGraphQLRequestPayload(sentryXhrData.body as string); - const span = shouldCreateSpanResult && hasParent ? startInactiveSpan({ @@ -377,7 +375,6 @@ export function xhrCallback( 'server.address': host, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', - body: graphqlRequest, }, }) : new SentryNonRecordingSpan(); @@ -398,6 +395,10 @@ export function xhrCallback( ); } + if (client) { + client.emit('outgoingRequestSpanStart', span, { body: getGraphQLRequestPayload(sentryXhrData.body as string) }); + } + return span; } diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index d0c299aa7d1f..34567e298803 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -463,7 +463,7 @@ export abstract class BaseClient implements Client { public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): () => void; /** @inheritdoc */ - public on(hook: 'outgoingRequestSpanStart', callback: (span: Span) => void): () => void; + public on(hook: 'outgoingRequestSpanStart', callback: (span: Span, { body }: { body: unknown }) => void): () => void; public on(hook: 'flush', callback: () => void): () => void; @@ -544,7 +544,7 @@ export abstract class BaseClient implements Client { public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void; /** @inheritdoc */ - public emit(hook: 'outgoingRequestSpanStart', span: Span): void; + public emit(hook: 'outgoingRequestSpanStart', span: Span, { body }: { body: unknown }): void; /** @inheritdoc */ public emit(hook: 'flush'): void; diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 438ea6234910..2d1df2ab4bdc 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -73,8 +73,6 @@ export function instrumentFetchRequest( const hasParent = !!getActiveSpan(); - const graphqlRequest = getGraphQLRequestPayload(body as string); - const span = shouldCreateSpanResult && hasParent ? startInactiveSpan({ @@ -87,7 +85,6 @@ export function instrumentFetchRequest( 'server.address': host, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: spanOrigin, [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', - body: graphqlRequest, }, }) : new SentryNonRecordingSpan(); @@ -116,6 +113,10 @@ export function instrumentFetchRequest( ); } + if (client) { + client.emit('outgoingRequestSpanStart', span, { body: getGraphQLRequestPayload(body as string) }); + } + return span; } diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index d99018b3f772..aa2fd7485ad0 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -295,7 +295,7 @@ export interface Client { * A hook for GraphQL client integration to enhance a span and breadcrumbs with request data. * @returns A function that, when executed, removes the registered callback. */ - on(hook: 'outgoingRequestSpanStart', callback: (span: Span) => void): () => void; + on(hook: 'outgoingRequestSpanStart', callback: (span: Span, { body }: { body: unknown }) => void): () => void; /** * A hook that is called when the client is flushing @@ -396,7 +396,7 @@ export interface Client { /** * Emit a hook event for GraphQL client integration to enhance a span and breadcrumbs with request data. */ - emit(hook: 'outgoingRequestSpanStart', span: Span): void; + emit(hook: 'outgoingRequestSpanStart', span: Span, { body }: { body: unknown }): void; /** * Emit a hook event for client flush From 55e6f1b8db2fc039e1ae48f9411fab0d4437a6ae Mon Sep 17 00:00:00 2001 From: Kaung Zin Hein Date: Sun, 29 Sep 2024 11:25:08 -0400 Subject: [PATCH 6/6] feat(browser): Update breadcrumbs with graphql request data Signed-off-by: Kaung Zin Hein --- .../integrations/graphqlClient/fetch/test.ts | 41 +++++++- .../integrations/graphqlClient/xhr/test.ts | 40 +++++++- .../browser/src/integrations/breadcrumbs.ts | 66 +++++++------ .../browser/src/integrations/graphqlClient.ts | 93 ++++++++++++++----- packages/core/src/baseclient.ts | 9 ++ packages/types/src/client.ts | 18 +++- 6 files changed, 208 insertions(+), 59 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts index 17bdfa4b9215..c6d12cb1f17f 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/fetch/test.ts @@ -13,7 +13,7 @@ const query = `query Test{ }`; const queryPayload = JSON.stringify({ query }); -sentryTest('should create spans for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { +sentryTest('should update spans for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); await page.route('**/foo', route => { @@ -56,3 +56,42 @@ sentryTest('should create spans for GraphQL Fetch requests', async ({ getLocalTe }), }); }); + +sentryTest('should update breadcrumbs for GraphQL Fetch requests', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.route('**/foo', route => { + return route.fulfill({ + status: 200, + body: JSON.stringify({ + people: [ + { name: 'Amy', pet: 'dog' }, + { name: 'Jay', pet: 'cat' }, + ], + }), + headers: { + 'Content-Type': 'application/json', + }, + }); + }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData?.breadcrumbs?.length).toBe(1); + + expect(eventData!.breadcrumbs![0]).toEqual({ + timestamp: expect.any(Number), + category: 'fetch', + type: 'http', + data: { + method: 'POST', + status_code: 200, + url: 'http://sentry-test.io/foo', + __span: expect.any(String), + graphql: { + query: query, + operationName: 'query Test', + }, + }, + }); +}); diff --git a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts index d1c78626d6c3..983c22905478 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts @@ -13,7 +13,7 @@ const query = `query Test{ }`; const queryPayload = JSON.stringify({ query }); -sentryTest('should create spans for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { +sentryTest('should update spans for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); await page.route('**/foo', route => { @@ -56,3 +56,41 @@ sentryTest('should create spans for GraphQL XHR requests', async ({ getLocalTest }, }); }); + +sentryTest('should update breadcrumbs for GraphQL XHR requests', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.route('**/foo', route => { + return route.fulfill({ + status: 200, + body: JSON.stringify({ + people: [ + { name: 'Amy', pet: 'dog' }, + { name: 'Jay', pet: 'cat' }, + ], + }), + headers: { + 'Content-Type': 'application/json', + }, + }); + }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData?.breadcrumbs?.length).toBe(1); + + expect(eventData!.breadcrumbs![0]).toEqual({ + timestamp: expect.any(Number), + category: 'xhr', + type: 'http', + data: { + method: 'POST', + status_code: 200, + url: 'http://sentry-test.io/foo', + graphql: { + query: query, + operationName: 'query Test', + }, + }, + }); +}); diff --git a/packages/browser/src/integrations/breadcrumbs.ts b/packages/browser/src/integrations/breadcrumbs.ts index db30a48dda67..726fbf0871ad 100644 --- a/packages/browser/src/integrations/breadcrumbs.ts +++ b/packages/browser/src/integrations/breadcrumbs.ts @@ -17,6 +17,7 @@ import type { HandlerDataHistory, HandlerDataXhr, IntegrationFn, + SeverityLevel, XhrBreadcrumbData, XhrBreadcrumbHint, } from '@sentry/types'; @@ -26,6 +27,7 @@ import { getBreadcrumbLogLevelFromHttpStatusCode, getComponentName, getEventDescription, + getGraphQLRequestPayload, htmlTreeAsString, logger, parseUrl, @@ -248,17 +250,16 @@ function _getXhrBreadcrumbHandler(client: Client): (handlerData: HandlerDataXhr) endTimestamp, }; - const level = getBreadcrumbLogLevelFromHttpStatusCode(status_code); + const breadcrumb = { + category: 'xhr', + data, + type: 'http', + level: getBreadcrumbLogLevelFromHttpStatusCode(status_code), + }; - addBreadcrumb( - { - category: 'xhr', - data, - type: 'http', - level, - }, - hint, - ); + client.emit('outgoingRequestBreadcrumbStart', breadcrumb, { body: getGraphQLRequestPayload(body as string) }); + + addBreadcrumb(breadcrumb, hint); }; } @@ -292,15 +293,18 @@ function _getFetchBreadcrumbHandler(client: Client): (handlerData: HandlerDataFe endTimestamp, }; - addBreadcrumb( - { - category: 'fetch', - data, - level: 'error', - type: 'http', - }, - hint, - ); + const breadcrumb = { + category: 'fetch', + data, + level: 'error' as SeverityLevel, + type: 'http', + }; + + client.emit('outgoingRequestBreadcrumbStart', breadcrumb, { + body: getGraphQLRequestPayload(handlerData.fetchData.body as string), + }); + + addBreadcrumb(breadcrumb, hint); } else { const response = handlerData.response as Response | undefined; const data: FetchBreadcrumbData = { @@ -313,17 +317,19 @@ function _getFetchBreadcrumbHandler(client: Client): (handlerData: HandlerDataFe startTimestamp, endTimestamp, }; - const level = getBreadcrumbLogLevelFromHttpStatusCode(data.status_code); - - addBreadcrumb( - { - category: 'fetch', - data, - type: 'http', - level, - }, - hint, - ); + + const breadcrumb = { + category: 'fetch', + data, + type: 'http', + level: getBreadcrumbLogLevelFromHttpStatusCode(data.status_code), + }; + + client.emit('outgoingRequestBreadcrumbStart', breadcrumb, { + body: getGraphQLRequestPayload(handlerData.fetchData.body as string), + }); + + addBreadcrumb(breadcrumb, hint); } }; } diff --git a/packages/browser/src/integrations/graphqlClient.ts b/packages/browser/src/integrations/graphqlClient.ts index ee6b4cd1f8b8..2e0843d91af4 100644 --- a/packages/browser/src/integrations/graphqlClient.ts +++ b/packages/browser/src/integrations/graphqlClient.ts @@ -5,7 +5,7 @@ import { defineIntegration, spanToJSON, } from '@sentry/core'; -import type { IntegrationFn } from '@sentry/types'; +import type { Client, IntegrationFn } from '@sentry/types'; import { parseGraphQLQuery } from '@sentry/utils'; interface GraphQLClientOptions { @@ -24,39 +24,82 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => { return { name: INTEGRATION_NAME, setup(client) { - client.on('outgoingRequestSpanStart', (span, { body }) => { - const spanJSON = spanToJSON(span); + _updateSpanWithGraphQLData(client, options); + _updateBreadcrumbWithGraphQLData(client, options); + }, + }; +}) satisfies IntegrationFn; + +function _updateSpanWithGraphQLData(client: Client, options: GraphQLClientOptions): void { + client.on('outgoingRequestSpanStart', (span, { body }) => { + const spanJSON = spanToJSON(span); + + const spanAttributes = spanJSON.data || {}; + const spanOp = spanAttributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]; - const spanAttributes = spanJSON.data || {}; + const isHttpClientSpan = spanOp === 'http.client'; - const spanOp = spanAttributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]; - const isHttpClientSpan = spanOp === 'http.client'; + if (isHttpClientSpan) { + const httpUrl = spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL] || spanAttributes['http.url']; - if (isHttpClientSpan) { - const httpUrl = spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL] || spanAttributes['http.url']; + const { endpoints } = options; + const isTracedGraphqlEndpoint = endpoints.includes(httpUrl); - const { endpoints } = options; - const isTracedGraphqlEndpoint = endpoints.includes(httpUrl); + if (isTracedGraphqlEndpoint) { + const httpMethod = spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || spanAttributes['http.method']; - if (isTracedGraphqlEndpoint) { - const httpMethod = spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || spanAttributes['http.method']; - const graphqlBody = body as GraphQLRequestPayload; + const operationInfo = _getGraphQLOperation(body); + span.updateName(`${httpMethod} ${httpUrl} (${operationInfo})`); + span.setAttribute('body', JSON.stringify(body)); + } + } + }); +} + +function _updateBreadcrumbWithGraphQLData(client: Client, options: GraphQLClientOptions): void { + client.on('outgoingRequestBreadcrumbStart', (breadcrumb, { body }) => { + const { category, type, data } = breadcrumb; + + const isFetch = category === 'fetch'; + const isXhr = category === 'xhr'; + const isHttpBreadcrumb = type === 'http'; - // Standard graphql request shape: https://graphql.org/learn/serving-over-http/#post-request - const graphqlQuery = graphqlBody.query; - const graphqlOperationName = graphqlBody.operationName; + if (isHttpBreadcrumb && (isFetch || isXhr)) { + const httpUrl = data && data.url; + const { endpoints } = options; - const { operationName = graphqlOperationName, operationType } = parseGraphQLQuery(graphqlQuery); - const newOperation = operationName ? `${operationType} ${operationName}` : `${operationType}`; + const isTracedGraphqlEndpoint = endpoints.includes(httpUrl); - span.updateName(`${httpMethod} ${httpUrl} (${newOperation})`); - span.setAttribute('body', JSON.stringify(graphqlBody)); - } + if (isTracedGraphqlEndpoint && data) { + if (!data.graphql) { + const operationInfo = _getGraphQLOperation(body); + + data.graphql = { + query: (body as GraphQLRequestPayload).query, + operationName: operationInfo, + }; } - }); - }, - }; -}) satisfies IntegrationFn; + + // The body prop attached to HandlerDataFetch for the span should be removed. + if (isFetch && data.body) { + delete data.body; + } + } + } + }); +} + +function _getGraphQLOperation(requestBody: unknown): string { + // Standard graphql request shape: https://graphql.org/learn/serving-over-http/#post-request + const graphqlBody = requestBody as GraphQLRequestPayload; + const graphqlQuery = graphqlBody.query; + const graphqlOperationName = graphqlBody.operationName; + + const { operationName = graphqlOperationName, operationType } = parseGraphQLQuery(graphqlQuery); + const operationInfo = operationName ? `${operationType} ${operationName}` : `${operationType}`; + + return operationInfo; +} /** * GraphQL Client integration for the browser. diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 34567e298803..b1633ff23401 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -465,6 +465,12 @@ export abstract class BaseClient implements Client { /** @inheritdoc */ public on(hook: 'outgoingRequestSpanStart', callback: (span: Span, { body }: { body: unknown }) => void): () => void; + /** @inheritdoc */ + public on( + hook: 'outgoingRequestBreadcrumbStart', + callback: (breadcrumb: Breadcrumb, { body }: { body: unknown }) => void, + ): () => void; + public on(hook: 'flush', callback: () => void): () => void; public on(hook: 'close', callback: () => void): () => void; @@ -546,6 +552,9 @@ export abstract class BaseClient implements Client { /** @inheritdoc */ public emit(hook: 'outgoingRequestSpanStart', span: Span, { body }: { body: unknown }): void; + /** @inheritdoc */ + public emit(hook: 'outgoingRequestBreadcrumbStart', breadcrumb: Breadcrumb, { body }: { body: unknown }): void; + /** @inheritdoc */ public emit(hook: 'flush'): void; diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index aa2fd7485ad0..b5b3c7dc0dff 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -292,11 +292,20 @@ export interface Client { on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): () => void; /** - * A hook for GraphQL client integration to enhance a span and breadcrumbs with request data. + * A hook for GraphQL client integration to enhance a span with request data. * @returns A function that, when executed, removes the registered callback. */ on(hook: 'outgoingRequestSpanStart', callback: (span: Span, { body }: { body: unknown }) => void): () => void; + /** + * A hook for GraphQL client integration to enhance a breadcrumb with request data. + * @returns A function that, when executed, removes the registered callback. + */ + on( + hook: 'outgoingRequestBreadcrumbStart', + callback: (breadcrumb: Breadcrumb, { body }: { body: unknown }) => void, + ): () => void; + /** * A hook that is called when the client is flushing * @returns A function that, when executed, removes the registered callback. @@ -394,10 +403,15 @@ export interface Client { emit(hook: 'startNavigationSpan', options: StartSpanOptions): void; /** - * Emit a hook event for GraphQL client integration to enhance a span and breadcrumbs with request data. + * Emit a hook event for GraphQL client integration to enhance a span with request data. */ emit(hook: 'outgoingRequestSpanStart', span: Span, { body }: { body: unknown }): void; + /** + * Emit a hook event for GraphQL client integration to enhance a breadcrumb with request data. + */ + emit(hook: 'outgoingRequestBreadcrumbStart', breadcrumb: Breadcrumb, { body }: { body: unknown }): void; + /** * Emit a hook event for client flush */