Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/deno/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export {
firebaseIntegration,
genericPoolIntegration,
googleGenAIIntegration,
graphqlDiagnosticsIntegration,
graphqlIntegration,

Copy link
Copy Markdown

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

graphqlDiagnosticsIntegration is removed from public Deno and @sentry/server-utils/orchestrion exports and replaced with graphqlIntegration without a deprecated alias. The main @sentry/server-utils entry also drops its previous graphqlIntegration export. 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.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 89a3910. Configure here.

Copy link
Copy Markdown
Member Author

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.

hapiIntegration,
kafkajsIntegration,
knexIntegration,
Expand Down
4 changes: 2 additions & 2 deletions packages/deno/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
firebaseIntegration,
genericPoolIntegration,
googleGenAIIntegration,
graphqlDiagnosticsIntegration,
graphqlIntegration,
hapiIntegration,
kafkajsIntegration,
koaIntegration,
Expand Down Expand Up @@ -65,7 +65,7 @@ export function getDefaultIntegrations(_options: Options): Integration[] {
denoServeIntegration(),
denoHttpIntegration(),
denoRedisIntegration(),
graphqlDiagnosticsIntegration(),
graphqlIntegration(),
vercelAiIntegration(),
// orchestrion-based instrumentations. We add a deliberate list here rather
// than every channel integration: each one needs a Deno test proving it
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export {
firebaseIntegration,
genericPoolIntegration,
googleGenAIIntegration,
graphqlDiagnosticsIntegration as graphqlIntegration,
graphqlIntegration,
hapiIntegration,
kafkajsIntegration as kafkaIntegration,
knexIntegration,
Expand Down
4 changes: 2 additions & 2 deletions packages/node/src/integrations/tracing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
firebaseIntegration,
genericPoolIntegration,
googleGenAIIntegration,
graphqlDiagnosticsIntegration,
graphqlIntegration,
hapiIntegration,
kafkajsIntegration,
koaIntegration,
Expand All @@ -31,7 +31,7 @@ export function getAutoPerformanceIntegrations(): Integration[] {
return [
expressIntegration(),
fastifyIntegration(),
graphqlDiagnosticsIntegration(),
graphqlIntegration(),
mongodbIntegration(),
mongooseIntegration(),
mysqlIntegration(),
Expand Down
30 changes: 0 additions & 30 deletions packages/server-utils/src/graphql/index.ts

This file was deleted.

1 change: 0 additions & 1 deletion packages/server-utils/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
export * from './exports';

// Exports using diagnostics channels
export { graphqlIntegration } from './graphql';
export { mongooseIntegration } from './mongoose';
export { mysql2Integration } from './mysql2';
export { instrumentPrisma, prismaIntegration } from './prisma';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import {
SPAN_STATUS_ERROR,
startInactiveSpan,
} from '@sentry/core';
import { bindTracingChannelToSpan } from '../tracing-channel';
import type { GraphqlDocumentNode } from './utils';
import { bindTracingChannelToSpan } from '../../tracing-channel';
import type { GraphqlDocumentNode } from './types';
import { collectGraphqlDocument, getOperationSpanName, hasResultErrors, renameRootSpanWithOperation } from './utils';

// Channel names published by graphql >= 17.0.0 (see graphql-js `src/diagnostics.ts`).
Expand Down Expand Up @@ -85,7 +85,7 @@ export interface GraphqlResolveData {
}

/** Options controlling which graphql channels the subscriber emits spans for. */
export interface GraphqlDiagnosticChannelsOptions {
export interface GraphQLOptions {
/**
* Do not create spans for resolvers. Resolver spans are per-field and can be very high volume.
* Defaults to `true`.
Expand Down Expand Up @@ -128,7 +128,7 @@ export type GraphqlTracingChannelFactory = <T extends object>(name: string) => T
*/
export function subscribeGraphqlDiagnosticChannels(
tracingChannel: GraphqlTracingChannelFactory,
options: GraphqlDiagnosticChannelsOptions = {},
options: GraphQLOptions = {},
): void {
const ignoreResolveSpans = options.ignoreResolveSpans !== false;
const ignoreTrivialResolveSpans = options.ignoreTrivialResolveSpans !== false;
Expand Down
52 changes: 22 additions & 30 deletions packages/server-utils/src/integrations/graphql/index.ts
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';
Expand All @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: setupOnce calls waitForTracingChannelBinding which may fail if the async context isn't ready during SDK init. The native GraphQL instrumentation will then silently fail without any recovery mechanism.
Severity: HIGH

Suggested Fix

Implement 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 Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/server-utils/src/integrations/graphql/index.ts#L37-L40

Potential issue: The `graphqlIntegration.setupOnce()` method calls
`setupNativeGraphQLInstrumentation()`, which uses `waitForTracingChannelBinding()` to
subscribe to native GraphQL diagnostic channels. This subscription can fail if the async
context strategy is not yet configured when `setupOnce()` is executed during SDK
initialization. The `waitForTracingChannelBinding()` function only retries once before
failing silently. Unlike the orchestrion instrumentation path, this new native path
lacks a recovery mechanism to re-attempt the subscription later. Consequently, for
GraphQL v17+, instrumentation may be silently disabled without any indication to the
user, leading to a loss of tracing data.

Did we get this right? 👍 / 👎 to inform future reviews.


Expand All @@ -45,6 +44,9 @@ const _graphqlIntegration = ((options: GraphqlDiagnosticChannelsOptions = {}) =>
setup(client) {
invokeOrchestrionInstrumentation(client, graphqlModuleNames, instrumentGraphql, [config, getConfig]);
},
setupOnce() {
setupNativeGraphQLInstrumentation(options);
},
};
}) satisfies IntegrationFn;

Expand All @@ -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);
9 changes: 2 additions & 7 deletions packages/server-utils/src/integrations/graphql/spans.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,8 @@ import {
SPAN_STATUS_ERROR,
startInactiveSpan,
} from '@sentry/core';
import type { GraphqlDocumentNode } from '../../graphql/utils';
import {
collectGraphqlDocument,
getOperationSpanName,
hasResultErrors,
renameRootSpanWithOperation,
} from '../../graphql/utils';
import type { GraphqlDocumentNode } from './types';
import { collectGraphqlDocument, getOperationSpanName, hasResultErrors, renameRootSpanWithOperation } from './utils';
import { GRAPHQL_DATA_SYMBOL, ORIGIN, SPAN_NAME_EXECUTE, SPAN_NAME_PARSE, SPAN_NAME_VALIDATE } from './constants';
import { getOperation, wrapFields, wrapFieldResolver } from './resolvers';
import type {
Expand Down
16 changes: 16 additions & 0 deletions packages/server-utils/src/integrations/graphql/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,19 @@ export interface GraphqlResolvedConfig {
ignoreTrivialResolveSpans: boolean;
useOperationNameForRootSpan: boolean;
}

/** Minimal shape of a graphql-js lexer token, enough to locate literal spans for redaction. */
export interface GraphqlToken {
kind: string;
start: number;
end: number;
next?: GraphqlToken | null;
}

/** Minimal shape of a parsed graphql-js `DocumentNode`, enough to read its source and tokens. */
export interface GraphqlDocumentNode {
loc?: {
startToken?: GraphqlToken;
source?: { body?: string };
};
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { SENTRY_GRAPHQL_OPERATION } from '@sentry/conventions/attributes';
import type { Span, SpanAttributeValue } from '@sentry/core';
import { getClient, isObjectLike, getRootSpan, spanToJSON, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
import type { GraphqlDocumentNode, GraphqlToken } from './types';

// Same key the OTel path uses, so renames stay consistent across both.
const ORIGINAL_DESCRIPTION_ATTRIBUTE = 'original-description';
Expand All @@ -10,22 +11,6 @@ const ORIGINAL_DESCRIPTION_ATTRIBUTE = 'original-description';
// `graphql.document`. Mirrors the legacy OTel instrumentation's redaction set.
const REDACTED_LITERAL_KINDS = new Set(['Int', 'Float', 'String', 'BlockString']);

/** Minimal shape of a graphql-js lexer token, enough to locate literal spans for redaction. */
interface GraphqlToken {
kind: string;
start: number;
end: number;
next?: GraphqlToken | null;
}

/** Minimal shape of a parsed graphql-js `DocumentNode`, enough to read its source and tokens. */
export interface GraphqlDocumentNode {
loc?: {
startToken?: GraphqlToken;
source?: { body?: string };
};
}

/**
* Rename the enclosing root span to include the operation name(s), e.g. `GET /graphql (query GetUser)`.
*/
Expand Down
5 changes: 2 additions & 3 deletions packages/server-utils/src/orchestrion/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { awsIntegration } from '../integrations/aws-sdk';
import { dataloaderIntegration } from '../integrations/dataloader';
import { genericPoolIntegration } from '../integrations/generic-pool';
import { googleGenAIIntegration } from '../integrations/google-genai';
import { graphqlIntegration, graphqlDiagnosticsIntegration } from '../integrations/graphql';
import { graphqlIntegration } from '../integrations/graphql';
import { hapiIntegration } from '../integrations/hapi';
import { koaIntegration } from '../integrations/koa';
import { ioredisChannelIntegration } from '../integrations/ioredis';
Expand Down Expand Up @@ -44,7 +44,6 @@ export {
genericPoolIntegration,
googleGenAIIntegration,
graphqlIntegration,
graphqlDiagnosticsIntegration,
hapiIntegration,
koaIntegration,
ioredisChannelIntegration,
Expand Down Expand Up @@ -115,7 +114,7 @@ export const channelIntegrations = {
hapiIntegration,
koaIntegration,
expressIntegration,
graphqlIntegration: graphqlDiagnosticsIntegration,
graphqlIntegration,
kafkajsIntegration,
tediousIntegration,
awsIntegration,
Expand Down
4 changes: 2 additions & 2 deletions packages/server-utils/test/graphql/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import * as core from '@sentry/core';
import { collectGraphqlDocument } from '../../src/graphql/utils';
import type { GraphqlDocumentNode } from '../../src/graphql/utils';
import { collectGraphqlDocument } from '../../src/integrations/graphql/utils';
import type { GraphqlDocumentNode } from '../../src/integrations/graphql/types';

vi.spyOn(core, 'getClient');

Expand Down
Loading