Skip to content

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

Merged
merged 4 commits into from
Apr 17, 2025

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Apr 16, 2025

Blazor - rendering metrics

new Microsoft.AspNetCore.Components.Rendering meter

  • has component.type which is GetType().FullName of the component being rendered
  • aspnetcore.components.rendering.count
    • this is total count, always growing. The viewer could calculate "component render/minute"
  • aspnetcore.components.rendering.duration
    • per component

new Microsoft.AspNetCore.Components.Server.Circuits meter

  • aspnetcore.components.circuits.count
    • this is total count, always growing. The viewer could calculate "new circuits/minute"
  • aspnetcore.components.circuits.active_circuits
    • in server memory
  • aspnetcore.components.circuits.connected_circuits
    • with connected signalR
  • aspnetcore.components.circuits.duration
    • from creation until GC

How to enable with OpenTelemetry/Aspire

builder.Services.ConfigureOpenTelemetryMeterProvider(meterProvider =>
{
    meterProvider.AddMeter("Microsoft.AspNetCore.Components.Rendering");
    meterProvider.AddMeter("Microsoft.AspNetCore.Components.Server.Circuits");
});

Fixes #53613

image

@pavelsavara pavelsavara added feature-diagnostics Diagnostic middleware and pages (except EF diagnostics) area-blazor Includes: Blazor, Razor Components labels Apr 16, 2025
@pavelsavara pavelsavara added this to the 10.0-preview5 milestone Apr 16, 2025
@pavelsavara pavelsavara self-assigned this Apr 16, 2025
@pavelsavara pavelsavara force-pushed the blazor_rendering_metrics branch from 6f6a498 to 51c936d Compare April 16, 2025 21:31
@@ -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
Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

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 like AddComponentsCore 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).

@pavelsavara pavelsavara marked this pull request as ready for review April 17, 2025 09:27
@pavelsavara pavelsavara requested a review from a team as a code owner April 17, 2025 09:27
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

@pavelsavara pavelsavara enabled auto-merge (squash) April 17, 2025 11:11
@pavelsavara
Copy link
Member Author

/ba-g CI timeout is #60989

@pavelsavara pavelsavara disabled auto-merge April 17, 2025 12:43
@pavelsavara pavelsavara enabled auto-merge (squash) April 17, 2025 12:44
@akoeplinger akoeplinger disabled auto-merge April 17, 2025 12:46
@pavelsavara pavelsavara enabled auto-merge (squash) April 17, 2025 12:47
@pavelsavara pavelsavara merged commit 183c128 into dotnet:main Apr 17, 2025
26 of 27 checks passed
Comment on lines +96 to +97
var meterFactory = serviceProvider.GetService<IMeterFactory>();
_renderingMetrics = meterFactory != null ? new RenderingMetrics(meterFactory) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a case where IMeterFactory is not ever on DI?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 AddMetrics and rely on it being there. I don't think this dependency should be optional.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking [FeatureSwitchDefinition("System.Diagnostics.Metrics.Meter.IsSupported")]

Comment on lines +935 to +943
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());
Copy link
Member

Choose a reason for hiding this comment

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

Several questions on this block:

  • Do we understand what this is measuring?
    • This is measuring the Render method on the component, which just updates the RenderTree, which might not be very interesting.
    • It might make more sense to measure when a render batch starts and ends, as that more clearly represents the time it took the app to create a single "snapshot" update.
    • A more representative thing to measure is SetComponentParametersAsync which is where most of the application logic for an app lives.
  • Do we know the perf cost of this? (Since it happens per render)
    • Does it make sense to cache the component FullName inside ComponentState

Copy link
Member Author

Choose a reason for hiding this comment

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

  * It might make more sense to measure when a render batch starts and ends

I agree that duration of whole batch could be separate metric.

  * A more representative thing to measure is `SetComponentParametersAsync` which is where most of the application logic for an app lives.

OK. I will need some help to understand what are all the places in which this is called.
You mean ComponentBase.SetParametersAsync(), right ?

