Repository navigation
Add overload to prevent delegate allocation for ConfigureScope #4228
Description
Problem Statement
I use the following code to configure a scope, capturing a value to be set as a tag on the scope. Unfortunately, this always results in a delegate allocation because of the capture.
string tagValue = "value";
SentrySdk.ConfigureScope(scope => scope.SetTag("tag_name", tagValue));Solution Brainstorm
Perhaps there already is a better way to set a tag on the current scope, but otherwise I'd propose adding the following methods:
public static partial class SentrySdk
{
+ public static void ConfigureScope<TArg>(Action<Scope, TArg> configureScope, TArg arg);
+ public static Task ConfigureScopeAsync<TArg>(Func<Scope, TArg, Task> configureScope, TArg arg);
}These methods could then be used like this:
string tagValue = "value";
SentrySdk.ConfigureScope(
static (scope, arg) => scope.SetTag("tag_name", arg),
tagValue);I'd be happy to provide the implementation if this is something you're interested in.
Activity
jamescrosswell commentedon May 28, 2025
@bruno-garcia maybe you've got some context around this.
In the Hub we have this internally:
sentry-dotnet/src/Sentry/Internal/Hub.cs
Line 34 in 34d8490
Why is it we only expose that via a ConfigureScope callback (instead of surfacing it directly via the IHub interface or some public extension method)?
bruno-garcia commentedon May 28, 2025
The idea is that once you return the reference explicitly you've lost ownership over it. One can hold it in a static field, access it concurrently etc. So having that state be accessed only behind our public API (SetTag, AddBreadcrumb, etc) allows us to control access, including having it be threadlocal, or asynclocal, or keep it per HTTP request (aspnet classic) etc.
bruno-garcia commentedon May 28, 2025
We should start with adding
SentrySdk.SetTag, surprised we actualy don't have that yet.Also not against adding overloads that allow for 0 allocations of course, but I do think we can add some stuff to our public API like SetTag and other common things.
KnapSac commentedon May 29, 2025
That would be even better, as you wouldn't have to go through the hassle of using a callback.
Would you be open to PRs for adding both (i.e.
SentrySdk.SetTagand these 0 allocation overloads)?bruno-garcia commentedon May 29, 2025
That'd be awesome. Would you start with SetTag please? Think that's slam dunk. The other one we might want to discuss further (just want to hear from @james.crosswell and @bitsandfoxes before we go ahead)
6 remaining items
bruno-garcia commentedon Jun 2, 2025
That's a good point. But we cold add the overload as
internalthen and keep the public API simpler, in case one concern is making things confusing to users (I'm slightly concerned about that)KnapSac commentedon Jun 2, 2025
Alright, I'll throw up a PR with the overload as internal. Should consumption of the new overload be a separate PR, or are you fine having it in the same one?
bruno-garcia commentedon Jun 2, 2025
Internal consumption it's fine on the same PR, but your call. Might be easier to get it added first and only change a couple spots as 'examples'. Get it merged. Then 1 large full rewrite? not sure, whatever it's easier for you and reviewers
Thanks a lot!!!
Flash0ver commentedon Jun 3, 2025
Fine for me either way, but I do have a slight preference towards "add and adopt in one big go" when it comes to public API. This way we may figure out potential problems when consuming in some corner cases before merging. To be fair, with the new overloads in question, I doubt adopting these will be anything else but straightforward.
We could also do an initial draft-PR including the new overloads, and then branch off of that with a new PR into that draft-PR where we use the new overloads everywhere. This way we could get smaller more focused reviews, and still detect potential (admittedly unlikely) consumption issues before merging and potentially publishing.
But I do leave it to your preference of working / reviewing.