fix(browser): Refactor span handler to use hint

Zen-cronic · Zen-cronic · commit b36a4be80e5a · 2025-01-22T20:07:11.000-05:00
- Moved fetch-related operations into the grahqlClient integration.
- Used Hint types for `beforeOutgoingRequestSpan` hooks.

Signed-off-by: Kaung Zin Hein &lt;83657429+Zen-cronic@users.noreply.github.com&gt;
diff --git a/packages/browser/src/integrations/graphqlClient.ts b/packages/browser/src/integrations/graphqlClient.ts
@@ -1,14 +1,14 @@
 import { SENTRY_XHR_DATA_KEY } from '@sentry-internal/browser-utils';
-import { getBodyString } from '@sentry-internal/replay';
+import { FetchHint, getBodyString, XhrHint } from '@sentry-internal/replay';
 import {
   SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
   SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_URL_FULL,
   defineIntegration,
   spanToJSON,
 } from '@sentry/core';
-import type { Client, HandlerDataFetch, HandlerDataXhr, IntegrationFn } from '@sentry/types';
-import { getGraphQLRequestPayload, isString, parseGraphQLQuery, stringMatchesSomePattern } from '@sentry/utils';
+import type { Client, IntegrationFn } from '@sentry/types';
+import { isString, stringMatchesSomePattern } from '@sentry/utils';
 
 interface GraphQLClientOptions {
   endpoints: Array<string | RegExp>;
@@ -35,34 +35,37 @@ const _graphqlClientIntegration = ((options: GraphQLClientOptions) => {
 }) satisfies IntegrationFn;
 
 function _updateSpanWithGraphQLData(client: Client, options: GraphQLClientOptions): void {
-  client.on('beforeOutgoingRequestSpan', (span, handlerData) => {
+  client.on('beforeOutgoingRequestSpan', (span, hint) => {
     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[SEMANTIC_ATTRIBUTE_URL_FULL] || spanAttributes['http.url'];
-      const httpMethod = spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || spanAttributes['http.method'];
+    if (!isHttpClientSpan) {
+      return;
+    }
 
-      if (!isString(httpUrl) || !isString(httpMethod)) {
-        return;
-      }
+    const httpUrl = spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL] || spanAttributes['http.url'];
+    const httpMethod = spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || spanAttributes['http.method'];
 
-      const { endpoints } = options;
-      const isTracedGraphqlEndpoint = stringMatchesSomePattern(httpUrl, endpoints);
-      const payload = _getRequestPayloadXhrOrFetch(handlerData);
+    if (!isString(httpUrl) || !isString(httpMethod)) {
+      return;
+    }
 
-      if (isTracedGraphqlEndpoint && payload) {
-        const graphqlBody = getGraphQLRequestPayload(payload) as GraphQLRequestPayload;
-        const operationInfo = _getGraphQLOperation(graphqlBody);
+    const { endpoints } = options;
+    const isTracedGraphqlEndpoint = stringMatchesSomePattern(httpUrl, endpoints);
+    const payload = _getRequestPayloadXhrOrFetch(hint);
 
-        span.updateName(`${httpMethod} ${httpUrl} (${operationInfo})`);
-        span.setAttribute('graphql.document', payload);
-      }
+    if (isTracedGraphqlEndpoint && payload) {
+      const graphqlBody = getGraphQLRequestPayload(payload) as GraphQLRequestPayload;
+      const operationInfo = _getGraphQLOperation(graphqlBody);
+
+      span.updateName(`${httpMethod} ${httpUrl} (${operationInfo})`);
+      span.setAttribute('graphql.document', payload);
     }
+    
   });
 }
 
@@ -113,26 +116,95 @@ function _getGraphQLOperation(requestBody: GraphQLRequestPayload): string {
 }
 
 /**
- * Get the request body/payload based on the shape of the HandlerData
- * @param handlerData - Xhr or Fetch HandlerData
+ * Get the request body/payload based on the shape of the hint
+ * TODO: export for test?
  */
-function _getRequestPayloadXhrOrFetch(handlerData: HandlerDataXhr | HandlerDataFetch): string | undefined {
-  const isXhr = 'xhr' in handlerData;
-  const isFetch = 'fetchData' in handlerData;
+function _getRequestPayloadXhrOrFetch(hint: XhrHint | FetchHint): string | undefined {
+  const isXhr = 'xhr' in hint;
+  const isFetch = !isXhr
 
   let body: string | undefined;
 
   if (isXhr) {
-    const sentryXhrData = (handlerData as HandlerDataXhr).xhr[SENTRY_XHR_DATA_KEY];
+    const sentryXhrData = hint.xhr[SENTRY_XHR_DATA_KEY];
     body = sentryXhrData && getBodyString(sentryXhrData.body)[0];
   } else if (isFetch) {
-    const sentryFetchData = (handlerData as HandlerDataFetch).fetchData;
-    body = getBodyString(sentryFetchData.body)[0];
+    const sentryFetchData = parseFetchPayload(hint.input);
+    body = getBodyString(sentryFetchData)[0];
   }
 
   return body;
 }
 
+function hasProp<T extends string>(obj: unknown, prop: T): obj is Record<string, string> {
+  return !!obj && typeof obj === 'object' && !!(obj as Record<string, string>)[prop];
+}
+
+/**
+ * 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;
+}
+
+interface GraphQLOperation {
+  operationType: string | undefined;
+  operationName: string | undefined;
+}
+
+/**
+ * Extract the name and type of the operation from the GraphQL query.
+ * @param query
+ */
+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,
+  };
+}
+
+/**
+ * Extract the payload of a request if it's GraphQL.
+ * Exported for tests only.
+ * @param payload - A valid JSON string
+ * @returns A POJO or undefined
+ */
+export function getGraphQLRequestPayload(payload: string): unknown | 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;
+    }
+  } finally {
+    // Fallback to undefined if payload is an invalid JSON (SyntaxError)
+
+    /* eslint-disable no-unsafe-finally */
+    return graphqlBody;
+  }
+}
+
 /**
  * GraphQL Client integration for the browser.
  */
diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts
@@ -33,6 +33,7 @@ import {
   stringMatchesSomePattern,
 } from '@sentry/core';
 import { WINDOW } from '../helpers';
+import { XhrHint } from '@sentry-internal/replay';
 
 /** Options for Request Instrumentation */
 export interface RequestInstrumentationOptions {
@@ -415,7 +416,7 @@ export function xhrCallback(
   }
 
   if (client) {
-    client.emit('beforeOutgoingRequestSpan', span, handlerData);
+    client.emit('beforeOutgoingRequestSpan', span, handlerData as XhrHint);
   }
 
   return span;
diff --git a/packages/browser/test/integrations/graphqlClient.test.ts b/packages/browser/test/integrations/graphqlClient.test.ts
@@ -0,0 +1,94 @@
+import { getGraphQLRequestPayload, parseFetchPayload, parseGraphQLQuery } from "../../src/integrations/graphqlClient";
+
+describe('GraphqlClient', () => {
+    describe('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],
+      [
+        '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 }) }],
+        jsonData,
+      ],
+      [
+        'Request URL & options',
+        [{ url: 'http://example.com' }, { method: 'post', body: JSON.stringify({ data }) }],
+        jsonData,
+      ],
+    ])('%s', (_name, args, expected) => {
+      const actual = parseFetchPayload(args as unknown[]);
+  
+      expect(actual).toEqual(expected);
+    });
+  }); 
+
+  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);
+    });
+  });
+  
+  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/core/src/client.ts b/packages/core/src/client.ts
@@ -14,13 +14,15 @@ import type {
   EventHint,
   EventProcessor,
   FeedbackEvent,
+  FetchBreadcrumbHint,
   HandlerDataFetch,
   HandlerDataXhr,
   Integration,
   MonitorConfig,
   Outcome,
   ParameterizedString,
   SdkMetadata,
+  SentryWrappedXMLHttpRequest,
   Session,
   SessionAggregates,
   SeverityLevel,
@@ -32,6 +34,7 @@ import type {
   TransactionEvent,
   Transport,
   TransportMakeRequestResponse,
+  XhrBreadcrumbHint,
 } from './types-hoist';
 
 import { getEnvelopeEndpointWithUrlEncodedAuth } from './api';
@@ -63,6 +66,17 @@ import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson }
 const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured.";
 const MISSING_RELEASE_FOR_SESSION_ERROR = 'Discarded session because of missing or non-string release';
 
+type RequestBody = null | Blob | BufferSource | FormData | URLSearchParams | string;
+
+export type XhrHint = XhrBreadcrumbHint & {
+    xhr: XMLHttpRequest & SentryWrappedXMLHttpRequest;
+    input?: RequestBody;
+};
+export type FetchHint = FetchBreadcrumbHint & {
+    input: HandlerDataFetch['args'];
+    response: Response;
+};
+
 /**
  * Base implementation for all JavaScript SDK clients.
  *
@@ -584,7 +598,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
   /** @inheritdoc */
   public on(
     hook: 'beforeOutgoingRequestSpan',
-    callback: (span: Span, handlerData: HandlerDataXhr | HandlerDataFetch) => void,
+    callback: (span: Span, hint: XhrHint | FetchHint ) => void,
   ): () => void;
 
   /** @inheritdoc */
