feat(browser): Add `graphqlClientIntegration` #13783

Zen-cronic · 2024-09-25T02:39:03Z

Resolves #13399

Todo:

~~Support fetch spans and tests~~
~~Support shorthand graphql queries (i.e., without a name)~~ Moved to later
~~Enhance breadcrumb data~~

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Added support for graphql query with `xhr` with tests. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

packages/browser/src/integrations/graphqlClient.ts

mydea · 2024-09-25T07:02:01Z

packages/browser/src/tracing/request.ts

@@ -357,6 +357,8 @@ export function xhrCallback(
    return undefined;
  }

+  const requestBody = JSON.parse(sentryXhrData.body as string);


This is dangerous, any such operations we have to try-catch as bodies may not be JSON!

mydea · 2024-09-25T07:04:50Z

packages/browser/src/tracing/request.ts

@@ -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,


h: We should not do this - this will attach the request bodies to all spans, always, which is a) large, b) potentially PII sensitive 😬

Instead of doing this in on('spanStart'), I think we'll need a hook that provides the request or body in some way. I am thinking of a new hook like:

client.on('outgoingRequestSpanStart', (span: Span, { body }: { body: unknown }) => { // ... });

and emit this hook in this file after the span was started 🤔

Noted. I implemented that hook and attached the req payload only to graphql spans.

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 <kaungzinhein113@gmail.com>

packages/utils/src/instrument/fetch.ts

Zen-cronic · 2024-09-26T14:59:27Z

packages/utils/src/instrument/fetch.ts

      const handlerData: HandlerDataFetch = {
        args,
        fetchData: {
          method,
          url,
+          body,


IMO collecting fetch req payload shouldn't be a problem as long as only the graphql requests are sampled. Because xhr instrumentation already collects the payload.

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic · 2024-09-26T16:07:17Z

~~Todo: fix failing fetch tests by removing body from non-graphql requests~~

…r graphql requests Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic · 2024-09-27T02:02:10Z

packages/utils/src/graphql.ts

+    const requestBody = JSON.parse(payload);
+
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+    const isGraphQLRequest = !!requestBody['query'];


This may not be the most accurate and safest way to detect a graphql request?

If we only move this into the graphql integration, instead of runing this for every request, we can IMHO safely make this check, because stuff already ran through the endpoints check, so we can assume it should be fine!

packages/browser/src/integrations/graphqlClient.ts

packages/browser/src/tracing/request.ts

… has started Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic · 2024-10-18T00:46:23Z

Just wanted to inform about the failing tests:

size limit exceeded by 3 bytes! (CDN Bundle (incl. Tracing, Replay))
all the playwright tests related to bundle (e.g., bundle_min, bundle_replay) have been failing with a timeout

dev-packages/browser-integration-tests/suites/integrations/graphqlClient/xhr/test.ts

packages/browser/src/integrations/breadcrumbs.ts

packages/browser/src/tracing/request.ts

packages/browser/src/integrations/graphqlClient.ts

mydea · 2024-12-11T08:19:08Z

packages/browser/src/integrations/graphqlClient.ts

+
+function _getGraphQLOperation(requestBody: unknown): string {
+  // Standard graphql request shape: https://graphql.org/learn/serving-over-http/#post-request
+  const graphqlBody = requestBody as GraphQLRequestPayload;


if we do not know this is a body, we should at least try-catch all of this. But if we (as mentioned in comments above) use getBodyString, we can simply pass this in types as string, as we know it was converted before (or is undefined, in which case, nothing to do)

mydea

I finally found the time to review this in depth. Overall, I really like it, I left some comments with requests for changes. Mostly it is about moving stuff around, and avoiding any graphql-specific code in the "general" places. Also, please note that if you rebase this in develop, there are/will be a bunch of conflicts - this is mostly because the utils package is deprecated and all that code was moved to core.

Zen-cronic · 2024-12-20T03:54:07Z

noted, i'll implement the feedback. thanks for the comprehensive review!

update span hook
update breadcrumb hook
resolve rebase conflicts

will move on with the conflicts if the current implementation checks out

…ific - Updated `outgoingRequestBreadcrumbStart` hook name to `beforeOutgoingRequestBreadcrumb`. - Updated standard graphql request payload structure Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

billyvg · 2025-01-07T16:41:34Z

packages/browser/src/integrations/graphqlClient.ts

@@ -0,0 +1,121 @@
+import { SENTRY_XHR_DATA_KEY } from '@sentry-internal/browser-utils';
+import { getBodyString } from '@sentry-internal/replay';


@mydea should this util be in the replay package?

if we use/need it here, I would move it to @sentry-internal/browser-utils, and use it from there both in replay and here!

otherwise, this will lead to circular dependency issues :)

- Renamed `outgoingRequestSpanStart` hook to `beforeOutgoingRequestSpan`. - Followed Otel semantic for GraphQL Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

- Added guard for missing `httpMethod`. - Refactored getting body based on xhr or fetch logic into a function. - Added Otel semantic in breadcrumb data. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic · 2025-01-07T18:45:15Z

packages/browser/src/integrations/graphqlClient.ts

+
+        if (!data.graphql && graphqlBody) {
+          const operationInfo = _getGraphQLOperation(graphqlBody as GraphQLRequestPayload);
+          data['graphql.document'] = (graphqlBody as GraphQLRequestPayload).query;


currently, these assertions are needed b/c the util getGraphQLRequestPayload doesn't have access to the GraphQLRequestPayload type.

mydea · 2025-01-13T16:01:50Z

packages/browser/src/integrations/graphqlClient.ts

+
+    const isHttpClientSpan = spanOp === 'http.client';
+
+    if (isHttpClientSpan) {


l: I would invert this and just early-return, makes this a bit easier to read IMHO :) so:

if (!isHttpClientSpan) { return; } // rest of logic here...

mydea · 2025-01-13T16:05:05Z

packages/types/src/client.ts

+   */
+  on(
+    hook: 'beforeOutgoingRequestSpan',
+    callback: (span: Span, handlerData: HandlerDataXhr | HandlerDataFetch) => void,


m: Let's instead type this like this:

Suggested change

callback: (span: Span, handlerData: HandlerDataXhr | HandlerDataFetch) => void,

callback: (span: Span, handlerData: FetchBreadcrumbHint | XhrBreadcrumbHint) => void,

So give it the (typed) hint instead. this should be a bit more "stable" then the handler data, and the relevant handler data should be part of the hint instead?

mydea · 2025-01-13T16:05:22Z

packages/types/src/client.ts

+   */
+  on(
+    hook: 'beforeOutgoingRequestBreadcrumb',
+    callback: (breadcrumb: Breadcrumb, handlerData: HandlerDataXhr | HandlerDataFetch) => void,


same here:

Suggested change

callback: (breadcrumb: Breadcrumb, handlerData: HandlerDataXhr | HandlerDataFetch) => void,

callback: (breadcrumb: Breadcrumb, handlerData: FetchBreadcrumbHint | XhrBreadcrumbHint) => void,

mydea · 2025-01-13T16:06:05Z

packages/browser/src/integrations/graphqlClient.ts

+        }
+
+        // The body prop attached to HandlerDataFetch for the span should be removed.
+        if (isFetch && data.body) {


do we need to unset this? what happens if we do not do that?

mydea · 2025-01-13T16:10:09Z

packages/utils/src/instrument/fetch.ts

@@ -58,6 +59,15 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
        startTimestamp: timestampInSeconds() * 1000,
      };

+      const body = parseFetchPayload(args);


The main work that IMHO is left is to remove this code here, and to instead do this in the integration itself. Through the hint you should have access to the args, so you should be able to get the payload in there directly (in a hook) instead of doing this generically for all requests.

Then, you can inline the getGraphQLRequestPayload code too into the integration itself, which should make typing etc. stuff much easier. Ideally, this file is not touched at all, and the only thing that is added outside of the integration is the hooks configuration - everything else (more or less) is encapsulated in the integration :)

mydea · 2025-01-13T16:10:30Z

I looked over it again, apart from some rebase stuff that is sadly going to have to happen 😬 (e.g. the client type is now gone and just part of core/client now, @sentry/types is gone and types should just be imported from @sentry/core now, ...), overall I want to stress that this is a really cool PR!

This: https://github.com/getsentry/sentry-javascript/pull/13783/files#r1913443495 is the main thing left to do IMHO - if you need more help/pointers please let us know!

feat(browser): Add graphqlClientIntegration

aa0b588

Added support for graphql query with `xhr` with tests. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

mydea reviewed Sep 25, 2024

View reviewed changes

packages/browser/src/integrations/graphqlClient.ts Outdated Show resolved Hide resolved

mydea reviewed Sep 25, 2024

View reviewed changes

packages/browser/src/integrations/graphqlClient.ts Outdated Show resolved Hide resolved

mydea reviewed Sep 25, 2024

View reviewed changes

feat(browser): Add support for fetch graphql request

4614a55

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 <kaungzinhein113@gmail.com>

Zen-cronic commented Sep 26, 2024

View reviewed changes

packages/utils/src/instrument/fetch.ts Outdated Show resolved Hide resolved

Zen-cronic commented Sep 26, 2024

View reviewed changes

Zen-cronic requested a review from mydea September 26, 2024 15:05

test(browser): Remove skip test

77c9fbc

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

fix(browser): Attach request payload to fetch instrumentation only fo…

20b6516

…r graphql requests Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic commented Sep 27, 2024

View reviewed changes

mydea reviewed Sep 27, 2024

View reviewed changes

packages/browser/src/integrations/graphqlClient.ts Outdated Show resolved Hide resolved

mydea reviewed Sep 27, 2024

View reviewed changes

packages/browser/src/tracing/request.ts Outdated Show resolved Hide resolved

Zen-cronic added 2 commits September 27, 2024 11:57

fix(browser): Emit the outgoingRequestSpanStart hook after the span…

a304218

… has started Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

feat(browser): Update breadcrumbs with graphql request data

55e6f1b

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic requested a review from mydea September 30, 2024 13:02