-
Notifications
You must be signed in to change notification settings - Fork 205
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
Only add AspNetCoreErrorDiagnosticsSubscriber if not present #1091
Only add AspNetCoreErrorDiagnosticsSubscriber if not present #1091
Conversation
This commit only adds the `AspNetCoreErrorDiagnosticsSubscriber` if an instance is not already present in the subscribers.
Codecov Report
@@ Coverage Diff @@
## master #1091 +/- ##
===========================================
- Coverage 79.02% 57.25% -21.78%
===========================================
Files 149 149
Lines 7362 7364 +2
===========================================
- Hits 5818 4216 -1602
- Misses 1544 3148 +1604
Continue to review full report at Codecov.
|
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Maybe we could also add a test to cover this.
I think it's going to be somewhat difficult to test this, because the test seams are concrete types and static methods i.e. apm-agent-dotnet/src/Elastic.Apm.AspNetCore/ApmMiddlewareExtension.cs Lines 66 to 83 in c01860c
This could be refactored to use the |
In e.g.
It'd be a bit of a work and additional code to iterate through But this made me think a bit: Is there any case where it makes sense to subscribe to any of our subscribers more than once? 🤔 I think we could generally say: A subscriber should only subscribe once. Because otherwise we'd create events multiple times (in case of So I wonder if it'd be better to have a general check - maybe in The last part about an overall check for each type is just a general thought and it's not really a problem that was ever reported, so if it's offtopic for this PR, then let's just ignore it. |
Added a unit test to assert that only one AspNetCoreErrorDiagnosticListener is registered.
I can see some value in doing this for our subscribers. Perhaps we can open a separate issue to track? |
Nice, looks good to me 👍
Opened #1119. I think this is ok to merge. |
This commit only adds the
AspNetCoreErrorDiagnosticsSubscriber
if an instance is not already present in the subscribers.