Allow custom processors to be added as a scoped dependency

[chrisza4]

Refer from issue #318

We can fixed this by turning SentryMiddleware to be a factory-based middleware. But this also required some interfaces changes and now all custom event processors will be injected using PopulateScope during OnEvaluation.

One concern is that this changes might break behaviour of public method GetAllEventProcessors since now it can only be used after scope is evaluated. I'm not sure if that could be a concern.

Alternatively, we can move out from onEvaluating hook but I don't know if that is more useful so I just put it there first to make it easier to write test in same pattern (testing PopulateScope). Let me know if you have other opinion.

[mattjohnsonpint]

Thanks for this! Looks promising on first glance. I'll review in more detail later this week.

[SimonCropp]

wouldn't this result in: processors explicitly added to options being ignored?

[chrisza4]

Few test failed. Let me take a look.

@SimonCropp

wouldn't this result in: processors explicitly added to options being ignored?

As I read into Sentry SDK doc, I thought we can only use processors via dependency injection. If there is other way then let me know where and I can take a look.

[mattjohnsonpint]

During initialization, event processors can be added to the options:

.UseSentry(options =>
{
    options.AddEventProcessor(new MyEventProcessor());
}

They can also be added for a given scope:

SentrySdk.ConfigureScope(scope =>
{
    scope.AddEventProcessor(new MyEventProcessor());
});

Both of those would need to continue to work.

[mattjohnsonpint]

Looking over the PR, the main issue I see is now *all* processors are registered on the scope.

Instead, let's control this based on how the processor was registered with DI.

• If registered using AddSingleton, then it should be applied globally - as if registered directly on the options during initialization. This is the existing behavior.
• If registered using AddScoped, then it should be applied for the current scope - as if registered with SentrySdk.ConfigureScope.

[mattjohnsonpint]

Actually, ignore my last comments. There's no need to separate those paths out, as the effect would be the same as the approach you're currently taking. In other words, registering the same instance of an event processor on each scope is identical to registering it once on options - since we gather both at the same time when we run them.

[mattjohnsonpint]

I see we are doing this for event processors and exception processors. Please also do the same for transaction processors added in #1862. Looks like we missed this back then. Thanks.

[mattjohnsonpint]

@SimonCropp

wouldn't this result in: processors explicitly added to options being ignored?

No, because scope.GetAllEventProcessors() returns both:

sentry-dotnet/src/Sentry/ScopeExtensions.cs

Lines 18 to 33 in 77ccdb2

	/// <summary>
	/// Invokes all event processor providers available.
	/// </summary>
	/// <param name="scope">The Scope which holds the processor providers.</param>
	public static IEnumerable<ISentryEventProcessor> GetAllEventProcessors(this Scope scope)
	{
	foreach (var processor in scope.Options.GetAllEventProcessors())
	{
	yield return processor;
	}
	foreach (var processor in scope.EventProcessors)
	{
	yield return processor;
	}
	}

[mattjohnsonpint]

Needs transaction processors added, and a changelog entry, then LGTM. Thanks!

[mattjohnsonpint]

I added the changelog entry. We'll do transaction processors in a separate PR. Thanks for your contribution!