Repository navigation
Request isolation is broken when using Sentry v8 with existing OTel setup #13055
Closed as not planned
Description
Is there an existing issue for this?
- I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
- I have reviewed the documentation https://docs.sentry.io/
- I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/node
SDK Version
8.20.0
Framework Version
N/A
Link to Sentry event
N/A
SDK Setup/Reproduction Example
https://github.com/nwalters512/sentry-memory-leak-repro
This example uses a minimal Sentry + OTel setup, with code/structure being copied directly from the Sentry + OTel documentation.
Steps to Reproduce
- Clone the reproduction repository
- Run
npm install - Run
npm run start - Run
curl http://localhost:3000/a few times
Expected Result
I would expect the log from each request to show just a single event processor. This would indicate that event processors aren't leaking between requests and thus that request isolation is working correctly.
Actual Result
Every time a request comes in, another event processor is added to the existing scope, indicating that the same scope is being shared among all events. In practice, this will lead to a memory leak, since the array of event processors will grow without bounds.
Activity
mydea commentedon Jul 26, 2024
Hey,
so your setup has a couple of issues.
You write:
What docs did you follow to set this up? I am just asking because the docs I am aware of about a custom otel setup: https://docs.sentry.io/platforms/javascript/guides/node/tracing/instrumentation/opentelemetry/#using-a-custom-opentelemetry-setup explain a quite different setup:
skipOpenTelemetrySetup: truein yourSentry.init({})dsn? (this should not break stuff, but just wondering)When your setup is correct, can you try again?
nwalters512 commentedon Jul 26, 2024
For OTel, I followed the official "getting started" documentation: https://opentelemetry.io/docs/languages/js/getting-started/nodejs/
For Sentry, I followed the "ESM (MJS)" docs under "installation methods": https://docs.sentry.io/platforms/javascript/guides/node/install/esm/.
Hmm, honestly, I didn't know those docs existed, but even then, I wouldn't say this is a "custom" OTel setup. This is a bog-standard setup from the official "getting started" documentation. I see the actual page says "If you already have OpenTelemetry set up yourself, you can also use your existing setup", which I guess applies here. But if that's the case, I would expect this to be part of the top-level setup instructions, not buried under "Set Up Tracing" (I don't use tracing, so this doesn't apply to me). Even the docs for
@sentry/opentelemetrystate "This package allows you to send your OpenTelemetry trace data to Sentry via OpenTelemetry SpanProcessors" - it makes no mention of this being necessary for the OG basic Sentry features to work correctly.All that said: adding just
contextManager: new Sentry.SentryContextManager()to theNodeSDKconstructor options seems to have fixed the context isolation issue.From looking at the
wrapContextManagerClassfunction, I can understand on a technical level why a custom context manager seems to be required: you're trying to provider interop between Sentry's bespoke "scope" API and the official OTel "contex" API, which is an admirable goal. If I can offer up my 2 cents: your current documentation isn't upfront enough about the fact that the Sentry SDK is now a fancy OTel wrapper and that you need a very specific OTel setup to function correctly. Not just for traces, as your documentation implies, but for all SDK functionality. It would also be very valuable to explain why these additional pieces are required, as opposed to just asserting that they are. For instance: it's not clear to me if/why I'd need a custom span processor, sampler, or propagator if I'm not using Sentry tracing.nwalters512 commentedon Jul 26, 2024
Additional feedback from trying to get things working:
SentrySamplerbecause I use my own sampling strategy (again, I'm not using Sentry tracing).SentrySpanProcessorwithout also includingSentrySampler, 100% of my traces (which I don't want going to Sentry!) are set to Sentry.So, it seems like only
SentryContextManageris required if one wants to use Sentry for error reporting but not tracing. This contradicts whatvalidateOpenTelemetrySetup()claims:sentry-javascript/packages/node/src/sdk/index.ts
Lines 198 to 205 in a880712
mydea commentedon Jul 29, 2024
Hey,
yeah we are aware that our docs on this are not ideal and a bit buried as of now - we are currently reworking these docs here: getsentry/sentry-docs#10872 where all of this should become more prominent and clear, hopefully.
Generally, also the other parts (sampler, propagator) are required for everything to work properly, even if you do not use tracing with Sentry. This is because we depend on this for trace propagation etc. to work as expected.
The span processor is actually not needed when you do not use sentry for tracing - I will adjust the validation logic to reflect this: #13079
5 remaining items
nwalters512 commentedon Aug 1, 2024
Interesting! We don't use Sentry in a way that spans multiple services or frontend/backend, but that's good to know. This is exactly the kind of thing I'd hope to see in the updated documentation.
chargome commentedon Aug 2, 2024
Feel free to read over the updates and leave feedback: getsentry/sentry-docs#10872
getsantry commentedon Oct 9, 2024
This issue has gone three weeks without activity. In another week, I will close it.
But! If you comment or otherwise update it, I will reset the clock, and if you remove the label
Waiting for: Community, I will leave it alone ... forever!"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