-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Draft PR: Effect Integration for Sentry Node SDK #17432
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
base: develop
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| import type { IntegrationFn } from '@sentry/core'; | ||
| import { captureException, defineIntegration, getCurrentScope, startSpan, withScope } from '@sentry/core'; | ||
| import type { EffectExit, EffectModule, EffectSpan, EffectTracer } from './types'; | ||
|
|
||
| const INTEGRATION_NAME = 'Effect'; | ||
|
|
||
| // Global state to track if tracing is enabled | ||
| let isInstrumentationEnabled = false; | ||
| let originalTracer: EffectTracer | undefined; | ||
|
|
||
| /** | ||
| * Instruments Effect spans to create Sentry spans. | ||
| */ | ||
| export const instrumentEffect = Object.assign( | ||
| (): void => { | ||
| if (isInstrumentationEnabled) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const Effect = require('effect') as EffectModule; | ||
|
|
||
| if (!Effect?.Tracer) { | ||
| return; | ||
| } | ||
|
|
||
| // Store the original tracer if it exists | ||
| originalTracer = Effect.Tracer.get?.() || Effect.Tracer.current?.() || undefined; | ||
|
|
||
| // Create our custom tracer that wraps operations in Sentry spans | ||
| const sentryTracer: EffectTracer = { | ||
| onSpanStart(span: EffectSpan) { | ||
| // Hook for span start - can be used for additional instrumentation | ||
| if (originalTracer?.onSpanStart) { | ||
| originalTracer.onSpanStart(span); | ||
| } | ||
| }, | ||
|
|
||
| onSpanEnd(span: EffectSpan, exit: EffectExit) { | ||
| // Hook for span end - handle failures | ||
| if (exit._tag === 'Failure' && exit.cause) { | ||
| withScope(scope => { | ||
| scope.setTag('effect.exit_tag', exit._tag); | ||
| scope.setContext('effect.span', { | ||
| name: span.name, | ||
| startTime: Number(span.startTime), | ||
| endTime: span.endTime ? Number(span.endTime) : undefined, | ||
| }); | ||
| captureException(exit.cause); | ||
| }); | ||
| } | ||
|
|
||
| if (originalTracer?.onSpanEnd) { | ||
| originalTracer.onSpanEnd(span, exit); | ||
| } | ||
| }, | ||
|
|
||
| span<A>(name: string, f: () => A): A { | ||
| return startSpan( | ||
| { | ||
| name, | ||
| op: 'effect.span', | ||
| origin: 'auto.effect', | ||
|
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. Span origin ignored because passed as wrong propertyMedium Severity The Additional Locations (1) |
||
| }, | ||
| () => { | ||
| try { | ||
| return f(); | ||
| } catch (error) { | ||
| const scope = getCurrentScope(); | ||
| scope.setTag('effect.error', true); | ||
| captureException(error); | ||
| throw error; | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
|
|
||
| withSpan<A>(span: EffectSpan, f: () => A): A { | ||
| return startSpan( | ||
| { | ||
| name: span.name, | ||
| op: 'effect.span', | ||
| origin: 'auto.effect', | ||
| startTime: Number(span.startTime) / 1000000, // Convert nanoseconds to milliseconds | ||
|
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: The Effect integration incorrectly converts span timestamps from nanoseconds to milliseconds instead of seconds, causing all reported span timings to be off by a factor of 1000. Suggested FixUpdate the timestamp conversion logic to divide the nanosecond value by Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
| data: span.attributes, | ||
|
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. Span attributes silently ignored due to wrong property nameMedium Severity The |
||
| }, | ||
| sentrySpan => { | ||
| try { | ||
| const result = f(); | ||
|
|
||
| // Set status based on span status | ||
| if (span.status && span.status.code !== 0) { | ||
| sentrySpan.setStatus('internal_error'); | ||
|
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. setStatus called with incorrect argument typeHigh Severity The Additional Locations (1) |
||
| if (span.status.message) { | ||
| sentrySpan.setData('effect.status_message', span.status.message); | ||
|
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. |
||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } catch (error) { | ||
| sentrySpan.setStatus('internal_error'); | ||
| const scope = getCurrentScope(); | ||
| scope.setTag('effect.error', true); | ||
| captureException(error); | ||
| throw error; | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| }; | ||
|
|
||
| // Register our tracer with Effect | ||
| if (typeof Effect.Tracer.set === 'function') { | ||
| Effect.Tracer.set(sentryTracer); | ||
| } else if (typeof Effect.Tracer.register === 'function') { | ||
| Effect.Tracer.register(sentryTracer); | ||
| } else if (typeof Effect.Tracer.use === 'function') { | ||
| Effect.Tracer.use(sentryTracer); | ||
| } else { | ||
| return; | ||
| } | ||
|
Comment on lines
+114
to
+122
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. Where did you find these methods on the effect tracer? I do not think they exist. |
||
|
|
||
| isInstrumentationEnabled = true; | ||
| } catch (error) { | ||
| // Silent failure - Effect may not be available | ||
| } | ||
| }, | ||
| { id: INTEGRATION_NAME }, | ||
| ); | ||
|
|
||
| const _effectIntegration = (() => { | ||
| return { | ||
| name: INTEGRATION_NAME, | ||
| setupOnce() { | ||
| instrumentEffect(); | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; | ||
|
|
||
| /** | ||
| * Adds Sentry tracing instrumentation for [Effect](https://effect.website/). | ||
| * | ||
| * This integration automatically traces Effect spans and captures errors that occur | ||
| * within Effect computations as Sentry exceptions with proper context. | ||
| * | ||
| * @example | ||
| * ```javascript | ||
| * const Sentry = require('@sentry/node'); | ||
| * | ||
| * Sentry.init({ | ||
| * integrations: [Sentry.effectIntegration()], | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export const effectIntegration = defineIntegration(_effectIntegration); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { effectIntegration, instrumentEffect } from './effectIntegration'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // Type definitions based on Effect's actual API | ||
| export interface EffectSpan { | ||
| name: string; | ||
| startTime: bigint; | ||
| endTime?: bigint; | ||
| attributes?: Record<string, unknown>; | ||
| status?: { | ||
| code: number; | ||
| message?: string; | ||
| }; | ||
| parent?: EffectSpan; | ||
| } | ||
|
|
||
| export interface EffectExit<E = unknown, A = unknown> { | ||
| readonly _tag: 'Success' | 'Failure' | 'Interrupt'; | ||
| readonly cause?: E; | ||
| readonly value?: A; | ||
| } | ||
|
|
||
| export interface EffectTracer { | ||
| onSpanStart?: (span: EffectSpan) => void; | ||
| onSpanEnd?: (span: EffectSpan, exit: EffectExit) => void; | ||
|
Comment on lines
+21
to
+22
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. Where can you find this in effect? |
||
| span<A>(name: string, f: () => A): A; | ||
|
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 span method on custom tracers takes 6 args, not 2. |
||
| withSpan<A>(span: EffectSpan, f: () => A): A; | ||
|
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. Where can you find this in the effect tracer interface? 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. Effect tracer interface method signatures are incorrectHigh Severity The Additional Locations (1) |
||
| } | ||
|
|
||
| export interface EffectModule { | ||
| Tracer?: { | ||
| get?: () => EffectTracer | undefined; | ||
| current?: () => EffectTracer | undefined; | ||
| set?: (tracer: EffectTracer) => void; | ||
| register?: (tracer: EffectTracer) => void; | ||
| use?: (tracer: EffectTracer) => void; | ||
| }; | ||
| } | ||


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.
Missing mechanism parameter in captureException calls
Medium Severity
The
captureExceptioncalls at lines 50, 72, and 105 are missing themechanismparameter withhandledandtypeproperties. Other integrations likeconnect.ts,express.ts, andfastify/index.tsproperly include{ mechanism: { handled: false, type: 'auto.middleware.connect' } }. The mechanism should use a type like'auto.effect'to align with the span origin.Additional Locations (2)
packages/node/src/integrations/tracing/effect/effectIntegration.ts#L71-L72packages/node/src/integrations/tracing/effect/effectIntegration.ts#L104-L105