feat(effect): Add @sentry/effect SDK (alpha)#19644
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
JPeer264
changed the title
JPeer264
changed the title
JPeer264
force-pushed
the
jp/add-effect-sdk
branch
2 times, most recently
from
2db4b85 to
3070535
Compare
This is one of many PRs to create the effect SDK. Once this has been merged I will open the draft PR for the effect sdk and create the plan in there. (the almost final SDK can be viewed here: https://github.com/getsentry/sentry-javascript/tree/jp/effect-sdk. It might be that some specifics change, especially when having browser + server split, and with tracing) --- This PR focuses on the base skaffolding of `@sentry/effect`. This on its own is not really doing anything except setting up the skaffold. The README already reflects the actual usage, while the export doesn't exist yet, this will come in another PR (also `init` is exposed here, just for the sake of completeness) --------- Co-authored-by: Claude <[email protected]>
…9649) That adds now the functionality to use the `Sentry.effectLayer` properly. **But** it doesn't do anything, which means right now, to keep the PRs small, it returns an empty layer. Following can be used without any Sentry functionality: ```js const MainLive = HttpLive.pipe(Layer.provide(Sentry.effectLayer({ dsn: "", tracesSampleRate: 1.0, debug: true, }))) MainLive.pipe(Layer.launch, NodeRuntime.runMain) ```
This adds tracing to the `Sentry.effectLayer`. By setting `tracesSampleRate: 1.0` in the options tracing is enabled and spans can be send to Sentry
This adds the functionality to send logs to Sentry by setting `enableLogs: true` in the `Sentry.effectLayer`
This adds metrics to the `Sentry.effectLayer`. It is enabled when `enableMetrics: true` is added as option
This PR is now adding a different naming schema for enabling logs and metrics based on: https://develop.sentry.dev/sdk/telemetry/metrics/#auto-emitted-metrics For the logs I also added them, which might not make the most sense, as `enableLogs` is now `false` by default, which means that there is a double opt-in needed to make logs work via `Effect.log`. The naming is TBD, but this is the best I came up with: `enableEffectLogs` & `enableEffectMetrics`
This adds Node and Browser tests for the `@sentry/effect` SDK. I am not sure what to do with the browser part, as there is I guess no tree-shaking available right now. The basic usage for node and browser are the exact same, only the `effectLayer` has to be added into the runtime layer.
JPeer264
force-pushed
the
jp/add-effect-sdk
branch
from
8c2378e to
6d004ec
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| default: | ||
| logLevel satisfies never; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Effect log annotations silently dropped by logger
High Severity
The SentryEffectLogger destructures only { logLevel, message } from the Effect Logger callback, completely ignoring annotations. When users call Effect.annotateLogs('userId', '12345'), those annotations are available in the callback params but are never forwarded as attributes to the Sentry logger functions (sentryLogger.info(msg, attributes)). All log context data is silently lost. The E2E tests for "logs with context" don't assert on the attributes either, so this bug goes undetected.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| attributes: { ...attributes, word }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Frequency metrics double-count on periodic flush cycles
Medium Severity
Frequency metrics send total accumulated occurrence counts via sentryMetrics.count on every periodic flush, but sentryMetrics.count is additive. Unlike counter metrics which have delta tracking via sendDeltaMetricToSentry, frequency metrics always re-send the full cumulative count, causing values to inflate with each flush cycle. For example, if "apple" has 3 occurrences, after two flushes Sentry records 3+3=6 instead of 3.
Additional Locations (1)
| if (enableEffectLogs && enableLogs) { | ||
| const effectLogger = replaceLogger(defaultLogger, SentryEffectLogger); | ||
| layer = layer.pipe(provideMerge(effectLogger)); | ||
| } | ||
|
|
||
| if (enableEffectMetrics && enableMetrics) { | ||
| layer = layer.pipe(provideMerge(SentryEffectMetricsLayer)); | ||
| } |
There was a problem hiding this comment.
h: If we ship this, we cannot tree-shake logger and metrics-related code in the browser. This is not how we usually add features to browser SDKs. Was this a conscious decision? I might be missing a reason for this, so don't feel compelled to rework it instantly.
Have we explored tree-shakeable alternatives? Random thought coming from a person without context on Effect: Could we instruct users to manually register SentryEffectLogger and SentryEffectsMetricsLayer? This would also count as an opt in. We can leave the enableEffectMetrics flags for the server implementation.
I marked this as H but given the bundle size increase just concerns Effect, we can take some liberties here. I just wonder why we don't do the same for tracing then (?).
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Also, we should add e2e tests for metrics but feel free to follow up on this later
| const isolationScope = getIsolationScope(); | ||
| const transactionName = isolationScope.getScopeData().transactionName; | ||
| if (transactionName) { | ||
| return transactionName; | ||
| } |
There was a problem hiding this comment.
m/h: Who sets the transaction name on the scope? We never derive a span name from the transactionName on the scope. It should be the other way around: We find a good span name (see below) and we set this as the transactionName on the scope.
For a span with op http.server, the span name should follow the {METHOD} {ROUTE} pattern. Is there a way we can get a proper route?
My concern here also is that if it's just the default Node http integration setting the scope's transactionName based on the incoming request URL, this is an unparameterized path. Again leading me to the question if we can find a better route name here
| if (name.startsWith('http.client')) { | ||
| return 'http.client'; | ||
| } |
There was a problem hiding this comment.
m: just to confirm, we only set this op for outgoing (fetch/xhr/http) requests, correct?
And Q: Effect emits http.server and http.client as span names, correct?
| HttpRouter.get( | ||
| '/test-exception/:id', | ||
| Effect.sync(() => { | ||
| throw new Error('This is an exception with id 123'); | ||
| }), | ||
| ), |
There was a problem hiding this comment.
Are we able to extract /test-exception/:id for this route and set it as the scope's transactionName (see other comment)?
3 participants
closes #19641
closes JS-1864
This PR collects all Effect SDK integration PRs.
The Effect SDK provides first-class integration with Effect, enabling automatic tracing, logging, and metrics capture for Effect-based applications.
Features
Checklist of features
Merge checklist
Checklist of featuresare checked