-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
Allow custom processors to be added as a scoped dependency #1979
Conversation
test/Sentry.AspNetCore.Tests/IntegrationsTests.EventProcessor.cs
Outdated
Show resolved
Hide resolved
Thanks for this! Looks promising on first glance. I'll review in more detail later this week. |
wouldn't this result in: processors explicitly added to options being ignored? |
Few test failed. Let me take a look.
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. |
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. |
Instead, let's control this based on how the processor was registered with DI.
|
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. |
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. |
No, because sentry-dotnet/src/Sentry/ScopeExtensions.cs Lines 18 to 33 in 77ccdb2
|
Needs transaction processors added, and a changelog entry, then LGTM. Thanks! |
Co-authored-by: Matt Johnson-Pint <mattjohnsonpint@gmail.com>
I added the changelog entry. We'll do transaction processors in a separate PR. Thanks for your contribution! |
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 usingPopulateScope
duringOnEvaluation
.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 (testingPopulateScope
). Let me know if you have other opinion.