Skip to content
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

ASP.NET Core 3.x: use Diagnostic bits #475

Merged
merged 5 commits into from Apr 22, 2020

Conversation

NickCraver
Copy link
Member

@NickCraver NickCraver commented Apr 19, 2020

This adds timings based on the DiagnosticListener bits overhauled in dotnet/aspnetcore#11730 with some further cleanup in dotnet/aspnetcore@b23ea5b

This would also resolve #162 and #322.

Entirely possible there are more efficient ways to do the correlation tracking (I hope!) - putting this up for more eyes.

Example profiling:
image

cc @benaadams @rynowak @vcsjones

This adds timings based on the DiagnosticListener bits overhauled in dotnet/aspnetcore#11730 with some further cleanup in dotnet/aspnetcore@b23ea5b

This would also resolve #162 and #322.
@NickCraver NickCraver linked an issue Apr 19, 2020 that may be closed by this pull request
var action = descriptor.RouteValues.TryGetValue("action", out var a) ? a : "UnknownAction";

// TODO: Don't allocate this string more than once
return label + ": " + controller + "/" + action;
Copy link

@rynowak rynowak Apr 19, 2020

Choose a reason for hiding this comment

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

Is there a reason to avoid using ActionDescriptor.DisplayName? This approach assumes everything is a controller, and possibly assumes that everything has a unique action/controller name (definitely not true).

ActionDescriptors all have a unique ID (not stable across launches) also

Copy link
Member Author

@NickCraver NickCraver Apr 19, 2020

Choose a reason for hiding this comment

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

Mainly just that it's so long, they'd all appear the same way in the UI (e.g. seeing namespaces and ...) - maybe I could take a reverse truncation? I need to store a map of these so we don't reallocate any approach per-request, that's just being wasteful for something we can assume is fairly finite. I could just make name -> short name in a dictionary here I suppose. Let me play and see what looks good!

Copy link
Member Author

@NickCraver NickCraver Apr 21, 2020

Choose a reason for hiding this comment

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

Pushed up a new version here - may play with the caching a bit, just still catching up across the board after being out last week so a bit slower than usual. Thanks for all the comments here! Hopefully I'll be able to dogfood on Stack Overflow this week to get some testing in.

NickCraver added 3 commits Apr 19, 2020
Also puts the title for all cases on profiler steps. Not strictly related to this change (can PR the diff separately), but needed for the longer action names...so combining it. Also moves to CSS truncation instead of a magical number. Yay for modern browsers!
Makes action descriptor names a bit cleaner by removing the assembly name. Also caches so we only discard a cheaper tuple for lookup only. Could change to per-type methods and eliminate the tuple as well, or a dictionary based on key, or something else. Just depends how crazy we want to go here.
@NickCraver NickCraver merged commit 98118e9 into master Apr 22, 2020
3 checks passed
@NickCraver NickCraver deleted the craver/aspnetcore-mvc3-diagnostics branch Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement .net-core
Projects
None yet
2 participants