feat(node-core): Add node-core/light #18502
Conversation
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.
|
andreiborza
force-pushed
the
ab/node-core-light
branch
5 times, most recently
from
1687319 to
773cb0a
Compare
andreiborza
force-pushed
the
ab/node-core-light
branch
2 times, most recently
from
5f3ebcb to
f5bdfc9
Compare
size-limit report 📦
|
andreiborza
force-pushed
the
ab/node-core-light
branch
3 times, most recently
from
748eb5f to
fdb1403
Compare
Codecov Results 📊Generated by Codecov Action |
| "dependencies": { | ||
| "@apm-js-collab/tracing-hooks": "^0.3.1", | ||
| "@sentry/core": "10.38.0", | ||
| "@sentry/opentelemetry": "10.38.0", |
There was a problem hiding this comment.
Technically, this is a breaking change. That being said, the only place where we document node-core is in the README and the README already advised users to install @sentry/opentelemetry so... maaaybe this is ok?
sentry-javascript/packages/node-core/README.md
Lines 23 to 30 in 356776c
andreiborza
force-pushed
the
ab/node-core-light
branch
from
fdb1403 to
da1124d
Compare
|
To reviewers: Sorry for the big PR, I tried to split this up via commits. The first one adds all the core logic, the rest are testing related. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| serverName, | ||
| }; | ||
|
|
||
| applySdkMetadata(clientOptions, 'node'); |
There was a problem hiding this comment.
Should we mark this as node-light here?
There was a problem hiding this comment.
Right, yea we can. We didn't want to do that for node-core because every node SDK is based on it but I think it makes sense for node-light.
There was a problem hiding this comment.
Might be hard to miss adding exports here. Should we do something similar like dev-packages/e2e-tests/test-applications/node-exports-test-app? Or maybe add this?
There was a problem hiding this comment.
Good idea, I'll add a test app.
There was a problem hiding this comment.
alternatively, is there a way to have a common exports file so that we end up with discepancies in the first place? Just a thought, feel free to disregard
There was a problem hiding this comment.
Yep, went with a common exports file in efc79f8.
| maxRequestBodySize?: 'none' | 'small' | 'medium' | 'always'; | ||
| } | ||
|
|
||
| const _httpServerIntegration = ((options: HttpServerIntegrationOptions = {}) => { |
There was a problem hiding this comment.
l: Should we call this something else like HttpServerLightIntegration? Might be confusing to have this flying around a couple of times
There was a problem hiding this comment.
No I think this would be more confusing from a user's perspective. The light SDK has only one http server integration, calling it something else would only really benefit us maintainers. I think this is fine as is tbh.
| * in light mode (without OpenTelemetry). | ||
| * | ||
| * This is a lightweight alternative to the OpenTelemetry-based httpServerIntegration. | ||
| * It uses Node's native AsyncLocalStorage for scope isolation and Sentry's continueTrace for propagation. |
There was a problem hiding this comment.
Small docs thing to be more specific:
"for trace propagation"
| return target.apply(thisArg, args); | ||
| } | ||
|
|
||
| DEBUG_BUILD && debug.log(INTEGRATION_NAME, 'Handling incoming request (light mode)'); |
There was a problem hiding this comment.
Maybe call it "(node-light)" -> People could think about a theme when first reading "light mode" and might be confused.
There was a problem hiding this comment.
I removed this completely in f1084ce because there's no other mode when you use the light SDK and it's potentially confusing to users.
| } | ||
|
|
||
| // Update the isolation scope, isolate this request | ||
| isolationScope.setSDKProcessingMetadata({ normalizedRequest, ipAddress }); |
There was a problem hiding this comment.
M: Setting the IP address should be guarded with sendDefaultPii. I think this is not done here.
There was a problem hiding this comment.
Great catch! should be h: really 😅
There was a problem hiding this comment.
Fixed in ed91667 and added tests around this. Technically this will never end on envelopes because SDKprocessingMetaData is always wiped off of envelopes but it's better to guard against it here anyway.
I will send a separate PR for the same issue in node-core.
There was a problem hiding this comment.
Removed again in e3c2355 because otherwise the integration option for the RequestData integration won't take effect.
We have two safeguards for this not making it to the final event:
- the RequestData integration falls back to checking
sendDefaultPiiif its own option is not provided - envelope.ts deletes all
sdkProcessingMetadataoff the event before submitting it
| } | ||
|
|
||
| function withIsolationScope<T>(callback: (isolationScope: Scope) => T): T { | ||
| // FIX: Clone current scope as well to prevent leakage between concurrent requests |
There was a problem hiding this comment.
Q: Is this something that still needs to be fixed?
There was a problem hiding this comment.
It's already in here, I left the comment to indicate that this was previously not done but I think I'll just remove it. Now that I read it again it feels out of place.
andreiborza
force-pushed
the
ab/node-core-light
branch
from
947923f to
a0cdf5f
Compare
| import { expect, test } from '@playwright/test'; | ||
| import { waitForError } from '@sentry-internal/test-utils'; | ||
|
|
||
| test('should isolate scope data across concurrent requests', async ({ request }) => { |
There was a problem hiding this comment.
m: can we still test againt the traceId in event.contexts.trace? IIUC Node-light doesn't start any spans but it should still recycle trace ids for errors. Otherwise the product will show related errors that have nothing to do with each other.
There was a problem hiding this comment.
m: another test scenario I think we should cover: What happens when a service instrumented with node-code/light would receive incoming sentry-trace and baggage headers?
| // eslint-disable-next-line deprecation/deprecation | ||
| export { anrIntegration, disableAnrDetectionForCallback } from '../integrations/anr'; |
There was a problem hiding this comment.
l: WDYT about not even exporting the deprecated exports? Theoretically this isn't breaking and we don't have to do it later this way. But no strong feelings either way.
There was a problem hiding this comment.
Ah, yea I like that idea. No reason to ship something new that'll be immediately taken away.
There was a problem hiding this comment.
worst-case, if anyone requests it we can still add it 😅
packages/node-core/src/light/sdk.ts
Outdated
| // eslint-disable-next-line deprecation/deprecation | ||
| inboundFiltersIntegration(), |
There was a problem hiding this comment.
l: we could make the switch here already (see other comment)
packages/node-core/README.md
Outdated
| NODE_OPTIONS="--import ./instrument.mjs" npm run start | ||
| ``` | ||
|
|
||
| ## Errors-only Lightweight Mode |
There was a problem hiding this comment.
m: I feel like marketing node-light as "Errors-only" is underselling it a bit. Logs and metrics also work in this package. Assuming I didn't miss something, all three telemetry items would still be trace-connected so users can still make use of it. It even handles Tracing without Performance (god this name didn't age well).
I also struggle with a great name alternative but what about something like
- Span-less Lightweight Mode
- Passive Tracing Mode
- Just plain old "Lightweight Mode" ?
There was a problem hiding this comment.
I agree, it's definitely not an errors-only SDK and calling it that will probably be misleading and confusing for users and at worst will have people not use any other features even if they would have otherwise.
I also struggled with naming this... I think I'll go with "Lightweight Mode" and rely on the descriptions to explain what that means.
Thanks for the input on this!
packages/node-core/README.md
Outdated
|
|
||
| > **⚠️ Experimental**: The `@sentry/node-core/light` subpath export is experimental and may receive breaking changes in minor or patch releases. | ||
|
|
||
| If you only need error monitoring without performance tracing, you can use the lightweight mode which doesn't require OpenTelemetry dependencies. This mode is ideal for: |
There was a problem hiding this comment.
l: I would at least scratch "performance", maybe even replace "performance tracing" with:
| If you only need error monitoring without performance tracing, you can use the lightweight mode which doesn't require OpenTelemetry dependencies. This mode is ideal for: | |
| If you only need error monitoring without spans, you can use the lightweight mode which doesn't require OpenTelemetry dependencies. This mode is ideal for: |
or maybe something like
| If you only need error monitoring without performance tracing, you can use the lightweight mode which doesn't require OpenTelemetry dependencies. This mode is ideal for: | |
| If you only need error monitoring, logs and metrics but no spans, you can use the lightweight mode which doesn't require OpenTelemetry dependencies. This mode is ideal for: |
wdyt?
There was a problem hiding this comment.
Right, something like that. I'm curious about no spans since manual span creation is still intended.
Light mode supports logs, metrics, and distributed tracing — not just error tracking. Rename section from "Errors-only" to "Lightweight Mode" and update feature list accordingly.
andreiborza
force-pushed
the
ab/node-core-light
branch
from
2dd6c29 to
b1ef70d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
packages/node-core/src/light/integrations/httpServerIntegration.ts
Outdated
Show resolved
Hide resolved
Avoids drift between the two entry points by keeping shared exports in a single file. Entry-point-specific and deprecated exports remain in their respective index files.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
packages/node-core/src/light/integrations/httpServerIntegration.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for addressing my feedback. I had a couple of more L-level suggestions. We shouldn't mention "Performance Monitoring" anymore. I'd directly call out the absence of automatically started spans instead (left some suggestions into that direction).
Another suggestion: We should have 1-2 tests that covers trace propagation. Ideally for both, http as well as fetch requests. Happy to leave it up to you whether these are unit or e2e tests.
CHANGELOG.md
Outdated
|
|
||
| - **feat(node-core): Add node-core/light ([#18502](https://github.com/getsentry/sentry-javascript/pull/18502))** | ||
|
|
||
| This release adds a new light-weight `@sentry/node-core/light` export to `@sentry/node-core`. The export acts as a light-weight errors-only SDK that does not depend on OpenTelemetry. |
There was a problem hiding this comment.
l: something like this
| This release adds a new light-weight `@sentry/node-core/light` export to `@sentry/node-core`. The export acts as a light-weight errors-only SDK that does not depend on OpenTelemetry. | |
| This release adds a new light-weight `@sentry/node-core/light` export to `@sentry/node-core`. The export acts as a light-weight SDK that does not depend on OpenTelemetry and emits no spans. |
packages/node-core/README.md
Outdated
|
|
||
| If you don't need automatic spans/transactions, you can use the lightweight mode which doesn't require OpenTelemetry dependencies. This mode is ideal for: | ||
|
|
||
| - Applications that don't need automatic performance monitoring |
There was a problem hiding this comment.
l:
| - Applications that don't need automatic performance monitoring | |
| - Applications that don't need automatically started spans |
There was a problem hiding this comment.
I rewrote this to match the changelog entry more. Added in b333218.
CHANGELOG.md
Outdated
| This release adds a new light-weight `@sentry/node-core/light` export to `@sentry/node-core`. The export acts as a light-weight errors-only SDK that does not depend on OpenTelemetry. | ||
|
|
||
| Use this SDK when: | ||
| - You only need error tracking without performance monitoring |
There was a problem hiding this comment.
l:
| - You only need error tracking without performance monitoring | |
| - You only need error tracking without tracing data (no spans) |
packages/node-core/README.md
Outdated
| **Caveats:** | ||
|
|
||
| - Manual isolation prevents scope data leakage between requests | ||
| - However, **distributed tracing will not work correctly** - incoming `sentry-trace` and `baggage` headers won't be automatically extracted and propagated | ||
| - For full distributed tracing support, use Node.js 22+ or the full `@sentry/node` SDK with OpenTelemetry |
There was a problem hiding this comment.
l: I think the limitation is fine but WDYT about calling out node compatibility right at the top of the node-light section? Something like
This SDK requires Node 22 for full functionality. If you're using lower Node versions, this SDK only offers limited tracing support. Consider using
@sentry/nodeor@sentry/node-coreinstead.
Alternatively, we directly require Node >= 22 for this subpath export and make everything below "User at your own risk"
There was a problem hiding this comment.
Good idea, I moved the warning up but also left this one in to be sure. Added with b333218.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
4 participants
This PR adds a lightweight version of the node-core SDK that doesn't include nor requires OpenTelemetry dependencies. It provides a basic error-tracking SDK with support for request isolation via AsyncLocalStorage and basic tracing abilities
via our
Sentry.startSpan*apis.Request isolation requires Node 22.12.0+. On lower Node versions, manual wrapping via
Sentry.withIsolationScopeis necessary to isolate requests.Closes #19157 (added automatically)