From bd9f4cc731f067ab8e5459de70426d84bb014722 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 24 Sep 2024 16:57:44 +0200 Subject: [PATCH 1/3] fix(node): Ensure graphql options are correct when preloading Noticed this by chance, due to the way `generateInstrumentOnce` works, we need to pass in the fully formed config. --- packages/node/src/integrations/tracing/graphql.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/graphql.ts b/packages/node/src/integrations/tracing/graphql.ts index 914653ac745c..bd596c87496a 100644 --- a/packages/node/src/integrations/tracing/graphql.ts +++ b/packages/node/src/integrations/tracing/graphql.ts @@ -40,6 +40,8 @@ const INTEGRATION_NAME = 'Graphql'; export const instrumentGraphql = generateInstrumentOnce( INTEGRATION_NAME, (_options: GraphqlOptions = {}) => { + // We set default values here, which are used in the case that this is preloaded + // In that case, no options are passed in, and we want to use the defaults const options = { ignoreResolveSpans: true, ignoreTrivialResolveSpans: true, @@ -85,10 +87,20 @@ export const instrumentGraphql = generateInstrumentOnce( }, ); -const _graphqlIntegration = ((options: GraphqlOptions = {}) => { +const _graphqlIntegration = ((_options: GraphqlOptions = {}) => { return { name: INTEGRATION_NAME, setupOnce() { + // We set defaults here, too, because otherwise we'd update the instrumentation config + // to the config without defaults, as `generateInstrumentOnce` automatically calls `setConfig(options)` + // when being called the second time + const options = { + ignoreResolveSpans: true, + ignoreTrivialResolveSpans: true, + useOperationNameForRootSpan: true, + ..._options, + }; + instrumentGraphql(options); }, }; From 6de209af0b36da28efea18048be3f25b23151846 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 24 Sep 2024 16:59:25 +0200 Subject: [PATCH 2/3] refactor --- .../node/src/integrations/tracing/graphql.ts | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/packages/node/src/integrations/tracing/graphql.ts b/packages/node/src/integrations/tracing/graphql.ts index bd596c87496a..f4d817145cf2 100644 --- a/packages/node/src/integrations/tracing/graphql.ts +++ b/packages/node/src/integrations/tracing/graphql.ts @@ -40,14 +40,7 @@ const INTEGRATION_NAME = 'Graphql'; export const instrumentGraphql = generateInstrumentOnce( INTEGRATION_NAME, (_options: GraphqlOptions = {}) => { - // We set default values here, which are used in the case that this is preloaded - // In that case, no options are passed in, and we want to use the defaults - const options = { - ignoreResolveSpans: true, - ignoreTrivialResolveSpans: true, - useOperationNameForRootSpan: true, - ..._options, - }; + const options = getOptionsWithDefaults(_options); return new GraphQLInstrumentation({ ...options, @@ -87,21 +80,14 @@ export const instrumentGraphql = generateInstrumentOnce( }, ); -const _graphqlIntegration = ((_options: GraphqlOptions = {}) => { +const _graphqlIntegration = ((options: GraphqlOptions = {}) => { return { name: INTEGRATION_NAME, setupOnce() { // We set defaults here, too, because otherwise we'd update the instrumentation config // to the config without defaults, as `generateInstrumentOnce` automatically calls `setConfig(options)` // when being called the second time - const options = { - ignoreResolveSpans: true, - ignoreTrivialResolveSpans: true, - useOperationNameForRootSpan: true, - ..._options, - }; - - instrumentGraphql(options); + instrumentGraphql(getOptionsWithDefaults(options)); }, }; }) satisfies IntegrationFn; @@ -112,3 +98,12 @@ const _graphqlIntegration = ((_options: GraphqlOptions = {}) => { * Capture tracing data for GraphQL. */ export const graphqlIntegration = defineIntegration(_graphqlIntegration); + +function getOptionsWithDefaults(options?: GraphqlOptions): GraphqlOptions { + return { + ignoreResolveSpans: true, + ignoreTrivialResolveSpans: true, + useOperationNameForRootSpan: true, + ...options, + }; +} From 6e05169477f27de072f6192fcc08f79b2fdf2bec Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 8 Oct 2024 13:29:19 +0200 Subject: [PATCH 3/3] add test --- packages/node/src/otel/instrument.ts | 3 +- .../test/integrations/tracing/graphql.test.ts | 46 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 packages/node/test/integrations/tracing/graphql.test.ts diff --git a/packages/node/src/otel/instrument.ts b/packages/node/src/otel/instrument.ts index 5db7960e4019..6db677a3956c 100644 --- a/packages/node/src/otel/instrument.ts +++ b/packages/node/src/otel/instrument.ts @@ -1,7 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; -const INSTRUMENTED: Record = {}; +/** Exported only for tests. */ +export const INSTRUMENTED: Record = {}; /** * Instrument an OpenTelemetry instrumentation once. diff --git a/packages/node/test/integrations/tracing/graphql.test.ts b/packages/node/test/integrations/tracing/graphql.test.ts new file mode 100644 index 000000000000..779b8dace830 --- /dev/null +++ b/packages/node/test/integrations/tracing/graphql.test.ts @@ -0,0 +1,46 @@ +import { GraphQLInstrumentation } from '@opentelemetry/instrumentation-graphql'; +import { graphqlIntegration, instrumentGraphql } from '../../../src/integrations/tracing/graphql'; +import { INSTRUMENTED } from '../../../src/otel/instrument'; + +jest.mock('@opentelemetry/instrumentation-graphql'); + +describe('GraphQL', () => { + beforeEach(() => { + jest.clearAllMocks(); + delete INSTRUMENTED.Graphql; + + (GraphQLInstrumentation as unknown as jest.SpyInstance).mockImplementation(() => { + return { + setTracerProvider: () => undefined, + setMeterProvider: () => undefined, + getConfig: () => ({}), + setConfig: () => ({}), + enable: () => undefined, + }; + }); + }); + + it('defaults are correct for instrumentGraphql', () => { + instrumentGraphql({ ignoreTrivialResolveSpans: false }); + + expect(GraphQLInstrumentation).toHaveBeenCalledTimes(1); + expect(GraphQLInstrumentation).toHaveBeenCalledWith({ + ignoreResolveSpans: true, + ignoreTrivialResolveSpans: false, + useOperationNameForRootSpan: true, + responseHook: expect.any(Function), + }); + }); + + it('defaults are correct for _graphqlIntegration', () => { + graphqlIntegration({ ignoreTrivialResolveSpans: false }).setupOnce!(); + + expect(GraphQLInstrumentation).toHaveBeenCalledTimes(1); + expect(GraphQLInstrumentation).toHaveBeenCalledWith({ + ignoreResolveSpans: true, + ignoreTrivialResolveSpans: false, + useOperationNameForRootSpan: true, + responseHook: expect.any(Function), + }); + }); +});