feat: Add docs about request isolation in Node SDK

[mydea]

This adds docs for how to use withIsolationScope.

Additionally, it also adds a new section (for server SDKs) under "enriching events" describing how process isolation works. We lack docs for this as of now, and this is something users may def. fall into.

It would be ideal if we could somehow auto-fork this for middlewares, but in the meanwhile...

See getsentry/sentry-javascript#12191 (reply in thread) for how this affects users.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
changelog	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 10, 2024 8:03am
sentry-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 10, 2024 8:03am

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
develop-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 10, 2024 8:03am

[codecov]

Bundle Report

Changes will increase total bundle size by 537 bytes (0.0%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
sentry-docs-server-cjs	7.46MB	546 bytes (0.01%) ⬆️
sentry-docs-edge-server-array-push	257.07kB	3 bytes (-0.0%) ⬇️
sentry-docs-client-array-push	6.43MB	6 bytes (-0.0%) ⬇️

[Lms24]

I'm a bit concerned about the middleware thing tbh. I always thought the http instrumentation would create the root span and fork the isolation scope before any middleware is evaluated. After all, we have middleware child spans inside an http.server span 🤔

Docs-wise, I just had a couple of suggestions but good idea to more add more content for the isolation scope!

[Lms24]

not actionable for docs but
that's a massive limitation actually 😬 I was under the impression that the OTEL http instrumentation would fork the isolation scope automatically before any express middleware is evaluated. Is that not the case?

[nwalters512]

In my app, this is the case with the "standard" OTel instrumentation packages. I can't speak to Sentry's internals, but it's very very strange that this doesn't just work out of the box.

Regardless of that: the language "when the request is processed" is, to me, imprecise and maybe even inaccurate. I consider middleware execution to very much be part of "request processing" - after all, I have middleware parsing request bodies, performing authorization checks, redirecting authenticated requests to the login page, and so on. My app has many requests that are completely handled by middleware!

[mydea]

so in otel you do not have this concept of scopes and capturing data on them for a request etc., so this problem does not really exist there, as far as I can tell.

We have the problem that the way that the auto-isolation works is actually forking the isolation scope after a http.server span is emitted. And this is of course only emitted once the request is actually "done" (or at least started), not when the middlewares run. I agree that this kind of sucks and we should see if we find a way to solve this better. I'll adjust the wording for this for now!

[mydea]

OK, I did some more digging - actually this is not correct! Middlewares are actually correctly isolated in express, so I will remove this section again (the general content is still good).

@nwalters512 the problem in your app is not a fundamental problem with middlewares, but it happens because you added http instrumentation yourself, which overwrites ours (and this is what handles the auto isolation). See https://docs.sentry.io/platforms/javascript/guides/node/opentelemetry/custom-setup/#custom-http-instrumentation for details on this. Sentry relies on the http instrumentation for a bunch of things. It would be nice to find a way to warn about this, though 🤔

[sbriceland]

Hmmm.. if we can't create an instance of of OTEL http instrumentation, how are we supposed to go about registering it with our existing OTEL setup?

You're suggesting:

// do not import/create a new HttpInstrumentation instance
// import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
const sentryHttpIntegration = Sentry.httpIntegration();

const sentryClient = Sentry.init({
    dsn: process.env.SENTRY_DSN,
    integrations: (defaultIntegration) =>
        defaultIntegration.map((integration) => (integration.name === 'Http' ? sentryHttpIntegration : integration)),
    release,
    environment,
    skipOpenTelemetrySetup: true,
    tracesSampleRate: 1.0,
});

if that's the case what we're unable to register the instrumentation

[mydea]

You don't need to add the http instrumentation yourself at all - just do not run registerInstrumentations({ instrumentation: [new HttpInstrumentation()] }), this happens automatically under the hood. Does that lead to problems/does not work as expected for you, with your custom setup? 🤔 We could expose our HttpInstrumentation and allow you to add this yourself, I guess, if that's necessary!

[sbriceland]

I updated my demo sentry + OTEL setup as follows:

import * as Sentry from '@sentry/node';
import { SentryPropagator, SentrySampler } from '@sentry/opentelemetry';
// import { registerInstrumentations } from '@opentelemetry/instrumentation';
// import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express';
// import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { BatchSpanProcessor } from '@opentelemetry/sdk-trace-base';
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';

const release = 'sentry-scope-testing';
const environment = 'local';

const sentryClient = Sentry.init({
    dsn: process.env.SENTRY_DSN,
    beforeSend: (event, hint, ...args) => {
        const { contexts, exception, extra, tags, message, user } = event;
        console.dir(
            {
                whoami: 'sentry:beforeSend',
                event: { contexts, exception, extra, tags, message, user },
                hint,
                args,
            },
            { depth: null }
        );
        return event;
    },
    release,
    environment,
    skipOpenTelemetrySetup: true,
    tracesSampleRate: 1.0,
});

const provider = new NodeTracerProvider({
    sampler: new SentrySampler(sentryClient!),
});

provider.addSpanProcessor(
    // mock processor to avoid polluting console output
    new BatchSpanProcessor({
        export: (_spans, cb) => cb({ code: 0 }),
        shutdown: () => Promise.resolve(),
        forceFlush: () => Promise.resolve(),
    })
);

provider.register({
    propagator: new SentryPropagator(),
    contextManager: new Sentry.SentryContextManager(),
});

// registerInstrumentations({
//     instrumentations: [new ExpressInstrumentation(), new HttpInstrumentation()],
// });

Sentry.validateOpenTelemetrySetup();

export const initializedSentry = Sentry;

Then for the express middleware, i removed forking of the isolation scope to see if Middleware is correctly isolated in express:

const auth: RequestHandler = (req, res, next) => {
    const authUser = Users.find((u) => u.id === req.headers['authorization']);
    // Sentry.withIsolationScope(() => {
    if (!authUser) {
        Sentry.setTag('Authenticated', false);
        Sentry.setTag('UserID', null);
        Sentry.setUser(null);
        next(new Error('Authentication Error'));
    } else {
        Sentry.setTag('Authenticated', true);
        Sentry.setTag('UserID', authUser.id);
        Sentry.setUser(authUser);
        res.locals.authUser = authUser;
        next();
    }
    // });
};

However, my output reverted to showing the issue:

{
  whoami: 'sentry:beforeSend',
  event: { ... },
    exception: { ... },
    extra: {
      expectedUser: { id: '4', email: 'foo4@example.com', name: 'foo example4' }
    },
    tags: { Authenticated: true, UserID: '1' },
    message: undefined,
    user: { id: '1', email: 'foo@example.com', name: 'foo example' }
  },
  hint: { ... },
  args: []
}

