-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Blazor - rendering metrics #61516
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
Blazor - rendering metrics #61516
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Diagnostics.Metrics; | ||
using System.Linq; | ||
using Microsoft.AspNetCore.Components.HotReload; | ||
using Microsoft.AspNetCore.Components.Reflection; | ||
|
@@ -33,6 +34,7 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable | |
private readonly Dictionary<ulong, ulong> _eventHandlerIdReplacements = new Dictionary<ulong, ulong>(); | ||
private readonly ILogger _logger; | ||
private readonly ComponentFactory _componentFactory; | ||
private readonly RenderingMetrics? _renderingMetrics; | ||
private Dictionary<int, ParameterView>? _rootComponentsLatestParameters; | ||
private Task? _ongoingQuiescenceTask; | ||
|
||
|
@@ -90,6 +92,10 @@ public Renderer(IServiceProvider serviceProvider, ILoggerFactory loggerFactory, | |
_logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Components.RenderTree.Renderer"); | ||
_componentFactory = new ComponentFactory(componentActivator, this); | ||
|
||
// TODO register RenderingMetrics as singleton in DI | ||
var meterFactory = serviceProvider.GetService<IMeterFactory>(); | ||
_renderingMetrics = meterFactory != null ? new RenderingMetrics(meterFactory) : null; | ||
Comment on lines
+96
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a case where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In unit tests I think. Also I can imagine that somebody would like to disable it in non-cloud environments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the unit tests we would simply update them. For production workloads our hosts should just call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about WASM, do you think it should always have metrics in DI ? That would make impossible to disable/trim metrics for Blazor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's fine, if we are concerned, we can look at the size before and after the change to understand the delta. In any case we could put it behind and app compat switch so that it gets trimmed by default if not enabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a fair bit of code involved with metrics. I think you'll want a way to toggle it on and off in WASM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking |
||
|
||
ServiceProviderCascadingValueSuppliers = serviceProvider.GetService<ICascadingValueSupplier>() is null | ||
? Array.Empty<ICascadingValueSupplier>() | ||
: serviceProvider.GetServices<ICascadingValueSupplier>().ToArray(); | ||
|
@@ -926,12 +932,15 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) | |
{ | ||
var componentState = renderQueueEntry.ComponentState; | ||
Log.RenderingComponent(_logger, componentState); | ||
var startTime = (_renderingMetrics != null && _renderingMetrics.IsDurationEnabled()) ? Stopwatch.GetTimestamp() : 0; | ||
_renderingMetrics?.RenderStart(componentState.Component.GetType().FullName); | ||
componentState.RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment, out var renderFragmentException); | ||
if (renderFragmentException != null) | ||
{ | ||
// If this returns, the error was handled by an error boundary. Otherwise it throws. | ||
HandleExceptionViaErrorBoundary(renderFragmentException, componentState); | ||
} | ||
_renderingMetrics?.RenderEnd(componentState.Component.GetType().FullName, renderFragmentException, startTime, Stopwatch.GetTimestamp()); | ||
Comment on lines
+935
to
+943
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Several questions on this block:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree that duration of whole batch could be separate metric.
OK. I will need some help to understand what are all the places in which this is called.
It depends on if there is listener attached or not. When it's not it's negligible.
Reflection already does some caching. |
||
|
||
// Process disposal queue now in case it causes further component renders to be enqueued | ||
ProcessDisposalQueueInExistingBatch(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics; | ||
using System.Diagnostics.Metrics; | ||
using Microsoft.AspNetCore.Http; | ||
|
||
namespace Microsoft.AspNetCore.Components.Rendering; | ||
|
||
internal sealed class RenderingMetrics : IDisposable | ||
{ | ||
public const string MeterName = "Microsoft.AspNetCore.Components.Rendering"; | ||
|
||
private readonly Meter _meter; | ||
private readonly Counter<long> _renderTotalCounter; | ||
private readonly UpDownCounter<long> _renderActiveCounter; | ||
private readonly Histogram<double> _renderDuration; | ||
|
||
public RenderingMetrics(IMeterFactory meterFactory) | ||
{ | ||
Debug.Assert(meterFactory != null); | ||
|
||
_meter = meterFactory.Create(MeterName); | ||
|
||
_renderTotalCounter = _meter.CreateCounter<long>( | ||
"aspnetcore.components.rendering.count", | ||
unit: "{renders}", | ||
description: "Number of component renders performed."); | ||
|
||
_renderActiveCounter = _meter.CreateUpDownCounter<long>( | ||
"aspnetcore.components.rendering.active_renders", | ||
unit: "{renders}", | ||
description: "Number of component renders performed."); | ||
pavelsavara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
_renderDuration = _meter.CreateHistogram<double>( | ||
"aspnetcore.components.rendering.duration", | ||
unit: "ms", | ||
pavelsavara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description: "Duration of component rendering operations per component.", | ||
advice: new InstrumentAdvice<double> { HistogramBucketBoundaries = MetricsConstants.ShortSecondsBucketBoundaries }); | ||
} | ||
|
||
public void RenderStart(string componentType) | ||
{ | ||
var tags = new TagList(); | ||
tags = InitializeRequestTags(componentType, tags); | ||
pavelsavara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (_renderActiveCounter.Enabled) | ||
{ | ||
_renderActiveCounter.Add(1, tags); | ||
} | ||
if (_renderTotalCounter.Enabled) | ||
{ | ||
_renderTotalCounter.Add(1, tags); | ||
} | ||
} | ||
|
||
public void RenderEnd(string componentType, Exception? exception, long startTimestamp, long currentTimestamp) | ||
{ | ||
// Tags must match request start. | ||
var tags = new TagList(); | ||
tags = InitializeRequestTags(componentType, tags); | ||
|
||
if (_renderActiveCounter.Enabled) | ||
{ | ||
_renderActiveCounter.Add(-1, tags); | ||
} | ||
|
||
if (_renderDuration.Enabled) | ||
{ | ||
if (exception != null) | ||
{ | ||
TryAddTag(ref tags, "error.type", exception.GetType().FullName); | ||
} | ||
|
||
var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp); | ||
_renderDuration.Record(duration.TotalMilliseconds, tags); | ||
pavelsavara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
private static TagList InitializeRequestTags(string componentType, TagList tags) | ||
{ | ||
tags.Add("component.type", componentType); | ||
return tags; | ||
} | ||
|
||
public bool IsDurationEnabled() => _renderDuration.Enabled; | ||
|
||
public void Dispose() | ||
{ | ||
_meter.Dispose(); | ||
} | ||
|
||
private static bool TryAddTag(ref TagList tags, string name, object? value) | ||
{ | ||
for (var i = 0; i < tags.Count; i++) | ||
{ | ||
if (tags[i].Key == name) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
tags.Add(new KeyValuePair<string, object?>(name, value)); | ||
return true; | ||
} | ||
pavelsavara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
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.
It would be good to register
RenderingMetrics
as singleton. But I think it should be done in one of the "Extensions" helpers and I'm not sure which. This could be done in next PR when @javiercn is back.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.
There is one place for server https://github.com/dotnet/aspnetcore/blob/main/src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceCollectionExtensions.cs#L36 and one for WebAssembly https://github.com/dotnet/aspnetcore/blob/main/src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs#L299
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.
This is going to be "painful" because it lives in the
Microsoft.AspNetCore.Components
. Typically, what we do in these cases is expose a helper method that is then called by the different hosts to register it on DI.There's no way around not introducing a bit of public API for this. If I were to do something, I would go with creating an additional extension method here (look at AddValueProvider for a sample pattern) and have that called in the places that @maraf pointed out.
Later on, we can decide if we prefer to introduce a single extension method that we can pile up things in the future. The pattern MVC follows has
AddMvcCore
for this reason, we could have something likeAddComponentsCore
that is meant to be called out by the individual hosts, but for now let's just stick with what we've been doing so far (that would be my recommendation).