From c12ba6c92b2802e61f963f6a213148edd771a507 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 9 Apr 2025 11:58:40 +0200 Subject: [PATCH] fix(node): Ensure late init works with all integrations In cases where we tweak the options we pass to the instrumentation, our code did not handle this very well. This could lead to certain options missing when init is called again later, as we updated the config with incomplete/wrong options. To fix this properly, I added a new variant to `generateInstrumentOnce` which accepts a class and an options transformer, which ensures that the options are always correctly transformed, no matter if called the first or second time. --- .../src/integration/awslambda.ts | 13 ++-- .../node/src/integrations/node-fetch/index.ts | 21 +++--- .../node/src/integrations/tracing/graphql.ts | 9 ++- packages/node/src/otel/instrument.ts | 73 ++++++++++++++++++- .../src/server/integrations/opentelemetry.ts | 20 ++--- 5 files changed, 102 insertions(+), 34 deletions(-) diff --git a/packages/aws-serverless/src/integration/awslambda.ts b/packages/aws-serverless/src/integration/awslambda.ts index 61776daed18c..2ba148d8b165 100644 --- a/packages/aws-serverless/src/integration/awslambda.ts +++ b/packages/aws-serverless/src/integration/awslambda.ts @@ -15,22 +15,19 @@ interface AwsLambdaOptions { disableAwsContextPropagation?: boolean; } -export const instrumentAwsLambda = generateInstrumentOnce( +export const instrumentAwsLambda = generateInstrumentOnce( 'AwsLambda', - (_options: AwsLambdaOptions = {}) => { - const options = { + AwsLambdaInstrumentation, + (options: AwsLambdaOptions) => { + return { disableAwsContextPropagation: true, - ..._options, - }; - - return new AwsLambdaInstrumentation({ ...options, eventContextExtractor, requestHook(span) { span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.otel.aws-lambda'); span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'function.aws.lambda'); }, - }); + }; }, ); diff --git a/packages/node/src/integrations/node-fetch/index.ts b/packages/node/src/integrations/node-fetch/index.ts index dc0df9b5ef57..cfcc93f1881e 100644 --- a/packages/node/src/integrations/node-fetch/index.ts +++ b/packages/node/src/integrations/node-fetch/index.ts @@ -5,7 +5,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, defineIntegration, getClient } from ' import { generateInstrumentOnce } from '../../otel/instrument'; import type { NodeClient } from '../../sdk/client'; import type { NodeClientOptions } from '../../types'; -import type { SentryNodeFetchInstrumentationOptions } from './SentryNodeFetchInstrumentation'; import { SentryNodeFetchInstrumentation } from './SentryNodeFetchInstrumentation'; const INTEGRATION_NAME = 'NodeFetch'; @@ -33,14 +32,19 @@ interface NodeFetchOptions { ignoreOutgoingRequests?: (url: string) => boolean; } -const instrumentOtelNodeFetch = generateInstrumentOnce(INTEGRATION_NAME, config => { - return new UndiciInstrumentation(config); -}); +const instrumentOtelNodeFetch = generateInstrumentOnce( + INTEGRATION_NAME, + UndiciInstrumentation, + (options: NodeFetchOptions) => { + return getConfigWithDefaults(options); + }, +); -const instrumentSentryNodeFetch = generateInstrumentOnce( +const instrumentSentryNodeFetch = generateInstrumentOnce( `${INTEGRATION_NAME}.sentry`, - config => { - return new SentryNodeFetchInstrumentation(config); + SentryNodeFetchInstrumentation, + (options: NodeFetchOptions) => { + return options; }, ); @@ -52,8 +56,7 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { // This is the "regular" OTEL instrumentation that emits spans if (instrumentSpans) { - const instrumentationConfig = getConfigWithDefaults(options); - instrumentOtelNodeFetch(instrumentationConfig); + instrumentOtelNodeFetch(options); } // This is the Sentry-specific instrumentation that creates breadcrumbs & propagates traces diff --git a/packages/node/src/integrations/tracing/graphql.ts b/packages/node/src/integrations/tracing/graphql.ts index 945327064df2..dbcbe20dcc40 100644 --- a/packages/node/src/integrations/tracing/graphql.ts +++ b/packages/node/src/integrations/tracing/graphql.ts @@ -37,12 +37,13 @@ interface GraphqlOptions { const INTEGRATION_NAME = 'Graphql'; -export const instrumentGraphql = generateInstrumentOnce( +export const instrumentGraphql = generateInstrumentOnce( INTEGRATION_NAME, - (_options: GraphqlOptions = {}) => { + GraphQLInstrumentation, + (_options: GraphqlOptions) => { const options = getOptionsWithDefaults(_options); - return new GraphQLInstrumentation({ + return { ...options, responseHook(span) { addOriginToSpan(span, 'auto.graphql.otel.graphql'); @@ -73,7 +74,7 @@ export const instrumentGraphql = generateInstrumentOnce( } } }, - }); + }; }, ); diff --git a/packages/node/src/otel/instrument.ts b/packages/node/src/otel/instrument.ts index 6f8b10db2ba7..c5e94991140a 100644 --- a/packages/node/src/otel/instrument.ts +++ b/packages/node/src/otel/instrument.ts @@ -3,16 +3,47 @@ import { type Instrumentation, registerInstrumentations } from '@opentelemetry/i /** Exported only for tests. */ export const INSTRUMENTED: Record = {}; -/** - * Instrument an OpenTelemetry instrumentation once. - * This will skip running instrumentation again if it was already instrumented. - */ +export function generateInstrumentOnce< + Options, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + InstrumentationClass extends new (...args: any[]) => Instrumentation, +>( + name: string, + instrumentationClass: InstrumentationClass, + optionsCallback: (options: Options) => ConstructorParameters[0], +): ((options: Options) => InstanceType) & { id: string }; export function generateInstrumentOnce< Options = unknown, InstrumentationInstance extends Instrumentation = Instrumentation, >( name: string, creator: (options?: Options) => InstrumentationInstance, +): ((options?: Options) => InstrumentationInstance) & { id: string }; +/** + * Instrument an OpenTelemetry instrumentation once. + * This will skip running instrumentation again if it was already instrumented. + */ +export function generateInstrumentOnce( + name: string, + creatorOrClass: (new (...args: any[]) => Instrumentation) | ((options?: Options) => Instrumentation), + optionsCallback?: (options: Options) => unknown, +): ((options: Options) => Instrumentation) & { id: string } { + if (optionsCallback) { + return _generateInstrumentOnceWithOptions( + name, + creatorOrClass as new (...args: unknown[]) => Instrumentation, + optionsCallback, + ); + } + + return _generateInstrumentOnce(name, creatorOrClass as (options?: Options) => Instrumentation); +} + +// The plain version without handling of options +// Should not be used with custom options that are mutated in the creator! +function _generateInstrumentOnce( + name: string, + creator: (options?: Options) => InstrumentationInstance, ): ((options?: Options) => InstrumentationInstance) & { id: string } { return Object.assign( (options?: Options) => { @@ -38,6 +69,40 @@ export function generateInstrumentOnce< ); } +// This version handles options properly +function _generateInstrumentOnceWithOptions< + Options, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + InstrumentationClass extends new (...args: any[]) => Instrumentation, +>( + name: string, + instrumentationClass: InstrumentationClass, + optionsCallback: (options: Options) => ConstructorParameters[0], +): ((options: Options) => InstanceType) & { id: string } { + return Object.assign( + (_options: Options) => { + const options = optionsCallback(_options); + + const instrumented = INSTRUMENTED[name] as InstanceType | undefined; + if (instrumented) { + // Ensure we update options + instrumented.setConfig(options); + return instrumented; + } + + const instrumentation = new instrumentationClass(options) as InstanceType; + INSTRUMENTED[name] = instrumentation; + + registerInstrumentations({ + instrumentations: [instrumentation], + }); + + return instrumentation; + }, + { id: name }, + ); +} + /** * Ensure a given callback is called when the instrumentation is actually wrapping something. * This can be used to ensure some logic is only called when the instrumentation is actually active. diff --git a/packages/remix/src/server/integrations/opentelemetry.ts b/packages/remix/src/server/integrations/opentelemetry.ts index 7ba99421c82f..dac05ed89d33 100644 --- a/packages/remix/src/server/integrations/opentelemetry.ts +++ b/packages/remix/src/server/integrations/opentelemetry.ts @@ -7,22 +7,24 @@ import type { RemixOptions } from '../../utils/remixOptions'; const INTEGRATION_NAME = 'Remix'; -const instrumentRemix = generateInstrumentOnce( - INTEGRATION_NAME, - (_options?: RemixOptions) => - new RemixInstrumentation({ - actionFormDataAttributes: _options?.sendDefaultPii ? _options?.captureActionFormDataKeys : undefined, - }), -); +interface RemixInstrumentationOptions { + actionFormDataAttributes?: Record; +} + +const instrumentRemix = generateInstrumentOnce(INTEGRATION_NAME, (options?: RemixInstrumentationOptions) => { + return new RemixInstrumentation(options); +}); const _remixIntegration = (() => { return { name: 'Remix', setupOnce() { const client = getClient(); - const options = client?.getOptions(); + const options = client?.getOptions() as RemixOptions | undefined; - instrumentRemix(options); + instrumentRemix({ + actionFormDataAttributes: options?.sendDefaultPii ? options?.captureActionFormDataKeys : undefined, + }); }, setup(client: Client) {