[sbriceland]

Furthermore, switching my "mock exporter" to the ConsoleSpanExporter does not print an OTEL spans

provider.addSpanProcessor(
    new BatchSpanProcessor(new ConsoleSpanExporter())
    // mock processor to avoid polluting console output
    // new BatchSpanProcessor({
    //     export: (_spans, cb) => cb({ code: 0 }),
    //     shutdown: () => Promise.resolve(),
    //     forceFlush: () => Promise.resolve(),
    // })
);

[mydea]

I opened a PR here: sbriceland/sentry-debug#1 where we can discuss this more!

[Lms24]

l: I'd move the sentence about bg jobs after the user/middleware example for consistency

Suggested change

	This is also necessary if you want to isolate a non-request process, e.g. a background job.
	The following example shows how you can use `withIsolationScope` to attach a user and a tag in an auth middleware:
	```javascript
	const auth = (req, res, next) => {
	Sentry.withIsolationScope(() => {
	const authUser = findUserForHeader(req.headers["authorization"]);
	if (!authUser) {
	Sentry.setTag("Authenticated", false);
	Sentry.setUser(null);
	next(new Error("Authentication Error"));
	} else {
	Sentry.setTag("Authenticated", true);
	Sentry.setUser(authUser);
	next();
	}
	});
	};
	```
	This way, the user & tag will only be attached to events captured during the request that passed the auth middleware.
	The following example shows how you can use `withIsolationScope` to attach a user and a tag in an auth middleware:
	\```javascript
	const auth = (req, res, next) => {
	Sentry.withIsolationScope(() => {
	const authUser = findUserForHeader(req.headers["authorization"]);
	if (!authUser) {
	Sentry.setTag("Authenticated", false);
	Sentry.setUser(null);
	next(new Error("Authentication Error"));
	} else {
	Sentry.setTag("Authenticated", true);
	Sentry.setUser(authUser);
	next();
	}
	});
	};
	\```
	This way, the user & tag will only be attached to events captured during the request that passed the auth middleware.
	Manually calling `withIsolationScope` is also necessary if you want to isolate a non-request task, like background jobs or queue task.

[Lms24]

argh the nested ``` lines destroy the diff. I just moved the bg job sentence to the bottom and slightly adjusted it.

