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
14 changes: 10 additions & 4 deletions packages/effect/src/client/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
import type { BrowserOptions } from '@sentry/browser';
import * as EffectLayer from 'effect/Layer';
import * as Sentry from '@sentry/browser';
import type * as EffectLayer from 'effect/Layer';
import { suspend as suspendLayer } from 'effect/Layer';
import { buildEffectLayer } from '../utils/buildEffectLayer';

/**
* Options for the Sentry Effect client layer.
*/
export type EffectClientLayerOptions = BrowserOptions;

/**
* Creates an empty Effect Layer
* Creates an Effect Layer that initializes Sentry for browser clients.
*
* This layer provides Effect applications with full Sentry instrumentation including:
* - Effect spans traced as Sentry spans
*
* @example
* ```typescript
Expand All @@ -25,6 +31,6 @@ export type EffectClientLayerOptions = BrowserOptions;
* Effect.runPromise(Effect.provide(myEffect, ApiClientWithSentry));
* ```
*/
export function effectLayer(_: EffectClientLayerOptions): EffectLayer.Layer<never, never, never> {
return EffectLayer.empty;
export function effectLayer(options: EffectClientLayerOptions): EffectLayer.Layer<never, never, never> {
return suspendLayer(() => buildEffectLayer(options, Sentry.init(options)));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: We should add an sdk.ts that exports an init and at least apply sdk metadata so we know that data came from the effect sdk, not the actual browser sdk.

Have a look at: https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/client/sdk.ts

Not sure if you're planning to do this in follow-up PRs, so feel free to disregard if so.

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.

True, that is one thing that was already mentioned before by @s1gr1d and then I forgot to add it here. Great re-catch.

}
13 changes: 9 additions & 4 deletions packages/effect/src/server/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import type { NodeOptions } from '@sentry/node-core';
import * as EffectLayer from 'effect/Layer';
import * as Sentry from '@sentry/node-core/light';
import type * as EffectLayer from 'effect/Layer';
import { buildEffectLayer } from '../utils/buildEffectLayer';

/**
* Options for the Sentry Effect server layer.
*/
export type EffectServerLayerOptions = NodeOptions;

/**
* Creates an empty Effect Layer
* Creates an Effect Layer that initializes Sentry for Node.js servers.
*
* This layer provides Effect applications with full Sentry instrumentation including:
* - Effect spans traced as Sentry spans
*
* @example
* ```typescript
Expand All @@ -27,6 +32,6 @@ export type EffectServerLayerOptions = NodeOptions;
* MainLive.pipe(Layer.launch, NodeRuntime.runMain);
* ```
*/
export function effectLayer(_: EffectServerLayerOptions): EffectLayer.Layer<never, never, never> {
return EffectLayer.empty;
export function effectLayer(options: EffectServerLayerOptions): EffectLayer.Layer<never, never, never> {
return buildEffectLayer(options, Sentry.init(options));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: Same here.

}
201 changes: 201 additions & 0 deletions packages/effect/src/tracer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
import type { Span } from '@sentry/core';
import {
getActiveSpan,
getIsolationScope,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startInactiveSpan,
withActiveSpan,
} from '@sentry/core';
import type * as Context from 'effect/Context';
import * as Exit from 'effect/Exit';
import type * as Layer from 'effect/Layer';
import { setTracer } from 'effect/Layer';
import * as Option from 'effect/Option';
import * as EffectTracer from 'effect/Tracer';

const KIND_MAP: Record<EffectTracer.SpanKind, 'internal' | 'server' | 'client' | 'producer' | 'consumer'> = {
internal: 'internal',
client: 'client',
server: 'server',
producer: 'producer',
consumer: 'consumer',
};

function deriveOp(name: string, kind: EffectTracer.SpanKind): string {
if (name.startsWith('http.server')) {
return 'http.server';
}

if (name.startsWith('http.client')) {
return 'http.client';
}

return KIND_MAP[kind];
}

function deriveOrigin(name: string): string {
if (name.startsWith('http.server') || name.startsWith('http.client')) {
return 'auto.http.effect';
}

return 'auto.function.effect';
}

function deriveSpanName(name: string, kind: EffectTracer.SpanKind): string {
if (name.startsWith('http.server') && kind === 'server') {
const isolationScope = getIsolationScope();
const transactionName = isolationScope.getScopeData().transactionName;
if (transactionName) {
return transactionName;
}
}
return name;
}

type HrTime = [number, number];

const SENTRY_SPAN_SYMBOL = Symbol.for('@sentry/effect.SentrySpan');

function nanosToHrTime(nanos: bigint): HrTime {
const seconds = Number(nanos / BigInt(1_000_000_000));
const remainingNanos = Number(nanos % BigInt(1_000_000_000));
return [seconds, remainingNanos];
}

interface SentrySpanLike extends EffectTracer.Span {
readonly [SENTRY_SPAN_SYMBOL]: true;
readonly sentrySpan: Span;
}

function isSentrySpan(span: EffectTracer.AnySpan): span is SentrySpanLike {
return SENTRY_SPAN_SYMBOL in span;
}

class SentrySpanWrapper implements SentrySpanLike {
public readonly [SENTRY_SPAN_SYMBOL]: true;
public readonly _tag: 'Span';
public readonly spanId: string;
public readonly traceId: string;
public readonly attributes: Map<string, unknown>;
public readonly sampled: boolean;
public readonly parent: Option.Option<EffectTracer.AnySpan>;
public readonly links: Array<EffectTracer.SpanLink>;
public status: EffectTracer.SpanStatus;
public readonly sentrySpan: Span;

public constructor(
public readonly name: string,
parent: Option.Option<EffectTracer.AnySpan>,
public readonly context: Context.Context<never>,
links: ReadonlyArray<EffectTracer.SpanLink>,
startTime: bigint,
public readonly kind: EffectTracer.SpanKind,
existingSpan: Span,
) {
this[SENTRY_SPAN_SYMBOL] = true as const;
this._tag = 'Span' as const;
this.attributes = new Map<string, unknown>();
this.parent = parent;
this.links = [...links];
this.sentrySpan = existingSpan;

const spanContext = this.sentrySpan.spanContext();
this.spanId = spanContext.spanId;
this.traceId = spanContext.traceId;
this.sampled = this.sentrySpan.isRecording();
this.status = {
_tag: 'Started',
startTime,
};
}

public attribute(key: string, value: unknown): void {
if (!this.sentrySpan.isRecording()) {
return;
}

this.sentrySpan.setAttribute(key, value as Parameters<Span['setAttribute']>[1]);
this.attributes.set(key, value);
}

public addLinks(links: ReadonlyArray<EffectTracer.SpanLink>): void {
this.links.push(...links);
}

public end(endTime: bigint, exit: Exit.Exit<unknown, unknown>): void {
this.status = {
_tag: 'Ended',
endTime,
exit,
startTime: this.status.startTime,
};
Comment on lines +126 to +131
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

q: Shouldn't we also move this to after the isRecording check?

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.

That is a very good question indeed. I'm in between of changing and not changing it. For one reason:

When the span would end from the outside (for whatever reason), the this.status would never be set. It would hold a slightly false endTime, but it would set the _tag to Ended. If we would change it and the span would end from the outside the this.status would always be _tag: 'Started'. This doesn't have any downsides right now, as the information is not really used right now, but it would be false.

The good side of changing it is that endTime would always be correct (IF it was set). I still lean to not changing it and leaving it as is, for the sake of setting _tag to Ended in this edge case scenario.


if (!this.sentrySpan.isRecording()) {
return;
}

if (Exit.isFailure(exit)) {
const cause = exit.cause;
const message =
cause._tag === 'Fail' ? String(cause.error) : cause._tag === 'Die' ? String(cause.defect) : 'internal_error';
this.sentrySpan.setStatus({ code: 2, message });
} else {
this.sentrySpan.setStatus({ code: 1 });
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Interrupted spans incorrectly marked as errors

Medium Severity

Effect Interrupt causes (fiber cancellations) fall through the ternary chain and get span status code: 2 (ERROR) with message 'internal_error'. In Effect, interruptions are a normal control flow mechanism (cooperative cancellation), not errors. Marking interrupted spans as errors will inflate error rates in Sentry dashboards, producing false positive alerts. The cause._tag check only handles 'Fail' and 'Die', but 'Interrupt', 'Sequential', and 'Parallel' all silently become error spans with a misleading 'internal_error' message. Interruptions could use code: 1 (OK) or a 'cancelled' message, and composite causes (Sequential/Parallel) deserve more descriptive messages.

Fix in Cursor Fix in Web


this.sentrySpan.end(nanosToHrTime(endTime));
}

public event(name: string, startTime: bigint, attributes?: Record<string, unknown>): void {
if (!this.sentrySpan.isRecording()) {
return;
}

this.sentrySpan.addEvent(name, attributes as Parameters<Span['addEvent']>[1], nanosToHrTime(startTime));
}
}

function createSentrySpan(
name: string,
parent: Option.Option<EffectTracer.AnySpan>,
context: Context.Context<never>,
links: ReadonlyArray<EffectTracer.SpanLink>,
startTime: bigint,
kind: EffectTracer.SpanKind,
): SentrySpanLike {
const parentSentrySpan =
Option.isSome(parent) && isSentrySpan(parent.value) ? parent.value.sentrySpan : (getActiveSpan() ?? null);

const spanName = deriveSpanName(name, kind);

const newSpan = startInactiveSpan({
name: spanName,
op: deriveOp(name, kind),
startTime: nanosToHrTime(startTime),
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: deriveOrigin(name),
},
...(parentSentrySpan ? { parentSpan: parentSentrySpan } : {}),
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing SEMANTIC_ATTRIBUTE_SENTRY_OP attribute in startInactiveSpan

Low Severity

The startInactiveSpan call sets the op via the top-level op option but does not set SEMANTIC_ATTRIBUTE_SENTRY_OP as a span attribute. The review rules require that SEMANTIC_ATTRIBUTE_SENTRY_OP is set in span attributes when calling any startSpan API. Other packages in this codebase (e.g., nestjs) follow the convention of setting it explicitly in the attributes object using the SEMANTIC_ATTRIBUTE_SENTRY_OP constant.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

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.

Not for startInactiveSpan, at least based on my short research


return new SentrySpanWrapper(name, parent, context, links, startTime, kind, newSpan);
}

const makeSentryTracer = (): EffectTracer.Tracer =>
EffectTracer.make({
span(name, parent, context, links, startTime, kind) {
return createSentrySpan(name, parent, context, links, startTime, kind);
},
context(execution, fiber) {
const currentSpan = fiber.currentSpan;
if (currentSpan === undefined || !isSentrySpan(currentSpan)) {
return execution();
}
return withActiveSpan(currentSpan.sentrySpan, execution);
},
});

/**
* Effect Layer that sets up the Sentry tracer for Effect spans.
*/
export const SentryEffectTracerLayer: Layer.Layer<never, never, never> = setTracer(makeSentryTracer());
24 changes: 24 additions & 0 deletions packages/effect/src/utils/buildEffectLayer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import type * as EffectLayer from 'effect/Layer';
import { empty as emptyLayer } from 'effect/Layer';
import { SentryEffectTracerLayer } from '../tracer';

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface EffectLayerBaseOptions {}

/**
* Builds an Effect layer that integrates Sentry tracing.
*
* Returns an empty layer if no Sentry client is available. Otherwise, starts with
* the Sentry tracer layer and optionally merges logging and metrics layers based
* on the provided options.
*/
export function buildEffectLayer<T extends EffectLayerBaseOptions>(
options: T,
client: unknown,
): EffectLayer.Layer<never, never, never> {
if (!client) {
return emptyLayer;
}

return SentryEffectTracerLayer;
}
57 changes: 57 additions & 0 deletions packages/effect/test/buildEffectLayer.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { describe, expect, it, vi } from '@effect/vitest';
import * as sentryCore from '@sentry/core';
import { Effect, Layer } from 'effect';
import { empty as emptyLayer } from 'effect/Layer';
import { buildEffectLayer } from '../src/utils/buildEffectLayer';

describe('buildEffectLayer', () => {
describe('when client is falsy', () => {
it('returns empty layer when client is null', () => {
const layer = buildEffectLayer({}, null);

expect(layer).toBeDefined();
expect(Layer.isLayer(layer)).toBe(true);
expect(layer).toBe(emptyLayer);
});

it('returns empty layer when client is undefined', () => {
const layer = buildEffectLayer({}, undefined);

expect(layer).toBeDefined();
expect(Layer.isLayer(layer)).toBe(true);
expect(layer).toBe(emptyLayer);
});
});

describe('when client is truthy', () => {
const mockClient = { mock: true };

it('returns a valid layer with default options', () => {
const layer = buildEffectLayer({}, mockClient);

expect(layer).toBeDefined();
expect(Layer.isLayer(layer)).toBe(true);
});

it.effect('layer can be provided to an Effect program', () =>
Effect.gen(function* () {
const result = yield* Effect.succeed('test-result');
expect(result).toBe('test-result');
}).pipe(Effect.provide(buildEffectLayer({}, mockClient))),
);

it.effect('layer enables tracing for Effect spans via Sentry tracer', () =>
Effect.gen(function* () {
const startInactiveSpanSpy = vi.spyOn(sentryCore, 'startInactiveSpan');
const result = yield* Effect.withSpan('test-sentry-span')(Effect.succeed('traced'));
expect(result).toBe('traced');
expect(startInactiveSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
name: 'test-sentry-span',
}),
);
startInactiveSpanSpy.mockRestore();
}).pipe(Effect.provide(buildEffectLayer({}, mockClient))),
);
});
});
Loading
Loading