Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge Hub & Scope in SDKs #122

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
161 changes: 161 additions & 0 deletions text/0122-sdk-hub-scope-merge.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
- Start Date: 2023-11-07
- RFC Type: feature
- RFC PR: https://github.com/getsentry/rfcs/pull/122
- RFC Status: draft

# Summary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/


We want to streamline Hub & Scope usage for the SDK.

# Motivation

Generally, Hubs and Scopes are a very Sentry-specific concept, and can be a bit hard to explain.
Also, how Hub & Scope forking behaves is currently a bit tricky, and does not play well e.g. with how OpenTelemetry contexts work.

This RFC aims to streamline this by merging the concepts of Hub and Scope into a singular Scope concept.

# Background

TODO

# Proposed Solution

Hubs will be removed as a concept for SDK users.
Instead, from a users perspective only Scopes remain, which will become the singular entity to hold context data etc.

Scopes will be _similar_ to how they work today, but not entirely the same.
Scopes can have data (e.g. tags, user, ...) added to them the same way you can do today.
This RFC _does not_ aim to change any of the data that is kept on the scope and is applied to events.

The following APIs will be removed:

* `getCurrentHub()`
* `configureScope()` (instead just get the scope and set on it directly)
* Any APIs currently on the Hub only:
* `hub.pushScope()`
* `hub.popScope()`
* `hub.isOlderThan()`
* `hub.bindClient()`
* `hub.getStack()`
* `hub.getStackTop()`
* `hub.run()` (use `withScope()` instead)
mydea marked this conversation as resolved.
Show resolved Hide resolved

Instead, we will introduce some new APIs:

```ts
// get the currently active scope. replacement for `getCurrentHub().getScope()`
export function getScope(): Scope;

// get the currently active client. May return a NOOP client. Replacement for `getCurrentHub().getClient()`.
export function getClient(): Client;

// make a scope the current scope. Replacement for `makeMain(hub)`
export function setCurrentScope(scope: Scope): void;
```

The following APIs already exist but will behave differently:

* `withScope()` will still work, but it will actually fork an execution context. So this will roughly do the same as doing `hub.run()` today in languages that have that, which forks an execution context.

APIs that are currently on the hub should instead be called directly on the scope (e.g. `scope.captureException()` etc.), or via a global method (e.g. `Sentry.captureException()`).

The current scope may be kept similar to how we currently keep the current hub, but this is SDK specific and not part of this RFC.

## Clients

Instead of a client being optional, there will now _always_ be a client. It may be a Noop Client that does nothing, if `init()` has not been called yet.

A scope has a reference to a client. By default it will reference a noop client. You can bind a client to a scope via `scope.setClient()`.
The client is inherited by forked scopes.
mydea marked this conversation as resolved.
Show resolved Hide resolved
mydea marked this conversation as resolved.
Show resolved Hide resolved

```js
const client1 = new Client();
const scope = new Scope();

scope.getClient(); // <-- returns a noop client by default

scope.setClient(client1);
mydea marked this conversation as resolved.
Show resolved Hide resolved
scope.getClient(); // <-- returns client1
```

The current scope may be kept similar to how we currently keep the current hub, but this is SDK specific and not part of this RFC.
mydea marked this conversation as resolved.
Show resolved Hide resolved

When calling `getScope()` before a scope was made the current one (=before init was called), we will return a scope for a noop client.
mydea marked this conversation as resolved.
Show resolved Hide resolved
A noop client is a regular client that simply does not send anything.

This way, the API for `getClient()` can always return a client, and users do not have to guard against this being undefined all the time.
We may also expose a util like `sentryIsInitialized()` that checks if the current client is a Noop client (which currently you could have checked as `getCurrentHub().getClient() === undefined`).

If you want to have multiple isolated clients, you can achieve this easily with this new setup:

```js
const client1 = new Client();
mydea marked this conversation as resolved.
Show resolved Hide resolved
const client2 = new Client();

const scope1 = new Scope();
const scope2 = new Scope();

scope1.setClient(client1);
scope2.setClient(client2);

scope1.captureException(); // <-- isolated from scope2
```

## Scopes

Scopes behave similar to how they behave today.
When a scope is forked via `withScope()`, a new scope is created that inherits all data currently set on the parent scope.

The main change to Scopes is that they do not push/pop anymore, but instead fork an execution context (in languages where this makes sense/is possible).
Basically, `withScope()` should behave like `hub.run()` does today in languages that have execution context forking.

`client.getScope()` should return the current scope of this client in the current execution context.
mydea marked this conversation as resolved.
Show resolved Hide resolved

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong feelings about withScope, interested to hear what others think on this! I'd be fine with either.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to withScope(), 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


You can make a scope the current one via `setCurrentScope(scope)`, which should bind the scope to the current execution context (or a global, in SDKs without execution context). This is a replacement for the current APIs like `makeMain(hub)` or `setCurrentHub(hub)`.

You can still clone scopes manually the same way as before, e.g. via `Scope.clone(oldScope)` or a similar API. In contrast to `withScope()`, this will _not_ fork an execution context.

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.

## Should users care about Clients?
mydea marked this conversation as resolved.
Show resolved Hide resolved

Generally speaking, for most regular use cases the client should be mostly hidden away from users.
Users should just call `Sentry.init()`, which will setup a client under the hood. Users should generally only interact with scopes, and we should keep clients out of most public facing APIs.

The client is only there to allow an escape hatch when users need to do more complex/special things, like isolating Sentry instances or multiplexing. So client APIs should be designed to _allow_ to do things that cannot be done via `Sentry.init()`, but our main focus should be on making the default experience easy to understand, which includes that users should not have to care about the concept of clients by default.

## What about other Hub references?

While the Hub is mainly exposed via `getCurrentHub()`, it is also used as argument or similar in many places.
These occurences should be updated to instead take a scope or a client.

## What about backwards compatibility?

We should strive to provide a wrapper/proxy `getCurrentHub()` method that still exposes the key functionality to ease upgrading. E.g.:

```js
import {getScope, getClient} from '../internals';

function getCurrentHub() {
return {
getClient,
getScope
}
}
```

We need to decide what to keep in that proxy and what not.

## What about globals?

This RFC does not propose any concrete way to store the current scope. This is up to the concrete SDK and may behave the same way as it currently does for the hub, or differently if that makes more sense in a given scenario.

# Drawbacks

* This changes _a lot_ of public APIs and behavior.

# Unresolved questions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TODO
- What is the user experience like if there is no instrumentation to set isolation scopes?