[mydea]

hmm, I do not want to bury this at the very bottom because it is actually a very important aspect that we don't really cover anywhere 🤔 like, if you have a background job or similar you will definitely always have to do this manually 🤔

[sbriceland]

Thanks for taking the time to improve the docs! I just wanted to add my two cents in case it may help some folks down the road.

From the current documentation:

This portion of the existing docs is what lead me to believe that I could use the Sentry.setXXX methods such that when a Sentry.captureXXX is called it would auto-magically have the appropriate scoped data.

While this is an accurate description of Isolation Scope behavior, I believe it would be helpful to call out when you are inside of an isolation scope and a reference to your updated docs here. This way developers have a heads up that Isolation Scope isn't guaranteed e.g. when inside of middleware, unless the isolation scope is manually forked/setup.

[sbriceland]

It would be ideal if we could somehow auto-fork this for middlewares, but in the meanwhile...

On that point, either the @sentry/node lib could include a helper method like the following. Or since it's simple enough, the docs could let people know something like this should work for Express

import * as Sentry from '@sentry/node';
import type { RequestHandler } from 'express';

export const setupExpressIsolationScopeMiddleware: RequestHandler = (_req, _res, next) => {
    Sentry.withIsolationScope(() => {
        next();
    });
};

Set up isolation scope pre request for Express middleware:

const app: Application = express();

// before routes are added to app
app.use(setupExpressIsolationScopeMiddleware);

[mydea]

After some more digging, I figured out that actually middlewares do work as expected, the problem was a different one - see #11378 (comment).

However, this PR is still good to have because this content was missing, which was an oversight anyhow!

[Lms24]

Thanks for making the changes! LGTM now!

[lizokm]

Made a few comments. Looks good!

[mydea]

FYI I updated this and incorporated the feedback, will merge it soon.

[BYK]

For a person who is not very familiar with Sentry SDK, this sentence is probably too dense for me. I know what request isolation and isolating scopes are but what exactly is "forking a scope". What is the current scope and how it is determined? Why is is meant to be forked less frequently and how is it relevant to me or this conversation? What are those "most cases" where the SDK handles for me. At least what kind of cases are meant to be automatically handled?

[mydea]

we (try to) explain these things in the page above this section (see https://docs.sentry.io/platforms/javascript/guides/node/enriching-events/scopes/). I do agree that all of this is not super easy to understand, feel free to PR changes to wording to this!

[BYK]

I do agree that all of this is not super easy to understand, feel free to PR changes to wording to this!

That's the thing! I would love to but I 100% don't understand what's going on. The questions I raised in my first comment are questions I don't know the answers to 😅

[elliotdickison]

I am initializing Sentry for Express as instructed in the setup guide (making sure that Sentry is initialized before express is imported), do not have any custom instrumentation, and am setting tags on the isolation scope per the instructions here. Despite all that tags are still not set per-request. This is a massive problem for us. Any ideas?