@@ -720,7 +734,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
   public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void;
 
   /** @inheritdoc */
-  public emit(hook: 'beforeOutgoingRequestSpan', span: Span, handlerData: HandlerDataXhr | HandlerDataFetch): void;
+  public emit(hook: 'beforeOutgoingRequestSpan', span: Span, hint: XhrHint | FetchHint): void;
 
   /** @inheritdoc */
   public emit(
diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts
@@ -17,6 +17,7 @@ import { parseUrl } from './utils-hoist/url';
 import { hasTracingEnabled } from './utils/hasTracingEnabled';
 import { getActiveSpan } from './utils/spanUtils';
 import { getTraceData } from './utils/traceData';
+import { FetchHint } from './baseclient';
 
 type PolymorphicRequestHeaders =
   | Record<string, string | undefined>
@@ -109,7 +110,8 @@ export function instrumentFetchRequest(
   }
 
   if (client) {
-    client.emit('beforeOutgoingRequestSpan', span, handlerData);
+    const fetchHint = { input: handlerData.args , response: handlerData.response, startTimestamp: handlerData.startTimestamp, endTimestamp: handlerData.endTimestamp } satisfies FetchHint
+    client.emit('beforeOutgoingRequestSpan', span, fetchHint);
   }
 
   return span;
diff --git a/packages/core/src/utils-hoist/instrument/fetch.ts b/packages/core/src/utils-hoist/instrument/fetch.ts
@@ -1,7 +1,6 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import type { HandlerDataFetch } from '../../types-hoist';
 
-import { getGraphQLRequestPayload } from '../graphql';
 import { isError } from '../is';
 import { addNonEnumerableProperty, fill } from '../object';
 import { supportsNativeFetch } from '../supports';
@@ -236,16 +235,3 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str
   };
 }
 
-/**
- * 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/replay-internal/src/index.ts b/packages/replay-internal/src/index.ts
@@ -13,6 +13,8 @@ export type {
   ReplaySpanFrameEvent,
   CanvasManagerInterface,
   CanvasManagerOptions,
+  FetchHint,
+  XhrHint,
 } from './types';
 
 export { getReplay } from './util/getReplay';
diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts
diff --git a/packages/utils/test/instrument/fetch.test.ts b/packages/utils/test/instrument/fetch.test.ts

Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,7 @@ import {`
`33`	`33`	`stringMatchesSomePattern,`
`34`	`34`	`} from '@sentry/core';`
`35`	`35`	`import { WINDOW } from '../helpers';`
	`36`	`+import { XhrHint } from '@sentry-internal/replay';`
`36`	`37`
`37`	`38`	`/** Options for Request Instrumentation */`
`38`	`39`	`export interface RequestInstrumentationOptions {`
`@@ -415,7 +416,7 @@ export function xhrCallback(`
`415`	`416`	`}`
`416`	`417`
`417`	`418`	`if (client) {`
`418`		`- client.emit('beforeOutgoingRequestSpan', span, handlerData);`
	`419`	`+ client.emit('beforeOutgoingRequestSpan', span, handlerData as XhrHint);`
`419`	`420`	`}`
`420`	`421`
`421`	`422`	`return span;`
Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@ import { parseUrl } from './utils-hoist/url';`
`17`	`17`	`import { hasTracingEnabled } from './utils/hasTracingEnabled';`
`18`	`18`	`import { getActiveSpan } from './utils/spanUtils';`
`19`	`19`	`import { getTraceData } from './utils/traceData';`
	`20`	`+import { FetchHint } from './baseclient';`
`20`	`21`
`21`	`22`	`type PolymorphicRequestHeaders =`
`22`	`23`	`\| Record<string, string \| undefined>`
`@@ -109,7 +110,8 @@ export function instrumentFetchRequest(`
`109`	`110`	`}`
`110`	`111`
`111`	`112`	`if (client) {`
`112`		`- client.emit('beforeOutgoingRequestSpan', span, handlerData);`
	`113`	`+ const fetchHint = { input: handlerData.args , response: handlerData.response, startTimestamp: handlerData.startTimestamp, endTimestamp: handlerData.endTimestamp } satisfies FetchHint`
	`114`	`+ client.emit('beforeOutgoingRequestSpan', span, fetchHint);`
`113`	`115`	`}`
`114`	`116`
`115`	`117`	`return span;`