Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions packages/effect/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const MainLive = HttpLive.pipe(
Layer.provide(
Sentry.effectLayer({
dsn: '__DSN__',
tracesSampleRate: 1.0,
enableLogs: true,
enableEffectLogs: true,
enableEffectMetrics: true,
Expand Down
4 changes: 4 additions & 0 deletions packages/effect/src/index.client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@ export * from '@sentry/browser';

export { effectLayer, init } from './client/index';
export type { EffectClientLayerOptions } from './client/index';

export { SentryEffectTracer } from './tracer';
export { SentryEffectLogger } from './logger';
export { SentryEffectMetricsLayer } from './metrics';
4 changes: 4 additions & 0 deletions packages/effect/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@ export * from '@sentry/node-core/light';

export { effectLayer, init } from './server/index';
export type { EffectServerLayerOptions } from './server/index';

export { SentryEffectTracer } from './tracer';
export { SentryEffectLogger } from './logger';
export { SentryEffectMetricsLayer } from './metrics';
2 changes: 1 addition & 1 deletion packages/effect/src/tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
} from '@sentry/core';
import type * as Context from 'effect/Context';
import * as Exit from 'effect/Exit';
import type * as Layer from 'effect/Layer';

Check warning on line 11 in packages/effect/src/tracer.ts

View workflow job for this annotation

GitHub Actions / Lint

eslint(no-unused-vars)

Type 'Layer' is imported but never used.
import { setTracer } from 'effect/Layer';

Check warning on line 12 in packages/effect/src/tracer.ts

View workflow job for this annotation

GitHub Actions / Lint

eslint(no-unused-vars)

Identifier 'setTracer' is imported but never used.
import * as Option from 'effect/Option';
import * as EffectTracer from 'effect/Tracer';

Expand Down Expand Up @@ -198,4 +198,4 @@
/**
* Effect Layer that sets up the Sentry tracer for Effect spans.
*/
export const SentryEffectTracerLayer: Layer.Layer<never, never, never> = setTracer(makeSentryTracer());
export const SentryEffectTracer = makeSentryTracer();
Copy link

Choose a reason for hiding this comment

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

Bug: The export SentryEffectTracerLayer was renamed, but the test file tracer.test.ts was not updated and still imports the old, non-existent name, which will break tests.
Severity: HIGH

Suggested Fix

Update packages/effect/test/tracer.test.ts to import the new SentryEffectTracer and wrap it with setTracer before providing it to the Effect in the tests, for example: Effect.provide(setTracer(SentryEffectTracer)). Also, remove the unused setTracer import from packages/effect/src/tracer.ts.

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/effect/src/tracer.ts#L201

Potential issue: The refactoring in `packages/effect/src/tracer.ts` replaced the `Layer`
export `SentryEffectTracerLayer` with a raw `Tracer` object named `SentryEffectTracer`.
However, the corresponding test file, `packages/effect/test/tracer.test.ts`, was not
updated. It continues to import `SentryEffectTracerLayer`, which no longer exists. This
import will resolve to `undefined` at runtime, causing all 16 test cases that call
`Effect.provide(SentryEffectTracerLayer)` to fail with a runtime error. Additionally, an
unused import for `setTracer` remains in `tracer.ts`.

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

13 changes: 9 additions & 4 deletions packages/effect/src/utils/buildEffectLayer.ts
Copy link
Member

Choose a reason for hiding this comment

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

ignoring this for the review since we remove it in #19823

Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import type { Client } from '@sentry/core';
import { hasSpansEnabled, type Client } from '@sentry/core';
import type * as EffectLayer from 'effect/Layer';
import { empty as emptyLayer, provideMerge } from 'effect/Layer';
import { empty as emptyLayer, provideMerge, setTracer } from 'effect/Layer';
import { defaultLogger, replace as replaceLogger } from 'effect/Logger';
import { SentryEffectLogger } from '../logger';
import { SentryEffectMetricsLayer } from '../metrics';
import { SentryEffectTracerLayer } from '../tracer';
import { SentryEffectTracer } from '../tracer';

export interface EffectLayerBaseOptions {
enableEffectLogs?: boolean;
Expand All @@ -27,10 +27,15 @@ export function buildEffectLayer<T extends EffectLayerBaseOptions>(
}

const clientOptions = client.getOptions();
const hasSpans = hasSpansEnabled(clientOptions);
const enableMetrics = clientOptions.enableMetrics ?? clientOptions._experiments?.enableMetrics ?? true;
const enableLogs = clientOptions.enableLogs ?? clientOptions._experiments?.enableLogs ?? false;
const { enableEffectLogs = false, enableEffectMetrics = false } = options;
let layer: EffectLayer.Layer<never, never, never> = SentryEffectTracerLayer;
let layer = emptyLayer;

if (hasSpans) {
layer = layer.pipe(provideMerge(setTracer(SentryEffectTracer)));
}

if (enableEffectLogs && enableLogs) {
const effectLogger = replaceLogger(defaultLogger, SentryEffectLogger);
Expand Down
Loading