Merge Hub & Scope in SDKs #122
Conversation
text/0122-sdk-hub-scope-merge.md
Outdated
| ```js | ||
| const client1 = new Client(); | ||
| const client2 = new Client(); | ||
|
|
||
| const scope1 = client1.getScope(); | ||
| const scope2 = client2.getScope(); | ||
|
|
||
| scope1.captureException(); // <-- isolated from scope2 | ||
| ``` |
text/0122-sdk-hub-scope-merge.md
Outdated
|
|
||
| `client.getScope()` should return the current scope of this client in the current execution context. | ||
|
|
||
| From a users perspective, this should mostly not be noticeable - they can always run `getScope()` to get the current scope, or `withScope(callback)` to fork a new scope off the current scope. |
There was a problem hiding this comment.
This has been a major pain point in the past: withScope() was used in places where we actually wanted to change the current scope, but instead created a fork and then people were wondering why the changes were not visible on the original scope. Not sure if feasible within this RFC, but renaming withScope() to forkScope() would help here.
There was a problem hiding this comment.
I have no strong feelings about withScope, interested to hear what others think on this! I'd be fine with either.
There was a problem hiding this comment.
I find forkScope better, also considering this:
You can still clone scopes manually the same way as before, e.g. via
Scope.clone(oldScope)or a similar API. In contrast towithScope(), this will not fork an execution context.
Both essentially create a new scope off of an existing one, but in different ways. It'd be good to have the name communicate what the difference is.
There was a problem hiding this comment.
I think the main reason to keep withScope is to avoid breaking as much as possible. We could also consider keeping withScope (deprecated?) and adding forkScope as an alias.
For reference only, OTEL uses context.with(callback) to do the same, so it seems we are not the only ones using this wording. Doesn't mean we have to keep it, just pointing this out :)
There was a problem hiding this comment.
Isn't withScope() mainly used to create a temporary scope which is only applied to the callback?
I don't see a good reason to rename/remove this.
There was a problem hiding this comment.
IMHO generally we should strive to replace this usage of withScope - only to add a temporary event processor - with different pattern(s). (not possible for all cases, but most of them). So generally there should be little (not none) need to use it manually!
Co-authored-by: Markus Hintersteiner <[email protected]>
|
Updated this based on feedback in regard to scope<>client relationship. Now, a scope holds a reference to a client, which can be set via |
This is something that should not concern most users, but for those who need it (multiple clients...) we need to make sure stuff still works/is possible to do. But yes, we should def. focus on making the "regular" use case most convenient and abstracted away! |
|
I read it and everything looks reasonable. How clients and the noop client are handled i have not yet fully figured out for python. But I guess I need to tinker with some code to fully get the picture and give meaningful feedback. Will try to make time for this this week. |
|
I don't have much to add from the Mobile perspective because, on Mobile, we encourage our users to call the static APIs
I agree with @adinauer. I would actually love to make the client internal/private. I would prefer passing options to the scope, and that's all the flexibility you get. On Cocoa, we don't support creating your own implementation of a hub, client, or transport, and nobody ever complained. Our only complaints so far are that some customers want to send the data to Sentry and their backend, but that has no impact on this RFC. |
There are users who customize clients, e.g. to support multi DSN or send events somewhere else as well. While rare this is still a use case we probably shouldn't drop without good reason. |
IMHO the goal should be to hide the client away as much as possible by default (which we already do anyhow generally, via |
|
I applied all the feedback and moved this out of draft - I think this is ready to "really" review! |
|
From RN point of view I don't we can merge Hub and Scope. 99% time users can use RN Uses only one Hub and extends browser JS implementation of Scope to synchronize with the native SDKs (iOS, Android). Since RN heavily depends on browser JS SDKs, we will have to adopt it basically immediately when this changes in JS. Event if the native SDKs haven't made the change at the time, it should not affect the scope synchronizing. |
This change will def. happen in a major only, so there should be enough time etc. to sync this up with RN! |
|
After talking to more mobile folks, we don't see a problem with moving forward with this, but we don't see a need to change that cause it doesn't cause any problems in our SDKs. We will most likely make that change in one of our next major versions, but we won't do a major version because of that. |
|
After talking about this with @mitsuhiko , I updated this RFC to again include the (updated) concepts of a global scope, as well as an isolation scope. The idea is that ALL SDKs should have the concepts. Although it may be less relevant for some - but in these cases, they will simply do nothing anyhow. But esp. for isomorphic envs like JS, the same APIs should exist for server & client. Please re-read the RFC for the added sections. We had some talks before about the fact that this is not equally relevant for all SDKs (which is why I initially split this into two RFCs), but @mitsuhiko reiterated that we should do this in an aligned way. |
text/0122-sdk-hub-scope-merge.md
Outdated
| ## Global Scope | ||
|
|
||
| In addition to the currently active scope, there will also be a new special scope, the **Global Scope**. | ||
| The global scope is _not_ the initial scope, but a special scope that belongs to a client and is applied to any event that belongs to this client. |
There was a problem hiding this comment.
This would make the client stateful all of a sudden. Not the end of the world, but feels odd.
There was a problem hiding this comment.
Not necessarily. This does not mean that the global scope is defined on the client, but that the global scope has a reference to the client :) the mechanism to look up the global scope can/should probably be more in parallel to getting the active scope. Think of it like this:
const executionContext1; // this is one execution context somewhere
const executionContext2; // this is some other execution context somewhere else
executionContext1.globalScope --> Scope
executionContext2.activeScope --> Scope
executionContext3.isolationScope --> ScopeSo all three of these scopes can be put on the execution context (or wherever the active scope is put). How exactly this is done would be up to the SDK/language, but the idea is that the client remains stateless.
There was a problem hiding this comment.
I was also initially confused by this, but I think it's just a matter of wording this a bit differently. It says:
but a special scope that belongs to a client and is applied to any event that belongs to this client
This makes it sound like the client owns the scope, even though technically it's the other way around. Maybe we can write something like
| The global scope is _not_ the initial scope, but a special scope that belongs to a client and is applied to any event that belongs to this client. | |
| The global scope is _not_ the initial scope, but a special scope that has a client and is applied to any event that belongs to its client. |
There was a problem hiding this comment.
The tricky thing to convey (and actually, we need to specifically spec this out), is that each client has a separete global scope.
At least, I think this is what makes most sense 🤔 When talking with @mitsuhiko both options are possible (global scope is truly global, or global scope is basically a "client scope").
Personally I tend to the idea that each client should have it's own global scope - simply because thinking about e.g. MFE and similar, we probably don't want to "pollute" other clients when doing getGlobalScope().setTag().
Wording wise, we could say:
The global scope is not the initial scope, but a separate, special scope. The global scope is different for each client, and is applied to every event.
Or something like this?
text/0122-sdk-hub-scope-merge.md
Outdated
| Furthermore, there can also be **Isolation Scopes**. | ||
| Similar to the global scope, these are also applied to events. However, isolation scopes can be created, either by us internally (the most common scenario), or also by users. The new APIs for this are: |
There was a problem hiding this comment.
The client already holds the global scope, and now it also holds an isolation scope. But we do not expect to create a new client for each request, so the client should hold a stack of isolation scopes IMO, which will be tricky. Can we somehow clarify how this all should work?
There was a problem hiding this comment.
Added a comment above hopefully explaining the idea here - was probably not fully clear from how I wrote this down in the RFC. TLDR: active scope, global scope, isolation scope should be stored "next to each other" on each execution context. nothing should be stored directly on the client, which should remain stateless.
text/0122-sdk-hub-scope-merge.md
Outdated
|
|
||
| # Unresolved questions | ||
|
|
||
| TODO |
There was a problem hiding this comment.
| TODO | |
| - What is the user experience like if there is no instrumentation to set isolation scopes? |
|
I agree with @antonpirker's assessment that these changes seem reasonable at first glance for the Python SDK. My one suggestion for the RFC would be to consider adding a visualization, similar to the one on the unified API docs page, to describe how these changes will affect the API at a high level. |
Co-authored-by: Ivana Kellyerova <[email protected]>
| An isolation scope is similar to a global scope, but tied to a specific section of your app/code that should be isolated. | ||
| In a server side language, an isolation scope will commonly be created per request. Each request gets an isolated scope that you can assign data to, which should be applied to all events that are processed in this request. However, an isolation scope is not limited to requests - you could also have an isolation scope for a scheduled job, or for a zone in your UI (in theory). | ||
|
|
||
| The isolation scope can be used to e.g. add user or request data that should be applied to any event for the current request (or similar isolation unit). Similar to the global scope, it does not matter where you call it, but you can always be sure that data is correctly attached/available. This can especially be useful when writing hooks or plugins where you may not easily get access to the scope of a request: |
There was a problem hiding this comment.
Let's explicitly call out that breadcrumbs should be added on the isolation scope.
There was a problem hiding this comment.
I mention this further down here: https://github.com/getsentry/rfcs/blob/fn/merge-hub-scope/text/0122-sdk-hub-scope-merge.md#what-should-be-called-from-top-level-methods
but it's a general challenge for this RFC that stuff feels like it should be explained earlier that comes up later, making it hard to keep it somewhat readable 😬
There was a problem hiding this comment.
My comment here was I think that the breadcrumbs use case helps build the best intuition here why we need a isolation scope concept, it's also why we need something like this while opentelemetry does not fwiw.
There was a problem hiding this comment.
Thanks for the detailed RFC @mydea 👏
I like the idea of merging hub & scope, but I don't think an isolation scope is needed for Mobile. It will only confuse users. Furthermore, I'm confused about where to store the current scope on Mobile and Browser JS. If we keep it globally, it's basically the same as the global scope.
text/0122-sdk-hub-scope-merge.md
Outdated
| } | ||
| ``` | ||
|
|
||
| The global scope is _not_ the initial scope, but a separate, special scope. The global scope is different for each client, in order to avoid polluting this on accident when working with multiple completely separate clients. |
There was a problem hiding this comment.
I'm a bit confused as to why the global scope is different for each client. You set the client to the scope, but you also need to keep a global scope for every client. If I got this right, do we need some bookkeeping of global scopes for clients?
There was a problem hiding this comment.
This is something I am not 100% sure about yet. On the one hand, it kind of makes sense that a global scope is truly global. Thinking about this some more, probably you are right, and this should be truly global. If you have multiple clients, it's probably up to you to simply not use the global scope if you care about this not being polluted!
There was a problem hiding this comment.
If you have multiple clients, it's probably up to you to simply not use the global scope if you care about this not being polluted!
Yes. Furthermore, we don't need bookkeeping of global hubs for clients. I think that a truly global scope would simplify things.
text/0122-sdk-hub-scope-merge.md
Outdated
| } | ||
| ``` | ||
|
|
||
| The global scope is _not_ the initial scope, but a separate, special scope. The global scope is different for each client, in order to avoid polluting this on accident when working with multiple completely separate clients. |
There was a problem hiding this comment.
I am confused as to why the global scope is different for each client. If I got this right, do we need some bookkeeping of global scopes for clients? How is that going to work?
I would expect that the global scope is global 😄. It applies to all events.
| }); | ||
| ``` | ||
|
|
||
| Note that in environments where you do not have multiple execution contexts (e.g. Browser JS), there is basically only a single isolation scope that is fundamentally the same as the global scope. In these environments, global & isolation scope can be used completely interchangeably. However, for consistency we still have these concepts everywhere - but users do not need to care about them, and we can reflect this in the SDK specific documentation. |
There was a problem hiding this comment.
I don't think adding the isolation scope is a good idea when it's the same as the global scope. People will ask questions and need clarification about what the difference is. If it's the same, I would drop the isolation scope. We can always add it.
There was a problem hiding this comment.
it's not the same - the global scope will always be the same, while the isolation scope can be changed per context. In environments where there are no execution contexts (e.g. browser or probably mobile), they are more or less the same. But the idea is generally to not really expose this to users too much - users should just use Sentry.doX and not Sentry.getCurrentScope().doX, and we route it to the correct scope under the hood. that should work just as well in browser or mobile.
So the concept should still be there for consistency sake, and it does work (e.g. you can call it), it's just not commonly useful by default - which is fine, nobody is forced to use it, and users should not have to think about it! but for example looking at browser, if somebody decides to add and setup zone.js or something like this, or browsers eventually add something like execution contexts, we can then also wire this up to work there as well.
There was a problem hiding this comment.
I'm OK with your proposal.
|
|
||
| Isolation scopes will be documented in a new section under "Enriching Events > Per request data" (or similar, based on SDK language & needs), which will explain how to use `getIsolationScope()` and when/how data added there will be applied. | ||
|
|
||
| Global methods to set data, like `Sentry.setTag()` or `Sentry.setUser()`, will set on the isolation scope. Only if you want to set on a different scope, you need to manually get it (e.g. `getGlobalScope().setTag()`). |
There was a problem hiding this comment.
On mobile, global methods should change the global scope, as we don't need an isolation scope, IMO.
There was a problem hiding this comment.
it can/should still set on the isolation scope, which in absence of execution contexts will be functionally basically global. But if execution contexts are ever added in some way, this will then also "just work" - that's the idea here, at least.
| Scopes are applied in this order to events: | ||
|
|
||
| ```ts | ||
| class Scope { |
There was a problem hiding this comment.
So, clients will not be aware that there is a scope. The Scope class merges data into the event before passing it to the client. Did I get this correctly?
There was a problem hiding this comment.
that's the idea, this is also how it works today (at least in JS and I think more or less everywhere?) - this RFC does not propose to change this from it is today, except that the special scope data is also applied to the events!
text/0122-sdk-hub-scope-merge.md
Outdated
|
|
||
| - How should we handle this in docs? | ||
| - Naming: Can we come up with something better than Isolation Scope? | ||
| - Global Scope: Should it be per-client (current RFC) or truly global? |
There was a problem hiding this comment.
IMO, if you call it global, then it should be truly global. If you want it to be per-client, I would choose a different name.
|
|
||
| The following chart gives an overview of how these scopes work and to which events they would be applied: | ||
|
|
||
|  |
There was a problem hiding this comment.
Is the green scope in that diagram the current scope? If yes, you could update the diagram by renaming scope to current scope to make it more straightforward.
|
|
||
| ### How to store/reference scopes | ||
|
|
||
| Similar to how we currently store the current hub, we also need to store the current scope. The exact mechanism to do that is up to the concrete SDK/language. In environments with multiple execution contexts (or thread locals), we should attach the current scope to the execution context (or thread local). In other environments, it may be attached to some global (e.g. in Browser JS). This RFC does not propose to change this, so generally the current scope may be stored the same was as the current hub used to be. |
There was a problem hiding this comment.
If we store the current context globally on mobile and Brower JS, where there is no execution context, how is the current scope different from the global scope?
There was a problem hiding this comment.
In e.g. mobile or browser, we'll need to implement the current scope somehow differently. Either we still need to do it via some kind of stack, or with some other mechanism - the idea is to get as close to this desired UX as possible, with the tech that is available!
| async function setupGlobalData() { | ||
| const revision = await getRevision(); | ||
| // No need to worry or care about where this is called, this will _always_ work! | ||
| Sentry.getGlobalScope().setTag("revision", "abc"); |
There was a problem hiding this comment.
On Mobile, Sentry.setTag("revision", "abc") already does what Sentry.getGlobalScope().setTag("revision", "abc"); does, as the static API of Sentry. has a hub bound including a scope.
|
|
||
| - This changes _a lot_ of public APIs and behavior. | ||
|
|
||
| # Unresolved questions |
There was a problem hiding this comment.
We still need to figure out how scope sync across multiple layers works. For example, on RN we sync the scope to Cocoa, which syncs the scope to SentryCrash. I guess the SDKs should only sync the global scopes. We don't sync the scope from a lower layer, for example, Cocoa to RN.
|
|
||
| ## Applying scopes | ||
|
|
||
| Scopes are applied in this order to events: |
There was a problem hiding this comment.
Given the example on how scopes are merged I'm wondering what happens the other way round when scopes are read? E.g. Would Sentry.getCurrentScope().getRelease() return a value when the release was set on the global scope ~ Sentry.getGlobalScope().setRelease("1.0.0")
There was a problem hiding this comment.
It's a good question! I would say it should only return the value on the actual scope being accessed. If you want to have some cascading behavior it should be a top-level method like Sentry.getRelease() or something like this?
| # Unresolved questions | ||
|
|
||
| - How should we handle this in docs? | ||
| - Naming: Can we come up with something better than Isolation Scope? |
There was a problem hiding this comment.
Just throwing in some ideas: How about having a Global Scope / Root Scope / Current Scope? Or if we want to be similar to otel, naming it Context instead of scope; although it's not exactly the same as context is just a part of the scope as I understood it. Then we'd have Global Execution Context / Root Execution Context / Current Execution Context.
Not super happy with my ideas either, as root could imply some tree structure 🤷
There was a problem hiding this comment.
We also had root as a name already in earlier iterations here 😅 which was also found to not be super understandable by some folks. It's a hard naming problem 😬
Cleaning up existing code to make the refactoring of Hubs and Scopes easier. (See getsentry/rfcs#122) If this dogfooding will not raise any issues, we will merge and release it in the Sentry SDK.
Cleaning up existing code to make the refactoring of Hubs and Scopes easier. (See getsentry/rfcs#122) If this dogfooding will not raise any issues, we will merge and release it in the Sentry SDK.
|
|
||
| You can update the client of a scope via `scope.setClient(newClient)`. This will not affect any scope that has already been forked off this scope, but any scope forked off _after_ the client was updated will also receive the updated client. | ||
|
|
||
| A current scope always inherits from the previous current scope. |
There was a problem hiding this comment.
I'm assuming this only applies when withScope is used exclusively and setCurrentScope is never called, correct? Otherwise what stops you from simply creating a new scope and setting it as current?
| export function getIsolationScope(): Scope; | ||
|
|
||
| // Create a new isolation scope for this scope | ||
| // This will NOT make this scope the isolation scope, but will create a new isolation scope (based on the active isolation scope, if one exists) |
There was a problem hiding this comment.
This confuses me. So what happens if I call getActiveScope().isolate()?
Would it do some sort of multi clone where it clones the active scope and the isolation scope of the current execution context at the same time?
Or is this a method that should only be used on an isolation scope?
Also linked to this confusion is the following sentence:
The isolation scope always inherits from the previously active isolation scope.
If .isolate() has nothing to do with the scope it's called on I'd argue the method should go somewhere else or be skipped completely.
How long does the isolation scope set via scope.isolate() stay active?
| ### When to create an isolation scope | ||
|
|
||
| For most server-side SDKs, an isolation scope will be created for each request being processed. | ||
| Roughly, it will equate to each time we currently fork a hub. |
There was a problem hiding this comment.
I'm assuming the answer here is that adding tags, breadcrumbs, setting user etc. might get lost due to the user setting them on a scope that'll be lost and thus won't apply to sibling scopes.
// ... request incoming
const requestScope = getCurrentScope().fork() // whatever the correct syntax here would be
// some part of handling the request
val somePartScope = requestScope.fork()
setCurrentScope(somePartScope);
// somewhere her the user sets tags, breadcrumbs and user which is applied to events happening within the same scope or child scopes
// a different part of handling the request
val otherPartScope = requestScope.fork()
setCurrentScope(otherPartScope);
// tags, breadcrumbs and user are not applied for things happening here
The same example using isolation scope would allow the user to set tags, breadcrumbs, user etc. for the whole request without having to be in the right place to do so (i.e. gain access to the requestScope instead of a child scope). In other words isolation scope just seems to be an explicit concept for adding things to a scope specific for a request or whatever other things we wanna use it for (e.g. cron execution).
This behaviour would happen e.g. when we switch threads where today we clone the hub onto the other thread but when execution comes back the hub from that thread doesn't affect the rest of the request handling part. In the future instead of cloning the hub we'd be forking execution context + scope I presume.
|
|
||
| This RFC aims to align this so we can handle the OTEL way of context forking (=doing this very frequent and liberally). | ||
|
|
||
| Also, the RFC acknoledges that not all of the concepts laid out below are equally important for every environment - e.g. for Browser JS or mobile, we may not have multiple execution contexts, so some of the concept may not be so relevant. However, we still want to align the concepts across all SDKs for the following reasons: |
There was a problem hiding this comment.
| Also, the RFC acknoledges that not all of the concepts laid out below are equally important for every environment - e.g. for Browser JS or mobile, we may not have multiple execution contexts, so some of the concept may not be so relevant. However, we still want to align the concepts across all SDKs for the following reasons: | |
| Also, the RFC acknowledges that not all of the concepts laid out below are equally important for every environment - e.g. for Browser JS or mobile, we may not have multiple execution contexts, so some of the concept may not be so relevant. However, we still want to align the concepts across all SDKs for the following reasons: |
| export function getGlobalScope(): Scope; | ||
|
|
||
| // get the curren isolation scope | ||
| export function getIsolationScope(): Scope; |
There was a problem hiding this comment.
Some SDKs may also need a setter for isolation scope. In Java some frameworks do not play nicely with the callback based approach and instead we're just being told that something's about to happend and something just happened (e.g. changing threads).
| // This method is not defined by us, but is some external code | ||
| // Here just for demonstration purposes of how that may be implemented | ||
| function otelWrapHttpServerRequest(original: Function): Function { | ||
| // Fork an execution context for this server request, |
There was a problem hiding this comment.
I also don't really understand this example here. So is otelWrapHttpServerRequest some generic method, that's not defined by us? Why is that able to run withScope then if it has nothing to do with Sentry? Is this a generic OTEL hook that always triggers when a new span is created and automatically forks the current scope via withScope?
I'm unable to find scope.isolate() in JS implementation (https://github.com/search?q=repo%3Agetsentry%2Fsentry-javascript+%22isolate%22&type=code). I have another comment further up about it as well where I'm unsure what it should do.
It looks like this has been implemented using getIsolationScope.clone() instead of scope.isolate() here https://github.com/getsentry/sentry-javascript/blob/b0752b5d3e0840f3359688c58619fb0f638470bc/packages/opentelemetry/src/contextManager.ts#L53
Should we remove it if it's not implemented and unclear?
| // ... | ||
| ``` | ||
|
|
||
| SDKs _may_ also add an option to the client to opt-in to put breadcrumbs on the global scope instead (e.g. for mobile or scenarios where you always want breadcrumbs to be global). |
There was a problem hiding this comment.
Wouldn't isolation scope and global scope be the same usually on mobile anyways?
| While they have generally served us well, the shortcomings of the current systems have become apparent over time. | ||
| Apart from generally being unclear to users - what is applied when, why, how? - there are also problems with the way we currently instruct users to add event data. | ||
|
|
||
| A major reason for this change is also to better align with OpenTelemetry. In OpenTelemetry (OTEL), the equivalent to our scope is the "Context". However, the Context is immutable in OTEL, and works via copy-on-write - so each time you want to change _anything_ on the context, you fork it. This means that you have much more forking than you have in Sentry currently, and also since you always fork a context when you change the active span (as the span is set on the context, and updating it means forking a context), you can quickly get into deeply nested context situations due to auto instrumentation. |
There was a problem hiding this comment.
This RFC doesn't seem to mention where to store the current span. While we might no longer have a Sentry span after completely switching over to OTEL, there'll likely be intermediate steps as well as some SDKs that can't yet move to pure OTEL spans.
Where we could put it:
- Isolation span
- I don't think that's a good idea as
- It's a potential footgun allowing users to hide the current span set on isolation scope by the SDK by setting a span on the current span
- When a span ends we have to restore the previous span on the isolation scope
- I don't think that's a good idea as
- Current Span
- Since we're forking current scope anytime a new span is created anyways this is a good candidate
- A drawback here is that it requires a more complex copy-on-write implementation that's per property instead of simply copying the whole scope. It seems the number one reason for copying scope will be creating a new span.
- Execution Context
- This would keep current span separate from current scope and thus not cause as many current scope copies
There was a problem hiding this comment.
Another thing that probably should be mentioned is how to treat PropagationContext, which is used for tracing without performance (TwP).
I think it makes sense to put it on the isolation scope when we e.g. handle a new incoming request. Should Sentry.continueTrace() also write to isolation scope? This would probably only work if it's always combined with forking an isolation scope so maybe there should be a more abstract API to isolate which then forks isolation scope, current scope and sets a new propagation context on the isolation scope.
Things to consider here:
- freezing baggage should probably apply to the whole isolation scope as well
- if we put propagation context on the current scope we probably can't clone it but rather have to share the instance down into forked scopes
- this might be closely related to the question of whether to always for isolation scope and current scope together
|
|
||
| ## What about other Hub references? | ||
|
|
||
| While the Hub is mainly exposed via `getCurrentHub()`, it is also used as argument or similar in many places. |
There was a problem hiding this comment.
Wouldn't this need to access all three scopes (global, isolation and current) instead of the single hub?
|
|
||
| In environments where you don't have execution contexts, you may store them in a global place. The exact mechanism to do this is up to the concrete SDK/language. | ||
|
|
||
| For each execution context, there will _always_ be exactly one current & isolation scope. When the execution context is forked, these are forked along. |
There was a problem hiding this comment.
I feel like this can easily be misinterpreted as if you fork execution context (e.g. because a new span was started) you always have to fork isolation scope and current scope.
Maybe we should add a chapter about forking scopes as well as passing scopes along when e.g. changing to another thread. I feel like whenever we fork an isolation scope we should also fork the current scope as otherwise we'd open the door for scope bleed and always require users to call both withIsolationScope and withScope in combination. When moving to another thread we should bring along both isolation and current scope to avoid scope bleed from things that happened on that thread before.
There was a problem hiding this comment.
Another thing here is how do we figure out, when to create a new isolation scope for OTEL auto instrumentation spans?
Do we check if a span has no parent in the current process and then fork isolation scope for it? Does this happen in SpanProcessor?
Does this also work the same way, e.g. for queue workers?
There was a problem hiding this comment.
Today @antonpirker and me discovered some more questions / remarks.
|
|
||
| ## What should be called from top level methods? | ||
|
|
||
| Top level APIs that set something should generally interact with the isolation scope: |
There was a problem hiding this comment.
h I'm concerned about upgrade experience for users here.
Previously, Sentry.setTag() and Sentry.configureScope(s -> s.setTag) produced the same result.
In the future this is no longer true, since Sentry.setTag() will suddenly write to isolation scope and thus leak out of the scope it was previously bound to. While I agree this is a desired behaviour for the future, users upgrading may not notice this and suddenly have completely different behaviour.
IMO this change should be part of a major and we should try to make it very prominent in changelog, migration guide etc.
Here's an example. Please ignore the fact that pushScope and popScope will be replaced by withScope, the result should still be the same.
Here's an example, where tag A=1 suddenly leaks out to the whole isolation scope.
Previously:
global isolation current
Sentry.setTag(Z, 99)
Sentry.pushScope
Sentry.setTag(A, 1)
A = 1
Sentry.configureScope(s -> s.setTag(B, 2))
B = 2
HTTP GET -> google, A=1, B=2, Z=99
Sentry.popScope
HTTP GET -> reddit, Z=99
^---------------- old
Future:
global isolation current
Sentry.setTag(Z, 99)
Z = 99
Sentry.pushScope
Sentry.setTag(A, 1)
A = 1
Sentry.configureScope(s -> s.setTag(B, 2))
B = 2
HTTP GET -> google, A=1, B=2, Z=99
Sentry.popScope
HTTP GET -> reddit, A=1, Z=99
^---------------- new
| While they have generally served us well, the shortcomings of the current systems have become apparent over time. | ||
| Apart from generally being unclear to users - what is applied when, why, how? - there are also problems with the way we currently instruct users to add event data. | ||
|
|
||
| A major reason for this change is also to better align with OpenTelemetry. In OpenTelemetry (OTEL), the equivalent to our scope is the "Context". However, the Context is immutable in OTEL, and works via copy-on-write - so each time you want to change _anything_ on the context, you fork it. This means that you have much more forking than you have in Sentry currently, and also since you always fork a context when you change the active span (as the span is set on the context, and updating it means forking a context), you can quickly get into deeply nested context situations due to auto instrumentation. |
There was a problem hiding this comment.
Another thing that probably should be mentioned is how to treat PropagationContext, which is used for tracing without performance (TwP).
I think it makes sense to put it on the isolation scope when we e.g. handle a new incoming request. Should Sentry.continueTrace() also write to isolation scope? This would probably only work if it's always combined with forking an isolation scope so maybe there should be a more abstract API to isolate which then forks isolation scope, current scope and sets a new propagation context on the isolation scope.
Things to consider here:
- freezing baggage should probably apply to the whole isolation scope as well
- if we put propagation context on the current scope we probably can't clone it but rather have to share the instance down into forked scopes
- this might be closely related to the question of whether to always for isolation scope and current scope together
|
|
||
| In environments where you don't have execution contexts, you may store them in a global place. The exact mechanism to do this is up to the concrete SDK/language. | ||
|
|
||
| For each execution context, there will _always_ be exactly one current & isolation scope. When the execution context is forked, these are forked along. |
There was a problem hiding this comment.
Another thing here is how do we figure out, when to create a new isolation scope for OTEL auto instrumentation spans?
Do we check if a span has no parent in the current process and then fork isolation scope for it? Does this happen in SpanProcessor?
Does this also work the same way, e.g. for queue workers?
This refactors the SDK to move away from the Hub and have all the functionality in the Scope. Introducing different types of scopes. This aligns the SDK with how Opentelementry (OTel) handles data bringing us closer to be 100% OTel compatible. This change was discussed in this RFC: getsentry/rfcs#122 There is also a small FAQ: https://gist.github.com/mitsuhiko/1bc78d04ea7d08e5b50d27e42676db80 And a Miro board showing how the new scopes manage data: https://miro.com/app/board/uXjVNtPiOfI=/?share_link_id=216270218892 ### This RP contains - Introduction of global, isolation, and current scope - Deprecation of the Hub - All existing Hub based API still works and is still used by most of our integrations. Under the hood the new Scopes are used. - (this PR now includes all the changes made in the [first PR](#2609) introducing the new API) ### Breaking changes - The Pyramid integration will not capture errors that might happen in `authenticated_userid()` in a custom `AuthenticationPolicy` class. - The parameter `propagate_hub` in `ThreadingIntegration()` was deprecated and renamed to `propagate_scope`.
| ## Clients | ||
|
|
||
| Instad of picking the client from the current hub, going forward a client should always be fetched via `Sentry.getClient()`. | ||
| This should _always_ return a client. If `Sentry.init()` was not called yet, it should return a Noop Client that does nothing. |
There was a problem hiding this comment.
Instad of picking the client from the current hub, going forward a client should always be fetched via
Sentry.getClient().
This feels odd. If SDK users are only using one client (Sentry.getClient() implies a static/singleton reference to a Sentry Client) then this could work. It might be easier to make the Scope Stack a property of the client though and use typical DI techniques to get a reference to the Client... avoiding all the problems with statics/singletons.
If SDK users have multiple clients, getting the client via Sentry.getClient() is problematic. It results in weird things like temporarily pushing a different client to the scope stack (so Sentry.getClient() can return the top/current client from the stack)... in the case of Global/Shared scope, this could easily lead to problems.
For SDK users and SDK designers, it would be easier if the ScopeStack was a property of the client. If SDK users wanted two different clients to share the same Scope, those clients could be configured/initialised with some kind of shared scope stack (like the GlobalScope from this RFC)... but events would always end up being sent to the correct Sentry instance/project.
I'm not sure I understand why/when anyone would want the client to be ambient context (in the same way that scopes are) or why anyone would maintain a stack of clients (except as a workaround to our SDK design - namely that we retrieve the client reference via a static/singleton reference).
There was a problem hiding this comment.
This should just work like other things on static API that interact with the scope by creating a sort of view on top of global, isolation and current scope, then handing back whatever wins according to logic that should be detailed somewhere in this RFC. There shouldn't be a need to change the stack and stack should in theory go away with the proposed changes here.
|
|
||
| An isolation scope is similar to a global scope, but tied to a specific section of your app/code that should be isolated. | ||
| In a server side language, an isolation scope will commonly be created per request. Each request gets an isolated scope that you can assign data to, which should be applied to all events that are processed in this request. However, an isolation scope is not limited to requests - you could also have an isolation scope for a scheduled job, or for a zone in your UI (in theory). | ||
|
|
There was a problem hiding this comment.
Maybe not necessary to implement IsolatedScopes in all SDKs.
The dotnet SDK has an asynclocal scope manager that achieves this already. I'm not sure how unique that is to .NET though and whether equivalents exist in other languages/runtimes.
There was a problem hiding this comment.
Does that mean in dotnet you can have both an isolation scope (via asynclocal scope manager) and a separate current scope?
There was a problem hiding this comment.
Possibly but it's not really a problem we'd need to solve in .NET. In ASP.NET Core requests get handled by middleware which is async/await all the way through and so we use an asynclocal scope container (that has affinity for these requests).
rfcs/text/0122-sdk-hub-scope-merge.md
Line 304 in c4b1fb4
... this isn't really necessary in .NET as we already get that for free from asynclocal.
There was a problem hiding this comment.
I was trying to say that parts of static API interact with isolation scope and other parts interact with current scope. I'm guessing you have to propagate both using this asynclocal scope container. Not sure about .NET but there'll probably be some Sentry.withScope() and/or Sentry.pushScope/popScope API that forks current scope.
There was a problem hiding this comment.
Not sure about .NET but there'll probably be some
Sentry.withScope()and/orSentry.pushScope/popScopeAPI that forks current scope
There is yes. There's the SentrySdk.ConfigureScope which clones the current scope, puts the clone on the stack, then pops it off again once it's done (ie. when it's disposed).
I guess we could replace that with SentrySdk.GetIsolationScope or similar but I don't understand why we would. It doesn't seem to solve a problem that any of our users have and it doesn't get us any closer to using OTEL for spans (at least not for the .NET SDK - I realise it may be a prerequisite for the JavaScript SDKs).
| - RFC PR: https://github.com/getsentry/rfcs/pull/122 | ||
| - RFC Status: draft | ||
|
|
||
| # Summary |
There was a problem hiding this comment.
Since this RFC is no longer being updated, we're writing down how this works and collecting learnings etc. here: https://develop.sentry.dev/sdk/hub_and_scope_refactoring/
This RFC is about merging the concepts of hubs & scopes in the SDK into a singular Scope concept.
Rendered RFC
Note that this RFC is specifically designed to be relevant to all SDKs. For more specific requirements/changes for e.g. the JavaScript SDK, there is a separate RFC here: #120. This way we can keep the discussion more focused on generally applicable things vs. more language/SDK specific things.