feat(effect): Add tracing to the effectLayer#19655
feat(effect): Add tracing to the effectLayer#19655JPeer264 merged 2 commits intojp/add-effect-sdkfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| this.sentrySpan.setStatus({ code: 2, message }); | ||
| } else { | ||
| this.sentrySpan.setStatus({ code: 1 }); | ||
| } |
There was a problem hiding this comment.
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.
size-limit report 📦
|
packages/effect/src/client/index.ts
Outdated
| 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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
True, that is one thing that was already mentioned before by @s1gr1d and then I forgot to add it here. Great re-catch.
packages/effect/src/server/index.ts
Outdated
| 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)); |
| this.status = { | ||
| _tag: 'Ended', | ||
| endTime, | ||
| exit, | ||
| startTime: this.status.startTime, | ||
| }; |
There was a problem hiding this comment.
q: Shouldn't we also move this to after the isRecording check?
There was a problem hiding this comment.
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.
| 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, init(options)); |
There was a problem hiding this comment.
Server effectLayer eagerly calls init unlike client
Medium Severity
The server effectLayer calls init(options) eagerly at function-call time, while the client version wraps the call in suspendLayer to defer initialization until the layer is actually provided to an Effect program. This means on the server, calling effectLayer(options) immediately initializes Sentry as a side effect — even if the returned layer is never used. Multiple calls to effectLayer would also call init multiple times on the server, but only once (per layer usage) on the client. The server version likely needs suspendLayer wrapping for consistency.
Additional Locations (1)
There was a problem hiding this comment.
I think this is fine. Also, it already has suspendLayer as a wrapper
| export * from '@sentry/node-core/light'; | ||
|
|
||
| export { effectLayer } from './server/index'; | ||
| export { effectLayer, init } from './server/index'; |
There was a problem hiding this comment.
Missing eslint-disable for duplicate init export in server
Low Severity
index.server.ts re-exports init from ./server/index while also doing export * from '@sentry/node-core/light', which also exports init. The equivalent index.client.ts file includes a /* eslint-disable import/export */ comment to suppress the duplicate-export lint error for this exact pattern, but index.server.ts is missing it.
Triggered by project rule: PR Review Guidelines for Cursor Bot
There was a problem hiding this comment.
It is missing because there is no lint error. Apparently the ESLint rule has its problems with sub exports (or it works properly there, but that I doubt highly)
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/3
branch
from
0f2390f to
72829d3
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing
SEMANTIC_ATTRIBUTE_SENTRY_OPattribute instartInactiveSpan- Added SEMANTIC_ATTRIBUTE_SENTRY_OP to span attributes in startInactiveSpan call to follow codebase convention.
Or push these changes by commenting:
@cursor push b3d2b03dc2
Preview (b3d2b03dc2)
diff --git a/packages/effect/src/tracer.ts b/packages/effect/src/tracer.ts
--- a/packages/effect/src/tracer.ts
+++ b/packages/effect/src/tracer.ts
@@ -2,6 +2,7 @@
import {
getActiveSpan,
getIsolationScope,
+ SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startInactiveSpan,
withActiveSpan,
@@ -168,12 +169,15 @@
const spanName = deriveSpanName(name, kind);
+ const op = deriveOp(name, kind);
+
const newSpan = startInactiveSpan({
name: spanName,
- op: deriveOp(name, kind),
+ op,
startTime: nanosToHrTime(startTime),
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: deriveOrigin(name),
+ [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op,
},
...(parentSentrySpan ? { parentSpan: parentSentrySpan } : {}),
});| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: deriveOrigin(name), | ||
| }, | ||
| ...(parentSentrySpan ? { parentSpan: parentSentrySpan } : {}), | ||
| }); |
There was a problem hiding this comment.
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.
Triggered by project rule: PR Review Guidelines for Cursor Bot
There was a problem hiding this comment.
Not for startInactiveSpan, at least based on my short research
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Or push these changes by commenting: Preview (1725135f24)diff --git a/packages/effect/src/client/index.ts b/packages/effect/src/client/index.ts
--- a/packages/effect/src/client/index.ts
+++ b/packages/effect/src/client/index.ts
@@ -1,9 +1,11 @@
import type { BrowserOptions } from '@sentry/browser';
-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';
+import { init } from './sdk';
+export { init } from './sdk';
+
/**
* Options for the Sentry Effect client layer.
*/
@@ -32,5 +34,5 @@
* ```
*/
export function effectLayer(options: EffectClientLayerOptions): EffectLayer.Layer<never, never, never> {
- return suspendLayer(() => buildEffectLayer(options, Sentry.init(options)));
+ return suspendLayer(() => buildEffectLayer(options, init(options)));
}
diff --git a/packages/effect/src/client/sdk.ts b/packages/effect/src/client/sdk.ts
new file mode 100644
--- /dev/null
+++ b/packages/effect/src/client/sdk.ts
@@ -1,0 +1,20 @@
+import type { BrowserOptions } from '@sentry/browser';
+import { init as initBrowser } from '@sentry/browser';
+import type { Client } from '@sentry/core';
+import { applySdkMetadata } from '@sentry/core';
+
+/**
+ * Initializes the Sentry Effect SDK for browser clients.
+ *
+ * @param options - Configuration options for the SDK
+ * @returns The initialized Sentry client, or undefined if initialization failed
+ */
+export function init(options: BrowserOptions): Client | undefined {
+ const opts = {
+ ...options,
+ };
+
+ applySdkMetadata(opts, 'effect', ['effect', 'browser']);
+
+ return initBrowser(opts);
+}
diff --git a/packages/effect/src/index.client.ts b/packages/effect/src/index.client.ts
--- a/packages/effect/src/index.client.ts
+++ b/packages/effect/src/index.client.ts
@@ -1,4 +1,7 @@
+// import/export got a false positive, and affects most of our index barrel files
+// can be removed once following issue is fixed: https://github.com/import-js/eslint-plugin-import/issues/703
+/* eslint-disable import/export */
export * from '@sentry/browser';
-export { effectLayer } from './client/index';
+export { effectLayer, init } from './client/index';
export type { EffectClientLayerOptions } from './client/index';
diff --git a/packages/effect/src/index.server.ts b/packages/effect/src/index.server.ts
--- a/packages/effect/src/index.server.ts
+++ b/packages/effect/src/index.server.ts
@@ -1,4 +1,7 @@
+// import/export got a false positive, and affects most of our index barrel files
+// can be removed once following issue is fixed: https://github.com/import-js/eslint-plugin-import/issues/703
+/* eslint-disable import/export */
export * from '@sentry/node-core/light';
-export { effectLayer } from './server/index';
+export { effectLayer, init } from './server/index';
export type { EffectServerLayerOptions } from './server/index';
diff --git a/packages/effect/src/server/index.ts b/packages/effect/src/server/index.ts
--- a/packages/effect/src/server/index.ts
+++ b/packages/effect/src/server/index.ts
@@ -1,8 +1,11 @@
-import type { NodeOptions } from '@sentry/node-core';
-import * as Sentry from '@sentry/node-core/light';
+import type { NodeOptions } from '@sentry/node-core/light';
import type * as EffectLayer from 'effect/Layer';
+import { suspend as suspendLayer } from 'effect/Layer';
import { buildEffectLayer } from '../utils/buildEffectLayer';
+import { init } from './sdk';
+export { init } from './sdk';
+
/**
* Options for the Sentry Effect server layer.
*/
@@ -33,5 +36,5 @@
* ```
*/
export function effectLayer(options: EffectServerLayerOptions): EffectLayer.Layer<never, never, never> {
- return buildEffectLayer(options, Sentry.init(options));
+ return suspendLayer(() => buildEffectLayer(options, init(options)));
}
diff --git a/packages/effect/src/server/sdk.ts b/packages/effect/src/server/sdk.ts
new file mode 100644
--- /dev/null
+++ b/packages/effect/src/server/sdk.ts
@@ -1,0 +1,20 @@
+import type { Client } from '@sentry/core';
+import { applySdkMetadata } from '@sentry/core';
+import type { NodeOptions } from '@sentry/node-core/light';
+import { init as initNode } from '@sentry/node-core/light';
+
+/**
+ * Initializes the Sentry Effect SDK for Node.js servers.
+ *
+ * @param options - Configuration options for the SDK
+ * @returns The initialized Sentry client, or undefined if initialization failed
+ */
+export function init(options: NodeOptions): Client | undefined {
+ const opts = {
+ ...options,
+ };
+
+ applySdkMetadata(opts, 'effect', ['effect', 'node-light']);
+
+ return initNode(opts);
+}
diff --git a/packages/effect/test/layer.test.ts b/packages/effect/test/layer.test.ts
--- a/packages/effect/test/layer.test.ts
+++ b/packages/effect/test/layer.test.ts
@@ -1,5 +1,5 @@
import { describe, expect, it } from '@effect/vitest';
-import { getCurrentScope, getIsolationScope } from '@sentry/core';
+import { getClient, getCurrentScope, getIsolationScope, SDK_VERSION } from '@sentry/core';
import { Effect, Layer } from 'effect';
import { afterEach, beforeEach, vi } from 'vitest';
import * as sentryClient from '../src/index.client';
@@ -15,9 +15,9 @@
}
describe.each([
- ['client', sentryClient.effectLayer],
- ['server', sentryServer.effectLayer],
-])('effectLayer ($name)', (name, effectLayer) => {
+ [{ subSdkName: 'browser', effectLayer: sentryClient.effectLayer }],
+ [{ subSdkName: 'node-light', effectLayer: sentryServer.effectLayer }],
+])('effectLayer ($subSdkName)', ({ subSdkName, effectLayer }) => {
beforeEach(() => {
getCurrentScope().clear();
getIsolationScope().clear();
@@ -37,6 +37,28 @@
expect(Layer.isLayer(layer)).toBe(true);
});
+ it.effect('applies SDK metadata', () =>
+ Effect.gen(function* () {
+ yield* Effect.void;
+
+ const client = getClient();
+ const metadata = client?.getOptions()._metadata?.sdk;
+
+ expect(metadata?.name).toBe('sentry.javascript.effect');
+ expect(metadata?.packages).toEqual([
+ { name: 'npm:@sentry/effect', version: SDK_VERSION },
+ { name: `npm:@sentry/${subSdkName}`, version: SDK_VERSION },
+ ]);
+ }).pipe(
+ Effect.provide(
+ effectLayer({
+ dsn: TEST_DSN,
+ transport: getMockTransport(),
+ }),
+ ),
+ ),
+ );
+
it.effect('layer can be provided to an Effect program', () =>
Effect.gen(function* () {
const result = yield* Effect.succeed('test-result'); |
2 participants
This adds tracing to the
Sentry.effectLayer. By settingtracesSampleRate: 1.0in the options tracing is enabled and spans can be send to Sentry