feat(effect): Remove effectLayer auto composition#19823
feat(effect): Remove effectLayer auto composition#19823JPeer264 merged 2 commits intojp/add-effect-sdkfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const newSpan = startInactiveSpan({ | ||
| name: spanName, | ||
| op: deriveOp(name, kind), | ||
| name, |
There was a problem hiding this comment.
Missing SEMANTIC_ATTRIBUTE_SENTRY_OP in startInactiveSpan call
Medium Severity
The startInactiveSpan call no longer sets SEMANTIC_ATTRIBUTE_SENTRY_OP ('sentry.op'). The deriveOp function and the op parameter were removed as part of this PR. Per project rules, when calling any startSpan API, SEMANTIC_ATTRIBUTE_SENTRY_OP must always be set with a proper span op. The origin attribute is still correctly set, but the op attribute is missing entirely from the attributes object.
Triggered by project rule: PR Review Guidelines for Cursor Bot
size-limit report 📦
|
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/9
branch
from
56f59d3 to
cb6091d
Compare
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.
| test('Sends an HTTP transaction', async ({ baseURL }) => { | ||
| const transactionEventPromise = waitForTransaction('effect-node', transactionEvent => { | ||
| return ( | ||
| transactionEvent?.contexts?.trace?.op === 'http.server' && |
There was a problem hiding this comment.
Ambiguous E2E test transaction filters match wrong transactions
Medium Severity
The "Sends an HTTP transaction" and "Sends transaction for error route" tests both use the identical filter transactionEvent?.transaction === 'http.server GET' with no route-specific distinguishing criteria. Previously, filters included route paths like /test-success and /test-error. Now any http.server GET transaction satisfies both filters, making these tests flaky and unable to verify they received the transaction for the correct route.
Additional Locations (1)
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Nice, thanks a lot for reworking the SDK setup. As you said, I think this sets a solid foundation and -- if necessary -- we can build on top of it.
| expect(spans).toContainEqual( | ||
| expect.objectContaining({ | ||
| description: 'custom-effect-span', | ||
| data: expect.objectContaining({ |
There was a problem hiding this comment.
l: just checking, no action required: Does effect add any other attributes we could/should assert on?
(also, removing the op here sounds correct to me. Span kind doesn't map well to sentry ops and there's no internal op defined)
There was a problem hiding this comment.
I added couple of more assertions to this specific test and removed the .toContainEqual in the file completely.
| enableEffectLogs: true, | ||
| }); | ||
| const SentryLive = Layer.mergeAll( | ||
| Sentry.effectLayer({ |
There was a problem hiding this comment.
I think this is a fine setup and naming is also fine. To me (as an Effect noob), this signals that Sentry.effectLayer is the main one and the others are additional components. As long as we document this correctly, we should be good.
I have a question though: Does registering just the Sentry.effectLayer initialize error monitoring? This would be ideal, since errors should be part of the minimal Sentry setup. I believe you said for v4, we'll need an additional component for errors, which is also fine.
There was a problem hiding this comment.
Does registering just the Sentry.effectLayer initialize error monitoring?
Theoretically it would do everything a Sentry.init would do, so if the process crashes or someone would throw an unhandled error then yes. Once we get support for the ErrorReporter in v4, we could think of auto adding this or adding it manually with the layer (currently leaning towards auto adding, but this is another discussion)
| const newSpan = startInactiveSpan({ | ||
| name: spanName, | ||
| op: deriveOp(name, kind), | ||
| name, |
There was a problem hiding this comment.
I think not adding an op is fine for the initial release. However, as discussed we should refine our deriveSpanName and deriveOp logic to make Effects' spans more Sentry-esque in the long run. For example, we need sentry.op and certain span names to make our Insights pages work correctly. This especially concerns http.server, http.client and db spans.
Again, for this release, I'd rather not have ops than incorrect ones though.
| @@ -117,10 +113,56 @@ describe.each([ | |||
| expect(result).toBe(84); | |||
There was a problem hiding this comment.
l/q from an effect noob: does this test that our tracer works? Would it throw/not succeed if our tracer wasn't registered?
There was a problem hiding this comment.
Right, it wouldn't really test if the tracer is not applied. This is more of a smoke test that the tracer wouldn't throw. The actual tests would be inside the E2E tests, I'll add a simple mechanism here to check if tracing is getting called
| enableLogs: true, | ||
| }), | ||
| Layer.setTracer(Sentry.SentryEffectTracer), | ||
| Logger.replace(Logger.defaultLogger, Sentry.SentryEffectLogger), |
There was a problem hiding this comment.
l: should we add the metrics layer here?
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/9
branch
from
587e9e2 to
e8da292
Compare
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/9
branch
from
e8da292 to
6455f93
Compare
There are 2 changes in this PR: 1. No auto-injection anymore 2. Removal of the OP and the span names. We fully rely now on Effect as much as possible (related: #19644 (comment)) About the removal of auto injection of traces, logs or metrics from the `Sentry.effectLayer`: this means that `Sentry.effectLayer` is **only** initializing the browser/node client. So the usages in comparison for logs and traces: before: ```js Layer.provide(Sentry.effectLayer({ dsn: '', tracesSampleRate: 1.0, enableLogs: true, enableEffectLogs: true, })); ``` after: ```js import * as Layer from "effect/Layer"; import * as Logger from "effect/Logger" Layer.mergeAll( Sentry.effectLayer({ dsn: '', tracesSampleRate: 1.0, enableLogs: true, }, Layer.setTracer(Sentry.SentryEffectTracer), Logger.replace(Logger.defaultLogger, Sentry.SentryEffectLogger) )); ``` The benefit of this is to have a unified usage between browser and node, which also follows the usage of Effect, so users have to actively set the tracer with `setTracer` or replace the logger with `Logger.replace`. There is no extra opt-in via `enableEffectLogs` or `enableEffectMetrics`, this would also remove the confusion of not having the `enableEffectTraces` option (which didn't exist before, nor would have existed in any future versions) This was discussed offline with @Lms24 to have this. In the first alpha we can always shift if there is the need, but this is now way cleaner. I ask myself now if `Sentry.effectLayer` is now the best function name.
2 participants
There are 2 changes in this PR:
About the removal of auto injection of traces, logs or metrics from the
Sentry.effectLayer: this means thatSentry.effectLayeris only initializing the browser/node client. So the usages in comparison for logs and traces:before:
after:
The benefit of this is to have a unified usage between browser and node, which also follows the usage of Effect, so users have to actively set the tracer with
setTraceror replace the logger withLogger.replace. There is no extra opt-in viaenableEffectLogsorenableEffectMetrics, this would also remove the confusion of not having theenableEffectTracesoption (which didn't exist before, nor would have existed in any future versions)This was discussed offline with @Lms24 to have this. In the first alpha we can always shift if there is the need, but this is now way cleaner. I ask myself now if
Sentry.effectLayeris now the best function name.