Local scopes

[jpnurmi]

We would like to add support for local scopes for the needs of downstream SDKs (sentry-unreal).

API proposal for capturing events with local scopes, inspired by other Sentry SDKs:

sentry_uuid_t sentry_capture_event_with_scope(sentry_value_t event, sentry_scope_callback_t callback, void *userdata)

• Clones the current scope and pushes it on a (thread-local?) stack
• Calls the scope configuration callback
  typedef void (*sentry_scope_callback_t)(sentry_scope_t *scope, void *userdata)
• Flushes the local scope
• Captures the event
• Discards the local scope

Ideally, GenericPlatformSentryScope in sentry-unreal would be a thin wrapper around an opaque sentry_scope_t*. To achieve this, sentry-native could expose getters and setters for all the data stored in the scope:

typedef struct sentry_scope_s {
    char *transaction;
    sentry_value_t fingerprint;
    sentry_value_t user;
    sentry_value_t tags;
    sentry_value_t extra;
    sentry_value_t contexts;
    sentry_value_t breadcrumbs;
    sentry_level_t level;
    sentry_value_t client_sdk;
    sentry_transaction_t *transaction_object;
    sentry_span_t *span;
} sentry_scope_t;

• transaction

  const char *sentry_scope_get_transaction(const sentry_scope_t *scope);
  void sentry_scope_set_transaction(sentry_scope_t *scope, const char *transaction); // + _n

• fingerprint

  sentry_value_t sentry_scope_get_fingerprint(const sentry_scope_t *scope);
  void sentry_scope_set_fingerprint(sentry_scope_t *scope, const char *fingerprint, ...); // + _n
  void sentry_scope_remove_fingerprint(sentry_scope_t *scope);

• user

  sentry_value_t sentry_scope_get_user(const sentry_scope_t *scope);
  void sentry_scope_set_user(sentry_scope_t *scope, sentry_value_t user);
  void sentry_scope_remove_user(sentry_scope_t *scope);

• tags

  const char *sentry_scope_get_tag(const sentry_scope_t *scope, const char *key);
  void sentry_scope_set_tag(sentry_scope_t *scope, const char *key, const char *value); // + _n
  void sentry_scope_remove_tag(sentry_scope_t *scope, const char *key); // + _n

  sentry_value_t sentry_scope_get_tags(const sentry_scope_t *scope)
  void sentry_scope_set_tags(sentry_scope_t *scope, sentry_value_t tags)

• extras

  sentry_value_t sentry_scope_get_extra(const sentry_scope_t *scope, const char *key);
  void sentry_scope_set_extra(sentry_scope_t *scope, const char *key, sentry_value_t value); // + _n
  void sentry_scope_remove_extra(sentry_scope_t *scope, const char *key); // + _n

  sentry_value_t sentry_scope_get_extras(const sentry_scope_t *scope);
  void sentry_scope_set_extras(sentry_scope_t *scope, sentry_value_t extras);

• contexts

  sentry_value_t sentry_scope_get_context(const sentry_scope_t *scope, const char *key);
  void sentry_scope_set_context(sentry_scope_t *scope, const char *key, sentry_value_t value); // + _n
  void sentry_scope_remove_context(sentry_scope_t *scope, const char *key); // + _n

  sentry_value_t sentry_scope_get_contexts(const sentry_scope_t *scope);
  void sentry_scope_set_contexts(sentry_scope_t *scope, sentry_value_t contexts);

• breadcrumbs

  void sentry_scope_add_breadcrumb(sentry_scope_t *scope, sentry_value_t breadcrumb);
  void sentry_scope_clear_breadcrumbs(sentry_scope_t *scope);

