[2.0] new_scope does not apply scope to unhandled exception

[szokeasaurusrex]

TODO: We should also check whether push_scope in 1.x is affected!

How do you use Sentry?

Sentry Saas (sentry.io)

Version

2.0.0rc2

Steps to Reproduce

Run the following script:

import sentry_sdk

sentry_sdk.init("<dsn>")


with sentry_sdk.new_scope() as scope:
    scope.set_tag("custom tag 1", "value 1")
    1 / 0

Expected Result

The zero division error gets sent to Sentry, and the error event has the tag "custom tag 1" with the value "value 1" set.

Actual Result

Although the zero division error gets reported to Sentry, the error event is missing the tag. This is because the new_scope method restores the original scope within a finally block, which gets called before the scopes are merged and before the event is sent to Sentry. So, the scope that had the tag set is no longer active when the event is prepared for sending.

[adinauer]

Hmm if I look at this the other way around.
Sentry.captureXYZ is called outside of withScope so wouldn't applying data from inside withScope mean we're leaking scopes?

I just confirmed it works the same way in Java SDK with pushScope and popScope in finally. So isn't this the actual behaviour in all SDKs?

What are our options here?

1. Capture unhandled exceptions?
   • probably not what a user wants here
2. Store any unhandled exceptions that throw out of withScope (or similar API) along the exception for later use?
   • How would this work when an exception throws out of multiple withScope? Do we just use the innermost scope?
   • How would this work in the following code example (forgive the lack of python):

...
try {
  Sentry.withScope((scope) -> {
    scope.setTag("custom tag 1", "value 2")
    throw new RuntimeException("...")
  })
} catch (Exception e) {
  Sentry.configureScope((scope) -> { scope.setTag("custom tag 2", "value 2") })
  Sentry.captureException(e)
}

Would this now apply both tags?

When using isolation scope, tags should apply, as long as the exception is handled at some point during request handling.

[Lms24]

I can confirm that this is currently also the behaviour in the JS SDK (which is where we were notified about it in the first place, see getsentry/sentry-javascript#11133).

Leaving aside that the behaviour can be argued in both ways (intended vs bug), I'd like to mention a solution proposed by @jeengbe:

The catch block in our withScope (or language equivalent) API can attach the scope to the error object before popping it. Later, during event processing, we apply this scope to the event, probably with presedence over the other scopes (current, isolation, global). Obviously, this also implies that we need to take care of merging potentially existing scope objects on the error object for nested withScope or withIsolationScope calls.

To be clear: I think we have to continue popping/restoring the scope in the finally block. It's vital we retain this integrity. But attaching the scopes to the error object sounds like an interesting idea. I feel like there are edge cases attached to this solution but can't think of a concrete example. Would appreciate if anyone has thoughts around this.

---

My 2 cents on @adinauer's questions:

1. Capture unhandled exceptions?
   probably not what a user wants here

I agree, withScope APIs shouldn't capture exceptions. It'd be an unexpected side effect of this API leading to errors being caught that users might not want to be sent in the first place.

2. Store any unhandled exceptions that throw out of withScope (or similar API) along the exception for later use?

I think this goes into the direction of the solution outlined above for languages that can't just mess with the error object. Also the sub question about nested withScope calls.

Would this now apply both tags?

Yes, I think in this scenario both tags would make sense from a user perspective. Reasoning: The exception was thrown within the scope of custom tag 1. In the case outlined in getsentry/sentry-javascript#11133, users would like to apply a custom fingerprint to an exception they intentionally throw to be caught down the line. Ultimately the kind of data doesn't make much of a difference but it's a valid use case IMO. For this specific case there are other ways how to achieve propagating the fingerprint (e.g. getsentry/sentry-javascript#11133 (comment)) but they all require additional SDK APIs and/or users to add code.

[mitsuhiko]

For unhandled errors this is expected. There is not much that can be done about that as the unhandled error is handled on a much higher scope. I would close this as non actionable. The solution going forward will be to manipulate the isolate scope.

[Lms24]

Unfortunately, setting values on the isolation scope won't solve the reported issue in a satisfying way. If users set data on the isolation scope within a closure that also throws an error, it will outlive the that closure and be applied to all future events within the lifetime of the isolation scope, not just to the error they're throwing. This means, they need to manually unset that data after they deem it no longer necessary (which could be in a lot of places potentially).

Let's say I have something like this:

Sentry.setTag('global', tag);

function getUsers() {
  // assume there is an isolation scope set by the SDK
  Sentry.getIsolationScope().setTag('local', 'tag');
  throw new Error('This err has two 2 tags now ✅ ')
}

getUsers();

// this is intended behaviour for the isolation scope, no complaints here
// but it shows why mutating the isolation scope is not helpful for the 
// described use case 
throw new Error('This err now has 2 tags ⚠️');

In the reported issue in JS, we're not talking about setting a tag but setting a custom fingerprint which really shouldn't be applied to more events than intended by users 😅

To be clear: I'm fine if we all agree that the status quo is intended behaviour and cases where users want a different behaviour should live in user code. I just want to make sure we're aware that the current behaviour is potentially confusing (in fact I'm really surprised this didn't come up before) for users who work a lot with scopes.

Mental train of thought: I specifically assigned x value to this scope and the error happened within the scope. Why is x not in my error event?. Do I really need to additionally call captureException within the scope?

[jeengbe]

Do I really need to additionally call captureException within the scope?

I think this is a key point. In TS, for instance, there is no practical equivalent to an option type (there are ways, but not very practical ones), so it is most common to throw exceptions instead of using option types and exiting early for unhappy paths. An option type provides no information about the error/why no value is returned, so it is natural that logging is done within the scope.
In throw-land, however, you would either have to:

• Log within the scope and return null?, and have to catch it no further -> stupid idea, any errors that aren't directly try-catched crash the application.
• Log within the scope, re-throw error, root try-catch and make sure to not log it again -> stupid idea, hard to ensure you don't log errors twice.
• Log within the scope, throw non-error object, don't log non-errror objects at the root -> essentially a variation of the above

So in contexts where thrown error objects are commonly used to short-circuit any execution processes, you face the dilemma of either:

• try-catch and log immediately and ensure you don't log it again at the root
• only catch errors at the root but lose scoped information

The solution to this would be to attach the scope to errors, and (opt-in?) consider that when processing the error at the root.

By root, I mean e.g. http middlewares.

[mitsuhiko]

Unfortunately, setting values on the isolation scope won't solve the reported issue in a satisfying way. If users set data on the isolation scope within a closure that also throws an error, it will outlive the that closure and be applied to all future events within the lifetime of the isolation scope, not just to the error they're throwing.

Sure, but that's what it means. The error bubbles to the boundary (the edge of the isolation scope). The alternative would be to add a way to capture exceptions in a block.

import sentry_sdk

sentry_sdk.init("<dsn>")

with sentry_sdk.new_scope(error_boundary=True) as scope:
    scope.set_tag("custom tag 1", "value 1")
    1 / 0

Where if error_boundary=True it adds an implied capture_exception at the end of the block. It would be potentially weird though because it either a) silences the error or b) reports the error twice. Unless the dedup middleware prevents that.