[Design] Add DiagnosticSource to RazorView #6386
Conversation
| @@ -46,7 +47,8 @@ public class HtmlHelper : IHtmlHelper, IViewContextAware | |||
| IModelMetadataProvider metadataProvider, | |||
| IViewBufferScope bufferScope, | |||
| HtmlEncoder htmlEncoder, | |||
| UrlEncoder urlEncoder) | |||
| UrlEncoder urlEncoder, | |||
| DiagnosticSource diagnosticSource) | |||
There was a problem hiding this comment.
@Eilon do you want this breaking change? or add a new ctor?
There was a problem hiding this comment.
FWIW in other libs, we're going the clean route and changing the existing constructor since this it's a V2 release. Keep it clean since it's a good time to do so, IMO.
| { | ||
| "TypeId": "public class Microsoft.AspNetCore.Mvc.ViewFeatures.HtmlHelper<T0> : Microsoft.AspNetCore.Mvc.ViewFeatures.HtmlHelper, Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper<T0>", | ||
| "MemberId": "public .ctor(Microsoft.AspNetCore.Mvc.ViewFeatures.IHtmlGenerator htmlGenerator, Microsoft.AspNetCore.Mvc.ViewEngines.ICompositeViewEngine viewEngine, Microsoft.AspNetCore.Mvc.ModelBinding.IModelMetadataProvider metadataProvider, Microsoft.AspNetCore.Mvc.ViewFeatures.Internal.IViewBufferScope bufferScope, System.Text.Encodings.Web.HtmlEncoder htmlEncoder, System.Text.Encodings.Web.UrlEncoder urlEncoder, Microsoft.AspNetCore.Mvc.ViewFeatures.Internal.ExpressionTextCache expressionTextCache)", | ||
| "Kind": "Removal" |
There was a problem hiding this comment.
Eilon needs to approve these btw
| var adapter = new TestDiagnosticListener(); | ||
|
|
||
| var diagnosticSource = new DiagnosticListener("Test"); | ||
| diagnosticSource.SubscribeWithAdapter(adapter); |
There was a problem hiding this comment.
If you're not using it, you don't need this, just the new DiagnostisListener
| public void PartialMethods_DoesNotWrapThrownException( | ||
| Func<IHtmlHelper, IHtmlContent> partialMethod, | ||
| object unusedModel, | ||
| ViewDataDictionary unusedViewData) |
| return viewContext; | ||
| } | ||
|
|
||
| private class NullTempDataProvider : ITempDataProvider |
There was a problem hiding this comment.
Mock.Of<ITempDataProvider> would do the same thing
| Html5DateRenderingMode = Html5DateRenderingMode.CurrentCulture, | ||
| ValidationSummaryMessageElement = "test", | ||
| ValidationMessageElement = "test", | ||
| TempData = new TempDataDictionary(httpContext, new NullTempDataProvider()) |
There was a problem hiding this comment.
A lot of these things don't need to be initialized.
| @@ -582,5 +618,52 @@ public override string ToString() | |||
| return "test-model-content"; | |||
| } | |||
| } | |||
|
|
|||
| internal class TestHtmlHelper : HtmlHelper | |||
There was a problem hiding this comment.
Is this the pattern that other HtmlHelper*ExtensionsTest classes follow?
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| private ViewContext TestViewContext() |
There was a problem hiding this comment.
Name this like CreateViewContext()
|
Agree with existing notes, thanks for digging into this! I need it for MiniProfiler and haven't had the time, would love to have this fixed in 2.0. |
This is indeed planned for 2.0.0 |
jbagga
force-pushed
the
jbagga/DiagnosticsPartialViews6222
branch
from
8ddc2ad to
59180ff
Compare
|
@rynowak Updated |
jbagga
force-pushed
the
jbagga/DiagnosticsPartialViews6222
branch
from
59180ff to
7a1e91c
Compare
jbagga
force-pushed
the
jbagga/DiagnosticsPartialViews6222
branch
from
7a1e91c to
b6a122e
Compare
|
@rynowak Updated. Changed the initial design |
jbagga
changed the title
| { | ||
| diagnosticSource.Write( | ||
| "Microsoft.AspNetCore.Mvc.Razor.BeforeRazorView", | ||
| new { page = page, viewContext = viewContext, }); |
There was a problem hiding this comment.
include the action descriptor and http context with the same names as every other event. yes it's redundant and we do that on purpose
There was a problem hiding this comment.
I tried to make it consistent with BeforeView and AfterView here.
Should these also include the ActionDescriptor and HttpContext?
There was a problem hiding this comment.
No, the viewcontext gets you access to the two.
There was a problem hiding this comment.
No, the viewcontext gets you access to the two.
yes it's redundant and we do that on purpose
YES THESE NEED TO INCLUDE THE ACTION DESCRIPTOR AND HTTP CONTEXT.
We do that on purpose so that you don't have to maintain a million proxies.
If another event is missing those fields, they should be added.
| _pageActivator.Activate(page, context); | ||
|
|
||
| _diagnosticSource.AfterRazorView(page, context); |
There was a problem hiding this comment.
This isn't right. You need to add a try/finally around L175 and surround that
jbagga
changed the title
| { | ||
| diagnosticSource.Write( | ||
| "Microsoft.AspNetCore.Mvc.Razor.BeforeRazorView", | ||
| new { page = page, viewContext = viewContext, }); |
There was a problem hiding this comment.
No, the viewcontext gets you access to the two.
| @@ -155,7 +165,13 @@ public class RazorView : IView | |||
| private Task RenderPageCoreAsync(IRazorPage page, ViewContext context) | |||
| { | |||
| page.ViewContext = context; | |||
|
|
|||
| _diagnosticSource.BeforeRazorView(page, context); | |||
There was a problem hiding this comment.
The methods should probably be named BeforeRazorPage \ AfterRazorPage since they're logging individual page execution.
jbagga
force-pushed
the
jbagga/DiagnosticsPartialViews6222
branch
from
b6a122e to
6bc17c5
Compare
| IRazorPage page, | ||
| ViewContext viewContext) | ||
| { | ||
| if (diagnosticSource.IsEnabled("Microsoft.AspNetCore.Mvc.Razor.AfterRazorView")) |
There was a problem hiding this comment.
Is it page or view? the method says page, but the event says view
| { | ||
| return page.ExecuteAsync(); | ||
| } | ||
|
|
|
|
||
| try | ||
| { | ||
| return page.ExecuteAsync(); |
There was a problem hiding this comment.
This isn't valid, this needs to be an await
|
@rynowak Updated |
Addresses #6222
cc @rynowak