-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Add and use dataCollection.graphQL
#22221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,26 @@ export interface DataCollection { | |
| queryParams?: CollectBehavior; | ||
|
|
||
| /** | ||
| * Controls generative AI input/output recording. | ||
| * Allow collection of GraphQL operation data when using the SDK's GraphQL integrations | ||
| * | ||
| * These options do not add GraphQL instrumentation on their own. The corresponding integration must still be enabled | ||
| * for any data to be captured. | ||
| * | ||
| * - `document`: attach the GraphQL document (query/mutation source text). | ||
| * - `variables`: attach the variables passed to GraphQL operations. | ||
| * @default { document: true, variables: true } | ||
| */ | ||
| graphQL?: { | ||
| document?: boolean; | ||
| variables?: boolean; | ||
| }; | ||
|
Comment on lines
+63
to
+66
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker but something I was discussing with @ericapisani, this creates the expectation that the SDK is going to apply these options to all GraphQL requests which is not accurate. It would only apply if the user is using the client or the server GraphQL integrations. GraphQL requests outside of the integrations coverage won't be covered by these options which I find weird. I know the gen AI spans is already a precedent, so I don't have strong opinions here. An alternative I considered is adding the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The core idea of the top-level option is to have only one place where all of this is defined in an on/off switch manner (like: this is allowed to be sent, but you also need the integration to be enabled). This makes it easier to see it all at once. According to the spec, it's also possible to add it on integration level, which would overwrite the option set on the root-level. Maybe we should make this more clear in the JSDoc?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine, we already have a precedent for it with gen AI stuff so not feeling too strongly about it but it is something that I wanted to bring up is all. Maybe adding a line or two in the JS doc to explain what "it won't do" will help here. |
||
|
|
||
| /** | ||
| * Allow generative AI input/output recording when using the SDK's AI integrations. | ||
| * | ||
| * These options do not add AI instrumentation on their own. The corresponding integration must still be enabled for | ||
| * any data to be captured. Integration-level options, when set, take precedence over these | ||
| * values. | ||
| * @default { inputs: true, outputs: true } | ||
| */ | ||
| genAI?: { | ||
|
|
@@ -77,5 +96,6 @@ export interface DataCollection { | |
| */ | ||
| export type ResolvedDataCollection = Required<DataCollection> & { | ||
| httpHeaders: Required<NonNullable<DataCollection['httpHeaders']>>; | ||
| graphQL: Required<NonNullable<DataCollection['graphQL']>>; | ||
| genAI: Required<NonNullable<DataCollection['genAI']>>; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ export function defaultPiiToCollectionOptions(sendDefaultPii?: boolean): Resolve | |
| httpHeaders: { request: true, response: true }, | ||
| httpBodies: ['incomingRequest', 'outgoingRequest', 'incomingResponse', 'outgoingResponse'], | ||
| queryParams: true, | ||
| graphQL: { document: true, variables: true }, | ||
| genAI: { inputs: true, outputs: true }, | ||
| stackFrameVariables: true, | ||
| frameContextLines: 7, // default should be 5, but ContextLines integration uses 7 | ||
|
|
@@ -26,6 +27,9 @@ export function defaultPiiToCollectionOptions(sendDefaultPii?: boolean): Resolve | |
| httpHeaders: { request: { deny: PII_HEADER_SNIPPETS }, response: { deny: PII_HEADER_SNIPPETS } }, | ||
| httpBodies: [], | ||
| queryParams: { deny: PII_HEADER_SNIPPETS }, | ||
| // The GraphQL document has literal values redacted at collection time, so it was historically | ||
| // always attached regardless of `sendDefaultPii`; keep it on to preserve that behavior. | ||
| graphQL: { document: true, variables: true }, | ||
|
Comment on lines
+30
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: When Suggested FixChange the default setting for Prompt for AI Agent |
||
| genAI: { inputs: false, outputs: false }, | ||
| stackFrameVariables: true, | ||
| frameContextLines: 7, // default should be 5, but ContextLines integration uses 7 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import * as core from '@sentry/core'; | ||
| import { addSpanSource } from '../../../../src/integrations/tracing/graphql/vendored/utils'; | ||
| import type { Location, Token } from '@sentry/server-utils/orchestrion'; | ||
|
|
||
| vi.spyOn(core, 'getClient'); | ||
|
|
||
| function mockClient(graphQLDocument: boolean): void { | ||
| vi.mocked(core.getClient).mockReturnValue({ | ||
| getDataCollectionOptions: () => ({ graphQL: { document: graphQLDocument, variables: true } }), | ||
| } as any); | ||
| } | ||
|
|
||
| function makeLocation(): Location { | ||
| const token: Token = { | ||
| kind: 'Name', | ||
| start: 0, | ||
| end: 5, | ||
| line: 1, | ||
| column: 1, | ||
| value: 'hello', | ||
| prev: null, | ||
| next: null, | ||
| }; | ||
| return { start: 0, end: 5, startToken: token }; | ||
| } | ||
|
|
||
| describe('addSpanSource dataCollection gate', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('sets graphql.source attribute when graphQL.document is true', () => { | ||
| mockClient(true); | ||
| const span = { setAttribute: vi.fn() } as any; | ||
| addSpanSource(span, makeLocation()); | ||
| expect(span.setAttribute).toHaveBeenCalledWith('graphql.source', expect.any(String)); | ||
| }); | ||
|
|
||
| it('does not set attribute when graphQL.document is false', () => { | ||
| mockClient(false); | ||
| const span = { setAttribute: vi.fn() } as any; | ||
| addSpanSource(span, makeLocation()); | ||
| expect(span.setAttribute).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not set attribute when getClient() is undefined', () => { | ||
| vi.mocked(core.getClient).mockReturnValue(undefined); | ||
| const span = { setAttribute: vi.fn() } as any; | ||
| addSpanSource(span, makeLocation()); | ||
| expect(span.setAttribute).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The
graphQL.variablesconfiguration option is defined but never used, making the feature to control GraphQL variable collection non-functional.Severity: HIGH
Suggested Fix
Implement the logic to read the
getDataCollectionOptions().graphQL.variablessetting. Based on its value, conditionally collect or suppress thevariablesfield from GraphQL requests. This logic should be added where GraphQL bodies are processed, such as inpackages/browser/src/integrations/graphqlClient.tsandpackages/server-utils/src/graphql/utils.ts.Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.