* Do we know the perf cost of this? (Since it happens per render)

It depends on if there is listener attached or not. When it's not it's negligible.

  * Does it make sense to cache the component `FullName` inside ComponentState

Reflection already does some caching.

@javiercn javiercn requested a review from JamesNK April 21, 2025 09:36
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Overall changes look great. That said, there are a few things that I think we need to revisit:

  • Measuring renders:
    • We want to track from the start of a render batch to the completion of such render batch.
    • We want to track SetParametersAsync on component instances as this is where most of the user code lives, the execution time of RenderFragment is "generally" not relevant as all that code does is fill in an array of RenderTreeFrames and for the most part is compiler generated code.
      • It's valuable though to measure/track the size of the render tree of a given component, as that is a good indicator of components that are rendering a lot of data.
      • If we want to track the "impact" of a render, it's best to do so in RenderTreeDiffBuilder.ComputeDiff this is the bit of logic that actually does work like instantiate and set parameters on child components and so on.
  • Circuit metrics:
    • I think we might want to track the reason why a circuit is terminated (gracefully (session ended), timeout (circuit got disconnected and timed out), capacity (there were too many disconnected circuits)).
  • Performance considerations:
    • In general, we need to be very careful on the things that we do on a per-component basis, as a general principle, minimize allocations and avoid doing work unless needed. This is the "hotest" path for Blazor, so we need to ensure we don't regress perf.

"aspnetcore.components.circuits.duration",
unit: "s",
description: "Duration of circuit.",
advice: new InstrumentAdvice<double> { HistogramBucketBoundaries = MetricsConstants.VeryLongSecondsBucketBoundaries });
Copy link
Member

@JamesNK JamesNK Apr 21, 2025

Choose a reason for hiding this comment

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

What is wrong with the existing LongSecondsBucketBoundaries? That's already used for Kestrel connection duration and signalr connection duration.

I don't like having 3 different durations.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is used to bucketize the circuit duration, which I think naturally spans longer than the SignalR/Http one. If we were to use those, doesn't that mean that most data will end up in a single bucket? Ideally the buckets should be representative of the "durations" that we expect to track, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

The biggest value in th existing buckets is 5 minutes. Do you think most circuits last longer than 5 minutes?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. Circuits are associated with the "session" (a browser tab), so it's very feasible that they remain open while the user is looking at the tab. As for how long they can last, it's very app specific, but I think we want to have enough granularity to track whether sessions are very short (1-10 minutes) or last significantly longer (10 minutes, hours if the users leave the browser tab open and don't have energy saving settings)

Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific problem with having a different set of numbers? Is there a recommended number of values that we should strive for?

Copy link
Member

Choose a reason for hiding this comment

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

No there isn't a specific problem. Just there is an extra choice for people to choose from, and good values for the buckets need to be decided.

Copy link
Member

Choose a reason for hiding this comment

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

When you mean people, do you mean us, or do you mean developers consuming the metrics. I assume you mean us?

Copy link
Member

Choose a reason for hiding this comment

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

I mean us.

The comment on the new buckets doesn't seem right. SignalR connection duration goes up to 300 seconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that Blazor circuit could be alive for days. And since circuit keeps the state of the blazor session it could be memory hungry. So understanding the histogram of how long your users keep the tab open matters for sizing your cluster.

I thought that it could happen that 80% of your circuits live over 5 minutes but with the previous buckets you don't know if that's 6 minutes or 6 days.

The comment on the new buckets doesn't seem right. SignalR connection duration goes up to 300 seconds.

Could you please be more specific? I will fix it on next PR. Thanks.

@JamesNK
Copy link
Member

JamesNK commented Apr 21, 2025

These metrics need to be documented at https://learn.microsoft.com/en-us/aspnet/core/log-mon/metrics/built-in

@pavelsavara
Copy link
Member Author

pavelsavara commented Apr 22, 2025

Thank you. I will open another PR to address the feedback.

#61609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-diagnostics Diagnostic middleware and pages (except EF diagnostics)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Metrics for Blazor
4 participants