-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
ref(server-utils): Streamline graphql integration #23329
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
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,7 @@ | ||
| import * as diagnosticsChannel from 'node:diagnostics_channel'; | ||
| import type { Client, IntegrationFn } from '@sentry/core'; | ||
| import { defineIntegration, extendIntegration } from '@sentry/core'; | ||
| import { graphqlIntegration as graphqlNativeIntegration } from '../../graphql'; | ||
| import type { GraphqlDiagnosticChannelsOptions } from '../../graphql/graphql-dc-subscriber'; | ||
| import type { IntegrationFn } from '@sentry/core'; | ||
| import { defineIntegration, waitForTracingChannelBinding } from '@sentry/core'; | ||
| import { subscribeGraphqlDiagnosticChannels, type GraphQLOptions } from './graphql-dc-subscriber'; | ||
| import { CHANNELS } from '../../orchestrion/channels'; | ||
| import { graphqlModuleNames } from '../../orchestrion/config/graphql'; | ||
| import { invokeOrchestrionInstrumentation } from '../../orchestrion/instrumentation'; | ||
|
|
@@ -28,15 +27,15 @@ interface GraphqlChannelContext { | |
| error?: unknown; | ||
| } | ||
|
|
||
| function getOptionsWithDefaults(options: GraphqlDiagnosticChannelsOptions): GraphqlResolvedConfig { | ||
| function getOptionsWithDefaults(options: GraphQLOptions): GraphqlResolvedConfig { | ||
| return { | ||
| ignoreResolveSpans: options.ignoreResolveSpans !== false, | ||
| ignoreTrivialResolveSpans: options.ignoreTrivialResolveSpans !== false, | ||
| useOperationNameForRootSpan: options.useOperationNameForRootSpan !== false, | ||
| }; | ||
| } | ||
|
|
||
| const _graphqlIntegration = ((options: GraphqlDiagnosticChannelsOptions = {}) => { | ||
| const _graphqlIntegration = ((options: GraphQLOptions = {}) => { | ||
| const config = getOptionsWithDefaults(options); | ||
| const getConfig = (): GraphqlResolvedConfig => config; | ||
|
Comment on lines
37
to
40
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: Suggested FixImplement a recovery mechanism for the native GraphQL instrumentation path, similar to the one used by the orchestrion instrumentation. Instead of a single, immediate retry, the code should listen for an event indicating the async context is ready or use a more robust retry strategy to ensure the subscription to diagnostic channels eventually succeeds. Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
||
|
|
@@ -45,6 +44,9 @@ const _graphqlIntegration = ((options: GraphqlDiagnosticChannelsOptions = {}) => | |
| setup(client) { | ||
| invokeOrchestrionInstrumentation(client, graphqlModuleNames, instrumentGraphql, [config, getConfig]); | ||
| }, | ||
| setupOnce() { | ||
| setupNativeGraphQLInstrumentation(options); | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; | ||
|
|
||
|
|
@@ -66,30 +68,20 @@ function instrumentGraphql(config: GraphqlResolvedConfig, getConfig: () => Graph | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Orchestrion-driven graphql integration for graphql v14–16 (v17 publishes native | ||
| * `diagnostics_channel` events handled by `@sentry/server-utils`'s graphql integration instead). | ||
| * | ||
| * Subscribes to the `orchestrion:graphql:{parse,validate,execute}` channels the orchestrion code | ||
| * transform injects into `graphql`'s `language/parser.js`, `validation/validate.js` and | ||
| * `execution/execute.js`, emitting spans identical to the native path. Requires the orchestrion | ||
| * runtime hook or bundler plugin. | ||
| */ | ||
| export const graphqlIntegration = defineIntegration(_graphqlIntegration); | ||
| function setupNativeGraphQLInstrumentation(options: GraphQLOptions) { | ||
| if (!diagnosticsChannel.tracingChannel) { | ||
| return; | ||
| } | ||
|
|
||
| // Subscribe to graphql's native tracing channels (graphql >= 17). | ||
| // This is a no-op on versions that don't publish to the channels, so it is always safe to call. | ||
| waitForTracingChannelBinding(() => { | ||
| subscribeGraphqlDiagnosticChannels(diagnosticsChannel.tracingChannel, options); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * The complete graphql diagnostics-channel integration: the native subscriber (graphql v17) composed | ||
| * with the orchestrion subscriber (v14–16), so opting into injection instruments every supported | ||
| * version via diagnostics channels without the OTel patcher. Reuses the OTel `Graphql` name so | ||
| * enabling injection swaps this in for it. | ||
| * Instrument the graphql library. | ||
| * This works for graphql v14-v17. | ||
| */ | ||
| export const graphqlDiagnosticsIntegration = (options?: GraphqlDiagnosticChannelsOptions) => { | ||
| const orchestrion = graphqlIntegration(options); | ||
| // The native half is the base integration's own `setupOnce`; the orchestrion half | ||
| // registers lazily via `setup` (only once `graphql` is injected), so it isn't | ||
| // merged onto the base `setupOnce` — both run. | ||
| return extendIntegration(graphqlNativeIntegration(options), { | ||
| name: INTEGRATION_NAME, | ||
| setup: (client: Client) => orchestrion.setup?.(client), | ||
| }); | ||
| }; | ||
| export const graphqlIntegration = defineIntegration(_graphqlIntegration); | ||
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.
Public GraphQL API renamed without deprecation
Medium Severity
graphqlDiagnosticsIntegrationis removed from public Deno and@sentry/server-utils/orchestrionexports and replaced withgraphqlIntegrationwithout a deprecated alias. The main@sentry/server-utilsentry also drops its previousgraphqlIntegrationexport. This is a public API breaking change without a deprecation notice, which the PR review guidelines ask to flag. Nearby Deno renames keep deprecated aliases for the same reason.Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 89a3910. Configure here.
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.
this was not exposed before at all, so all good.