feat(node-experimental): Update to new Scope APIs #9799

mydea · 2023-12-12T12:13:53Z

This PR introduces the new scope APIs to node-experimental.

getCurrentHub() is still around, but just a mock hub that uses other methods under the hood.
Instead, there are the following new APIs:
- getCurrentScope()
- getIsolationScope()
- getGlobalScope()
- withIsolationScope()

Mostly existing tests should cover this OK. The main change here is that for spans, since we use the isolation scope any tags etc. added while the span is running are also added to the resulting event.

For POTEL, we automatically set an isolation scope whenever a http.server span is generated.

Replaces #9419

mydea · 2023-12-12T12:38:24Z

packages/node-experimental/src/integrations/http.ts

+
+            // Update the isolation scope, isolation this request
+            if (getSpanKind(span) === SpanKind.SERVER) {
+              setIsolationScope(Scope.clone(getIsolationScope()));


This is the actual code for auto-isolation with OTEL. Should work for any instrumentation that builds on top of http, so e.g. both express and fastify etc. are covered by this. (in theory, more testing will be needed)

github-actions · 2023-12-13T08:21:58Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	75.17 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	66.6 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	60.19 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.26 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	29.87 KB (0%)
@sentry/browser - Webpack (gzipped)	21.56 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	72.61 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	64.33 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	30.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped)	22.63 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	202.3 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	92.4 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	67.45 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	33.47 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	66.96 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped)	21.6 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	83.74 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	48.41 KB (0%)
@sentry-internal/feedback - Webpack (gzipped)	16.18 KB (0%)

packages/node-experimental/src/integrations/http.ts

lforst · 2023-12-14T09:52:11Z

packages/node-experimental/src/sdk/api.ts