• attachments (TODO feat: Support modifying attachments after init #433)
• level

  sentry_level_t sentry_scope_get_level(const sentry_scope_t *scope);
  void sentry_scope_set_level(sentry_scope_t *scope, sentry_level_t level);

• other

  void sentry_scope_clear(sentry_scope_t *scope);

Open questions

• should transaction_object and span be cloned?
• add dist for FGenericPlatformSentryScope::Get/SetDist?
• add environment for FGenericPlatformSentryScope::Get/SetEnvironment?

With all this in place, the implementation of CaptureEventWithScope in sentry-unreal could look something along the lines of:

TSharedPtr<ISentryId> FGenericPlatformSentrySubsystem::CaptureEventWithScope(TSharedPtr<ISentryEvent> event, const FSentryScopeDelegate& onScopeConfigure)
{
	FScopeLock Lock(&CriticalSection);

	sentry_value_t nativeEvent = Event->GetNativeObject();

	sentry_uuid_t id = sentry_capture_event_with_scope(nativeEvent, [](sentry_scope_t *scope, void *userdata) {
		TSharedPtr<FGenericPlatformSentryScope> NewLocalScope = MakeShareable(new FGenericPlatformSentryScope(scope));
		const FSentryScopeDelegate *onScopeConfigure = static_cast<const FSentryScopeDelegate *>(userdata);
		onScopeConfigure->ExecuteIfBound(NewLocalScope);
	}, &onScopeConfigure);

	return MakeShareable(new FGenericPlatformSentryId(id));
}

[tustanivsky]

add dist for FGenericPlatformSentryScope::Get/SetDist?
add environment for FGenericPlatformSentryScope::Get/SetEnvironment?

These two set via options and shouldn't change at runtime so apparently we don't need to expose setters/getters for them.

[tustanivsky]

From the Unreal SDK perspective this API looks good and should allow us to align the scope implementation across different platforms. That said let's hear what @supervacuus and @JoshuaMoelans think.

[supervacuus]

We would like to add support for local scopes for the needs of downstream SDKs (sentry-unreal).

Regarding other downstream scope variants, could this be compared to the scenario for

• Current Scope in Java, just that this one allows to capture multiple events within a "scoped" callback (could also be achieved by reusing the same scope callback + userdata).
• Local Scope which seems like direct translation

Since the scope must be redefined with each capture, it is not an Isolation Scope. However, the client could store their scope callback for a particular isolation context (like a request handler or some execution context that isn't a thread). So the proposed design leaves maintenance of the local scope to the client.

Is this the correct interpretation of this particular design?

Clones the current scope and pushes it on a (thread-local?) stack

What would be the value of pushing the scope to a stack if the scope only holds for the duration of a particular capture?

Similarly, if we make something thread-local, that would still be up to the client in the proposed design, right? If we provide thread-local scopes, then those should be maintained by the SDK, right? The proposed design essentially leaves this up to the client.

The core question for the proposed implementation is when this callback will be called, and I guess that would happen at the point where we would usually apply the scope:

• we call the callback before sentry__scope_apply_to_event, where users can adapt the scope to their needs
• then apply the modified scope to the event

In this setup, no storage (either thread-local or stack-based) is required, right? Or am I missing something?

should transaction_object and span be cloned?

I think they should be. For most requests we got for non-global scopes, users are interested in having different span hierarchies in separate execution contexts. Including them here would provide that separation and allow them full control of associating their scopes with a particular execution context.

[jpnurmi]

Thanks for the insights, @supervacuus! I was not familiar with the concept of "isolation scopes", but this term helped me to find Hub & Scope Refactoring in the dev docs.

If I understand correctly, the global scope should be frozen at initialization time, and all sentry_set_xxx modifications should go to a thread-local isolation scope. Finally, sentry_capture_event_with_scope would provide ephemeral local scopes. I hope this can be managed internally in the SDK, and isolation scopes don't necessarily need to be exposed in the API until there's demand/use case.

What comes to applying scope data to events, I thought it would only apply the current scope (which has been cloned/inherited from global+isolation scopes), but the dev docs suggest merging:

How Is Scope Data Applied to Events

Data from the different scope flavors is merged before it is applied to the event.

This is done in the following order:

1. take the data from the global scope
2. merge in the data from the isolation scope
3. merge in the data from the current scope
4. optional: merge in given additional data
5. apply the merged scope to the event

The idea of stacking scopes came directly from other SDKs providing a more generic with_scope API for executing an arbitrary piece of code with potential nesting, but is indeed not necessary for the simpler case of capturing events with scoped data.

[supervacuus]

Thanks for the insights, @supervacuus! I was not familiar with the concept of "isolation scopes", but this term helped me to find Hub & Scope Refactoring in the dev docs.

Yeah, I wasn't sure how much exposure you already had, so I just started somewhere. My primary concern is that we do not introduce an API that could conflict with any other variant that we might need to implement for other downstream uses.

If I understand correctly, the global scope should be frozen at initialization time, and all sentry_set_xxx modifications should go to a thread-local isolation scope. Finally, sentry_capture_event_with_scope would provide ephemeral local scopes.

Yeah, this is where the Native SDK currently deviates. The global scope is mutable and thread-safe, but it won't make sense for different threads to change it without considerable coordination. Thread-level isolation via TLS can make sense in some scenarios, but most people who have addressed this are interested in isolating along different execution-context boundaries, like

• I/O multiplexing paths (think select/poll, epoll, kqueue, etc.)
• cooperative/fiber-like "tasks" (Boost.Fiber, libco, etc.)
• last but certainly not least: worker-threads from a thread-pool, where TLS isolation is as useless as a global scope (since you typically want scope isolation along tasks or task-types and not share those across whatever runs on a thread)

So, while we won't provide an all-fitting abstraction immediately, having forced TLS isolation isn't a significant improvement for many users (even with multiple threads). My mid- to long-term target would be to have interfaces for "managed" TLS isolation along global scope changes, and maybe a "named" scope where users can look up a particular scope from the context variables of their respective execution context (which in native, in contrast to practically all managed code, all have inconsistent abstractions if any). I can also imagine that TLS isolation, together with nested with_scope, could solve some of the above scenarios (but certainly not all).

In any case, this local scope solution is an excellent fallback as an intermediate solution for all of the above scenarios, because

• Users have complete control over their userdata and to which execution context they bind it (they could reference TLS for instance)
• it is entirely stateless and thus wouldn't interact with any future "managed" isolation implementation
• it can still be abstracted on the client-side in a fashion that allows reuse across captures, even though we do not provide with_scope yet

I hope this can be managed internally in the SDK, and isolation scopes don't necessarily need to be exposed in the API until there's demand/use case.

Yes, now that I understand the proposal better, we can exclude thinking about SDK- "managed" scopes in the context of this issue and consider local scopes a stateless counterpoint to the currently available global scope.

What comes to applying scope data to events, I thought it would only apply the current scope (which has been cloned/inherited from global+isolation scopes), but the dev docs suggest merging:

We reach the same outcome by cloning the global scope and letting users overwrite in the local scope callback. I think the Native SDK also has a bit of leeway to explore how we can close in on this ideal from the dev docs. I think we follow the general idea.

If you mean this affects the order we apply (cloning global + modifying in the callback vs setting in the callback and then merging from global), I am open to any suggestions. However, I think the difference will have minimal effect on the current proposal.

The idea of stacking scopes came directly from other SDKs providing a more generic with_scope API for executing an arbitrary piece of code with potential nesting, but is indeed not necessary for the simpler case of capturing events with scoped data.

Okay, now I understand. If we introduce scoping that overlaps with lexical scoping (which also means it can be nested), then we'd need to keep a stack of the wrapping scopes to exit the inner. I think we can exclude this from this issue.

[supervacuus]

TL;DR of #1226 (comment): feel free to go ahead.

Please keep in mind that particularly span/transaction scoping should be well tested (also in the context of the recent propagation-context changes), and maybe keep @JoshuaMoelans in the loop because he has a couple of scenarios sketched that should be cross-checked.

[supervacuus]

What comes to applying scope data to events, I thought it would only apply the current scope (which has been cloned/inherited from global+isolation scopes), but the dev docs suggest merging:

I just had a conversation with @bruno-garcia. From his perspective, there should be no cloning at all (not only transaction/span), just an empty write-only scope instance, modified by the user and merged into the event after the global scope is applied.

This way, we have the merge-sequencing as the dev docs describe, and essentially no use of scope state inside the callback. This means we can eliminate all getters and clear/remove interfaces.

Transactions/spans do not make sense in this setup since they aren't cloned and cannot be retrieved to create children.

While we can keep the current callback setup, given the severe limitations, why do we use callbacks plus user data, where a user could do anything, including scoping new transactions or capturing other events?

Our users could have a much simpler:

// keep the opaque scope in the header
sentry_scope_t *local_scope = sentry_local_scope_new(); // or whatever naming, which initializes everything empty
// provide any "void sentry_scope_set_*" besides for "transaction_object", "span" and "propagation_context"
sentry_capture_event_with_scope(my_event, local_scope);

// Or applied to the sentry-unreal code snippet

TSharedPtr<ISentryId> FGenericPlatformSentrySubsystem::CaptureEventWithScope(TSharedPtr<ISentryEvent> event, const FSentryScopeDelegate& onScopeConfigure)
{
	FScopeLock Lock(&CriticalSection);

	sentry_value_t nativeEvent = Event->GetNativeObject();
	sentry_scope_t *scope =  sentry_local_scope_new();

	TSharedPtr<FGenericPlatformSentryScope> NewLocalScope = MakeShareable(new FGenericPlatformSentryScope(scope));
	onScopeConfigure->ExecuteIfBound(NewLocalScope);

	sentry_uuid_t id = sentry_capture_event_with_scope(nativeEvent, scope);
	return MakeShareable(new FGenericPlatformSentryId(id));
}

This way, there is no chance that something in the callback would have unintentional side effects, and it allows the same abstractions (wrap it in a lexical scope or a function). It also signals clearly that this is only used to overwrite/merge the global scope with the values you provide.

We can still create a callback interface similar to other SDKs, but for this use case, it looks pretty unnatural in C. We do not need to be somewhere inside the core execution to hook in since we provide no context to the user. If we do it, we should clearly document which functions are allowed to be called in the callback.

[jpnurmi]

Would missing getters be a problem for downstream SDKs trying to wrap the scope object? (FGenericPlatformSentryScope::GetXxx)

I heard the new refactored scopes have been implemented in Python, JS, and Java. Not cloning makes sense for the described merge sequence, but if I'm looking in the correct place, all of these provide getters for scope data and are doing full clones:

• scope.py
• scope.ts
• Scope.java

[supervacuus]

I am open to any sensible design.

We now have two extremes spanning the design space of how to interpret the needs we serve with this.

1. The restricted end that allows users to change scope entries for a particular event Local scopes #1226 (comment), which is an alternative approach summarized from the Discord thread.
2. The more flexible/powerful approach, which opens to potentially problematic interactions (Local scopes #1226 (comment)), is synthesized from your proposed interfaces.

I can live with both, and they allow different things, but the truth is somewhere in between. I agree that we shouldn't deviate significantly from other "recent" implementations. However, we should still consider what makes sense for users of the Native SDK (direct or via a downstream SDK).

[jpnurmi]

@tustanivsky says missing getters might not be a problem, so let's give it a try with that approach!