Repository navigation
feat(scope): Split global scope and expose isolation scope - #2160
Conversation
|
706849a to
d5c21b0
Compare
8b7e22e to
4dd460f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## jpnurmi/perf/scope-batch-update #2160 +/- ##
===================================================================
+ Coverage 75.58% 75.61% +0.03%
===================================================================
Files 103 103
Lines 28348 28685 +337
Branches 5192 5290 +98
===================================================================
+ Hits 21427 21691 +264
- Misses 5565 5628 +63
- Partials 1356 1366 +10 🚀 New features to boost your workflow:
|
|
Discussion continued from #2107 (comment)
@limbonaut Unfortunately, a plain getter wouldn't work because returning an owned reference would not be safe, and Here's a proposal to "acquire" a reference, which needs to be released after you're done. If/when adding support for isolation scopes, one could add |
limbonaut
left a comment
There was a problem hiding this comment.
Looks like it needs more work than initially expected 😅 The current implementation causes issues in some sentry_scope_set_x and capture_x functions, such as sentry_scope_set_tag. Also, I believe, changes are needed in the apply_to_x family of functions to check whether the passed scope is actually one of the layered scopes (currently only the global scope is layered). I.e. if (passed_scope == global) passed_scope = null.
Specific examples:
- Acquired scope mutations via
sentry_scope_set_xskip backend flush. sentry_scope_capture_event(scope, event, NULL)does not check for global scope and, for instance, adds breadcrumbs twice.sentry_scope_add_breadcrumbskips crash persistence and filtering.
If you think it's too much overhead for what we currently need, we can still go back to adding dedicated functions for batched updates on the global scope.
| * `sentry_local_scope_new`. | ||
| * `sentry_local_scope_new`, or acquired via `sentry_acquire_global_scope`. | ||
| */ | ||
| SENTRY_API void sentry_scope_free(sentry_scope_t *scope); |
There was a problem hiding this comment.
Yeah, I think release is a better name to pair with acquire, even if free is used underneath. After all, even sentry_value has a dedicated decref function. scope_free can be confusing. Should scopes adopt refcounting semantics in the API akin to sentry_value?
We already use similar semantics in sentry_value_t. I'd add a dedicated release function, as |
|
Thanks, good catch! ❤️ The global mutators in |
06f4d7e to
88ad7b5
Compare
88ad7b5 to
b475fd3
Compare
5923605 to
7fd5154
Compare
7fd5154 to
5fc726d
Compare
|
I'm back with a new proposal with ideas stolen straight from https://develop.sentry.dev/sdk/foundations/state-management/scopes/ 🤓 I ended up adding a separate isolation scope as a child of the global scope, and exposing direct access to that instead. There might still be some quirks and details to tweak, but this already gives a lot cleaner separation between:
For example, clearing the isolation scope preserves the inherited defaults, so there's no need to fight back and try to restore them after clearing. For now, it operates in a "global mode" as sentry-dotnet and sentry-java call it, meaning there's only one process-wide isolation scope. I think it's a good start, makes sense for client applications, and retains backwards compatibility with what we had. Should be doable to extend this later with thread-local isolation scopes as an opt-in, if we want. |
bd58389 to
b6698d0
Compare
b6698d0 to
820f31f
Compare
|
I have a few concerns about this scope model implementation. I'm not sure the global scope is necessarily supposed to be read-only. As I understand it, it's just a scope that's applied to the whole application (i.e. process). The idea behind an isolation scope is to provide a separate layer for clearly defined runtime jobs (think of a single request in a web application that handles multiple requests). But what constitutes a clearly defined job in a native application? I think this can only be defined by users. The specs mention that the isolation scope is supposed to be forked automatically by SDK integrations, but I don't think it's possible to automate this without knowing anything about the app's purpose. Here, the isolation scope feels like another layer in the global sense. It doesn't really provide much isolation. I think it should be more about scope lifetime (maybe thread locality), not scope's mutability. Another way to think about isolation scopes is in terms of the old Hubs model. When users needed isolation for a distinct job, they could make another Hub, which would keep reporting for that job separate from others. |
|
I actually had async signal-safe thread-local storage implementation at first, but then deliberately chose to drop it and start with one process-wide isolation scope. This model is directly inspired by the "global mode" in sentry-dotnet and sentry-java, used by default for client/mobile/desktop apps:
This not only keeps behavioral backwards compatibility, but also operates in the way direct Native SDK users presumably expect by default going forward, matching the "client/mobile/desktop" default in other SDKs. However, the structure is suitable for extending it with opt-in thread-local isolation scopes later. |
0b11a8c to
23a4316
Compare
|
Here, to summarize the discussion we had with @jpnurmi and our conclusions. The global mode for the isolation scope is the right call for most desktop applications. This means that the isolation scope is a layer on top of the global scope, and it's also a process-wide context. Currently, its main purpose is to isolate the data that goes onto global scope from At some point, we may want (not settled yet) to expose the As it currently stands, the global scope would effectively be read-only from public API, but we may add accessors for it in the future. |
There was a problem hiding this comment.
I can see why it's useful from data protection point. It's a good mechanism to avoid dodgy code that keeps re-adding data after clear(). And the mechanism is very straightforward and clean. One remaining question I have: which options should be intializing the global data, and which should go onto isolation scope? Things like attachments, when added through options, cannot be removed via usual means, unless we provide accessor to the global scope. What about changes made via initial_scope, should that go to isolation?
The split is, in a way, analogous to
The |
23a4316 to
c8e99a2
Compare
Split the existing global scope into an internal, read-only global scope for SDK/OS metadata and configured defaults, and a mutable isolation scope used by top-level APIs. Move user data, observers, and backend flushing to the isolation scope. Add `sentry_scope_acquire()` and `sentry_scope_release()` for direct isolation scope access. Implement Sentry's three-scope model with global -> isolation -> local scope layering. Retain parent references and merge layers when preparing events, telemetry, and attachments. Local values take precedence over isolation and global defaults. The SDK follows the global mode used by other Sentry SDKs for client applications: all threads share the same process-wide isolation scope. Changes made through top-level APIs or an acquired reference are shared across all threads. See: - https://develop.sentry.dev/sdk/foundations/state-management/scopes/#isolation-scope
c8e99a2 to
59a151f
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.
Reviewed by Cursor Bugbot for commit 59a151f. Configure here.

Split the existing global scope into:
Move user data, observers, and backend flushing to the isolation scope, and add
sentry_scope_acquire()andsentry_scope_release()for direct access to it:Implement Sentry's three-scope model with global -> isolation -> local scope layering. Retain parent references and merge layers when preparing events, telemetry, and attachments. Local values take precedence over isolation and global defaults.
The SDK follows the "global mode" used by other Sentry SDKs for client applications: all threads share the same process-wide isolation scope. Changes made through top-level APIs or an acquired reference are shared across all threads, retaining backwards compatibility with the previous implementation around a single global scope.
Dev docs: https://develop.sentry.dev/sdk/foundations/state-management/scopes/#isolation-scope