+ * Fork a scope from the current scope, and make it the current scope in the given callback.
+ * Additionally, also setup a new isolation scope for the callback.
+ */
+export function withIsolationScope<T>(callback: (scope: Scope, isolationScope: Scope) => T): T {


I don't fully understand the callback type here.

What scope is the scope argument?

Why do we need the scope argument?

So we have withScope((scope) => {}), which I think is clear - you get the scope as an argument.

Now withIsolationScope() is withScope() that also forks the isolation scope. So my idea was to pass both the forked current scope, as well as the forked isolation scope to the callback 🤔 would it be clearer to name them callback(currentScope: Scope, isolationScope: Scope)? Or we can also leave this away and only pass the isolation scope, and expect users to do getCurrentScope() if they need to inside of the callback 🤔

hmm. Just thinking about things naively I find it odd that withIsolationScope would create more than just an additional scope that happens to be an isolation scope. The way I initially read this function would have been that scope always is the same as isolationScope. Is that not the case? I would expect the currentScope inside withIsolationScope to be the isolation scope.

no, the isolation scope is never a current scope - like the global scope, it is completely separate. otherwise, inheritance of scopes would be messed up - think this:

withIsolationScope(scope => { // scope is both current & isolation scope scope.setTag('xx', 'yy'); withScope(innerScope => { // innerScope inherits from the previous current scope, // which would now be the isolation scope, which is bad }); });

Why is it bad to fork from the isolation scope?

it's bad if that happens implicitly, without the user knowing this. Keep in mind that in 99% of cases users will never call withIsolationScope manually but the isolation scope will be set under the hood, and users should not accidentally mutate it by accessing getCurrentScope() and putting something on it.

I've updated the function syntax to only pass the isolation scope for withIsolationScope.

You can refer to this graphic which hopefully explains what inherits from what where: https://github.com/getsentry/rfcs/blob/c4b1fb4fd2915246760156285393340af093d591/text/0122-images/scope-inheritance.png

packages/node-experimental/src/sdk/api.ts

packages/node-experimental/src/utils/contextData.ts

packages/node-experimental/src/sdk/api.ts

lforst · 2023-12-14T10:21:37Z

packages/node-experimental/src/sdk/scope.ts

+    newScope._tags = { ...this['_tags'] };
+    newScope._extra = { ...this['_extra'] };
+    newScope._contexts = { ...this['_contexts'] };
+    newScope._user = this['_user'];


we should probably also shallow clone user (?)

lforst · 2023-12-14T10:26:17Z

packages/node-experimental/src/sdk/types.ts

+  runWithAsyncContext<T>(callback: () => T): T;
+}
+
+export interface SentryCarrier {


Are we hard-depending on this anywhere? Idealy we get rid of this in v8.

In how far? We still need some way to keep the global stuff, or how would you keep that instead?

I would use a const that is exported instead of a global variable. That way we also won't have collisions if there are multiple Sentry versions on the same browser page for some reasons i believe.

makes sense I think, let's see if I can somewhat easily update this in here already!

lforst · 2023-12-14T10:28:40Z

packages/node-experimental/src/utils/prepareEvent.ts

+export type ExclusiveEventHintOrCaptureContext =
+  | (CaptureContext & Partial<{ [key in keyof EventHint]: never }>)
+  | (EventHint & Partial<{ [key in keyof ScopeContext]: never }>);
+
+/**
+ * Parse either an `EventHint` directly, or convert a `CaptureContext` to an `EventHint`.
+ * This is used to allow to update method signatures that used to accept a `CaptureContext` but should now accept an `EventHint`.
+ */
+export function parseEventHintOrCaptureContext(
+  hint: ExclusiveEventHintOrCaptureContext | undefined,
+): EventHint | undefined {
+  if (!hint) {
+    return undefined;
+  }
+
+  // If you pass a Scope or `() => Scope` as CaptureContext, we just return this as captureContext
+  if (hintIsScopeOrFunction(hint)) {
+    return { captureContext: hint };
+  }
+
+  if (hintIsScopeContext(hint)) {
+    return {
+      captureContext: hint,
+    };
+  }
+
+  return hint;
+}


This is messed up but I don't know what to do about it. Is captureException api just garbage?

Yeah this is because in the global Sentry.captureException() the API used to be captureException(exception, captureContext), while in other places the second argument is EventHint.

We could deprecate this for v8 and require this to be a hint, which means that users need to refactor:

Sentry.captureException(error, { captureContext: { tags: ... } })

🤷

packages/node/src/index.ts

packages/opentelemetry/src/utils/spanData.ts

packages/node-experimental/src/sdk/api.ts

Lms24 · 2023-12-14T10:43:36Z

packages/node-experimental/src/sdk/api.ts

+/**
+ * Fork a scope from the current scope, and make it the current scope in the given callback.
+ * Additionally, also setup a new isolation scope for the callback.
+ */
+export function withIsolationScope<T>(callback: (scope: Scope, isolationScope: Scope) => T): T {
+  const ctx = context.active();
+  const currentScopes = getScopesFromContext(ctx);
+  const scopes = currentScopes
+    ? { ...currentScopes }
+    : {
+        scope: getCurrentScope(),
+        isolationScope: getIsolationScope(),
+      };
+
+  scopes.isolationScope = scopes.isolationScope.clone();
+
+  return context.with(setScopesOnContext(ctx, scopes), () => {
+    return callback(getCurrentScope(), getIsolationScope());
+  });
+}


q: When would we use this function? Asking specifically because I'm not sure if cloning the isolation scope makes sense. Shouldn't a new isolation scope already be fresh and not potentially carry over data from another execution context?

We decided to fork the isolation scope, because otherwise you may loose data. Think of this realistic scenario:

Sentry.init(); Sentry.setTag('xx'); // <-- this actually writes on the initial isolation scope app.get('/my-route', function() { // this has a forked isolation scope - but it has inherited the xx tag from outside! // so the tag is still present on stuff in here }

Does that make sense?

For when to use it, this should mostly be used by ourselves in auto instrumentation, or if users want to do that themselves. Alternatively you can also use setIsolationScope(), which overwrites the isolation scope for the current execution context - whatever works better for the concrete use case.

Ok I guess that makes sense. I was under the impression that Sentry.setTag('xx') would write to the global scope and that we'd only create the first isolation scope when a request hits /my-route. Hence my question. But if we create one beforehand, then it makes sense to fork.

Anyway, my main concern here is avoiding bleed of e.g. user data between isolation scopes which is why I had mixed feelings about forking it.

packages/node-experimental/src/sdk/api.ts

Lms24 · 2023-12-14T11:47:11Z

packages/node-experimental/src/sdk/hub.ts

+/**
+ * This is for legacy reasons, and returns a proxy object instead of a hub to be used.
+ */
+export function getCurrentHub(): Hub {


Should we deprecate this?

That's a good question! I think for node-experimental it doesn't matter, but it's a general thing to think about for v8. I tend to say "yes"!

I mean the whole idea of the scope changes is to get rid of getCurrentHub, so I agree 😅

Lms24

Thanks for answering my questions!

Co-authored-by: Luca Forstner <luca.forstner@sentry.io>

This is a try to move the application of scope data to events out of the scope class into a util function. This changes the flow to be: 1. The scope has a `getScopeData()` method that returns data that should be applied to events. 2. The `prepareEvent` method uses that to actually apply scope data to the event etc. 3. This also makes it much easier to do the experimentation in node-experimental (#9799) because we only need to overwrite this, and can leave the rest of the event processing the same. I _think_ this makes sense but would appreciate some more eyes on this as well. For me the separation makes also more sense, as e.g. the application of event processors etc. should not really be tied to the scope at all. This is also is a first step to then allow us to add global scopes more easily. --------- Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

mydea requested review from lforst, Lms24 and AbhiPrasad December 12, 2023 12:13

mydea self-assigned this Dec 12, 2023

mydea commented Dec 12, 2023

View reviewed changes

mydea mentioned this pull request Dec 12, 2023

feat(core): Extract scope application to util #9804

Merged

mydea force-pushed the fn/node-experimental-scopes branch from f6ba1cd to c25d4e8 Compare December 13, 2023 08:13

This was referenced Dec 14, 2023

ref(core): Replace Scope.clone() with non-static scope.clone() #9801

Merged

Update Hub & Scope handling #9841

Closed

lforst reviewed Dec 14, 2023

View reviewed changes

Lms24 reviewed Dec 14, 2023

View reviewed changes

mydea force-pushed the fn/node-experimental-scopes branch from d57996f to 9730647 Compare December 18, 2023 09:05

lforst approved these changes Dec 18, 2023

View reviewed changes

Lms24 approved these changes Dec 18, 2023

View reviewed changes

mydea and others added 15 commits December 19, 2023 08:54

feat(node-experimental): Update to new Scope APIs

5a0c65f

auto set for http

02495bf

fix capture methods

3e70cf0

linting fixes

908c2c2

fix linting

680e251

WIP better scope type

f35a961

fixes to scope & use scope.clone()

817b2ac

fix circular dep checks

2054d00

fix scope clone

28180f0

add more tests & exports

ef387f6

add more tests & fix attachments

2606d33

Apply suggestions from code review (typos)

3d18735

Co-authored-by: Luca Forstner <luca.forstner@sentry.io>

PR feedback

9be6bb4

ref: Update withIsolationScope to not pass current scope

40dc544

shallow clone user

13c7856

mydea force-pushed the fn/node-experimental-scopes branch from 45272eb to 13c7856 Compare December 19, 2023 08:02

mydea merged commit 605fd50 into develop Dec 19, 2023
94 checks passed

mydea deleted the fn/node-experimental-scopes branch December 19, 2023 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node-experimental): Update to new Scope APIs #9799

feat(node-experimental): Update to new Scope APIs #9799

mydea commented Dec 12, 2023

mydea Dec 12, 2023

github-actions bot commented Dec 13, 2023 •

edited

lforst Dec 14, 2023

mydea Dec 14, 2023

lforst Dec 14, 2023

mydea Dec 15, 2023

lforst Dec 18, 2023

mydea Dec 18, 2023

lforst Dec 14, 2023

lforst Dec 14, 2023

mydea Dec 14, 2023

lforst Dec 14, 2023

mydea Dec 14, 2023

lforst Dec 14, 2023

mydea Dec 14, 2023

Lms24 Dec 14, 2023

mydea Dec 14, 2023

Lms24 Dec 18, 2023 •

edited

Lms24 Dec 14, 2023

mydea Dec 14, 2023

Lms24 Dec 18, 2023

Lms24 left a comment

feat(node-experimental): Update to new Scope APIs #9799

feat(node-experimental): Update to new Scope APIs #9799

Conversation

mydea commented Dec 12, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2023 • edited

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 Dec 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2023 •

edited

Lms24 Dec 18, 2023 •

edited