Sitelet https://github.com/Microsoft/ApplicationInsights-aspnetcore/pull/852
Skip to content
This repository was archived by the owner on Jun 10, 2020. It is now read-only.

Remove DiagnosticAdapter and use DiagnosticListener directly - #852

Merged
Liudmila Molkova (lmolkova) merged 4 commits into
developfrom
lmolkova/NoDiagnosticAdapter
Mar 23, 2019
Merged

Liudmila Molkova (lmolkova) merged 4 commits into
developfrom
lmolkova/NoDiagnosticAdapter

Conversation

@lmolkova

@lmolkova Liudmila Molkova (lmolkova) commented Mar 20, 2019 •

Copy link
Copy Markdown

Try out using DiagnosticListener without DiagnosticAdapter and see it it improves performance


public void OnNext(KeyValuePair<string, object> value)
{
if (value.Key == "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better as a switch instead of an if/else/../else chain.

@lmolkova Liudmila Molkova (lmolkova) changed the title [WIP] Remove DiagnosticAdapter and use DiagnosticListener directly Remove DiagnosticAdapter and use DiagnosticListener directly Mar 22, 2019
@lmolkova

Copy link
Copy Markdown
Author

I think Dmitry-Matveev has proof that it indeed saves some performance and it should be merged.
Current functional tests are sufficient.

I believe the main reason for perf hit from DiagnosticsAdapter was in IRouteData interface - we created this to avoid taking the dependency on the Microsoft.AspNetCore.Routing.Abstractions and DiagnosticAdapter was dynamically typing IRouteData from that package to ours.

@cijothomas Cijo Thomas (cijothomas) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Liudmila Molkova (@lmolkova) Please add changelog.md as well.

@cijothomas Cijo Thomas (cijothomas) added this to the 2.7.0 milestone Mar 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants