Skip to content

WrappedView breaks access to RazorView in ViewLocationExpander #322

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

Closed
PTwr opened this issue Sep 4, 2018 · 2 comments · Fixed by #475
Closed

WrappedView breaks access to RazorView in ViewLocationExpander #322

PTwr opened this issue Sep 4, 2018 · 2 comments · Fixed by #475

Comments

@PTwr
Copy link

PTwr commented Sep 4, 2018

In custom IViewLocationExpander I need to access RazorView from (ViewLocationExpander.ActionContext as ViewContext).View as RazorView.

For now I "fixed" it with Reflection, .GetType()?.GetField("_wrapped", BindingFlags.Instance | BindingFlags.NonPublic), but some civilized way would be nice.

WrappedView is currently Internal and wrapped IView is private. So I tried to provide my own implementation. However it must be provided via my own ProfilingViewEngine (which is public while sitting in Internal namespace), which in turn is provided in Configure hidden in internal classes underneath innocent looking AddMiniProfiler so replacing viewEngine requires building configuration from scratch. Which made reflection look like reasonable choice.

Is this blocked by design or for no apparent reason?

@NickCraver
Copy link
Member

You can see #162 for some details here. It's not intentionally blocked and I'd rather we not have this layer at all, but the current state of DiagnosticSource in ASP.NET MVC (no concrete or exposed types) is a nightmare to consume. I've asked (and gotten) the events fixed in aspnet/Mvc#6222, but the types are still anonymous and not exposed. In short, they're terrible for consumption or profiling.

For context, I've discussed this quite a bit with the team that owns the diagnostic bits (though no one "owns" it overall...and that's the problem, IMO). Lots of context in the StackExchange.Redis discussion here: StackExchange/StackExchange.Redis#833

Application Insights, Microsoft's own platform, doesn't use these. The diagnostic events have no first class consumer and as a result they don't seem to get prioritized. I'm really hoping the .NET teams will change the behavior here and make a usable API. I really did try to push for this, but gave up. The Entity Framework team has a great example of fixing this to what it should be in a consumable API, see dotnet/efcore#8351. As a result, MiniProfiler's observer can be quite reasonable on top of those diagnostic messages.

cc @DamianEdwards - can we please, please look at making DiagnosticSource usable in ASP.NET v3? Like the Entity Framework team did?

NickCraver pushed a commit that referenced this issue 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.
@NickCraver NickCraver linked a pull request Apr 19, 2020 that will close this issue
@NickCraver
Copy link
Member

Since concrete types are now in - this is unblocked! I'm working on it over in #475.

NickCraver added a commit that referenced this issue Apr 22, 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](https://user-images.githubusercontent.com/454813/79678735-784f5d00-81cc-11ea-93fc-11516b77ea59.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants