-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Blazor - rendering metrics and tracing #61609
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 and tracing #61609
Conversation
328a584
to
cebb68e
Compare
# Conflicts: # src/Components/Components/src/PublicAPI.Unshipped.txt
- add tracing
You're adding a lot of metrics here. I think you should do some performance testing. There is performance overhead of metrics - they require some synronization when incrementing counters and recording values. Having many low level metrics could cause performance issues. |
I removed few and kept only the most useful ones. I have 2 remaining issues
|
I don't know how Blazor circuits are created, but if it's from a Hub method then Activity.Current won't be the HTTP activity. We hop off the HTTP activity on purpose in SignalR: aspnetcore/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs Lines 398 to 403 in 9f2b088
Is that because the HTTP request is still running? I don't think activites show up in the dashboard until they're stopped, and if you're using SignalR you're likely using a websocket request which is long running. |
I'm capturing
This is it, thank you @BrennanConroy ! |
It's also topic to discuss for long running activities on Blazor.
We have 2 way how to deal with them I think
Right now I have short+links implementation. I guess developers use OTEL mostly in production and so even the long running traces would be recorded already. But maybe developers also use it in inner dev loop ? In which case it would be great to have "trace preview" for thing that started but not stopped yet. To not get confused the same way as I did. |
- cleanup
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.
Some naming suggestions.
For sessions, I wonder if when log messages are fired, is the activity context going to be the session activity, and if not is there a way to force it to be? @noahfalk this is an issue with essentially zero length spans - you might want to force log messages to be parented to it but something else is the activity at that point?
{ | ||
{ "component.type", componentType ?? "unknown" }, | ||
{ "component.method", methodName ?? "unknown" }, | ||
{ "attribute.name", attributeName ?? "unknown" } |
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.
I understand why this is based on the attribute, but its really also the event name. would that make more sense as "event"?
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 refers to onclick
in the example below. Attribute makes it easier for me personally but I don't have strong preference. It's attributeName
thru Blazor internals.
@danroth27 do you prefer "event" or something else ?
<button class="btn btn-primary" @onclick="Buy" @focus="OnFocus">Buy</button>
|
||
var tags = new TagList | ||
{ | ||
{ "component.type", componentType ?? "unknown" }, |
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 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.
We don't really have code reference, as in file name and line number.
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.
My initial thought is that the thing being refered to was the Blazor component abstraction in which case component.type and component.method made sense to me. The fact that code.class.name isn't currently defined also nudges me in that direction. If we did want to use code.* attributes instead this feels like another spot where OTel semconv feedback could be helpful.
Thanks, that helps a bunch on understanding where things are at now. A couple thoughts:
Activity.Current can be set at any time, but it can't be set to an Activity that is already stopped. If you did want to produce the set of all log messages that occurred within a given circuit I think the best options would be one of:
|
Co-authored-by: Sam Spencer <54915162+samsp-msft@users.noreply.github.com>
Co-authored-by: Sam Spencer <54915162+samsp-msft@users.noreply.github.com>
Co-authored-by: Sam Spencer <54915162+samsp-msft@users.noreply.github.com>
All, I created open-telemetry/semantic-conventions#2235 in order to validate and document the new metrics in OTEL community. As a result I updated all tags in this PR with prefix |
@lmolkova's feedback on the other PR is
"event" means something specific in otel. But also something in the browser and in Blazor. Suggestion to rename
|
- rename aspnetcore.components.circuit to aspnetcore.components.circuits - rename circuits.active_circuits to circuit.active and connected_circuits to circuit.connected - rename aspnetcore.components.event.duration to aspnetcore.components.event_handler and include exceptions - rename aspnetcore.components.update_parameters.duration to aspnetcore.components.update_parameters and include exceptions - rename aspnetcore.components.rendering.batch.duration to aspnetcore.components.render_diff and include exceptions
@lmolkova I pushed changes based on your feedback. The remaining issues are
I will discuss it with @javiercn and come to some conclusion. |
@@ -369,7 +369,7 @@ private async Task CaptureAsyncExceptions(Task task) | |||
} | |||
} | |||
|
|||
private static partial class Log | |||
private static new partial class Log |
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.
Is this new
really needed here?
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's a warning now after I added the project reference.
It is hiding the parent logger.
A) If we want to use the parent logger, I can delete this one.
B) or I can #pragma the warning
// do not register IConfigureOptions<StartupValidatorOptions> multiple times | ||
if (!IsMeterFactoryRegistered(services)) | ||
{ | ||
services.AddMetrics(); | ||
} |
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.
Is this check to avoid calling services.AddMetrics()
more than once? Is there a failure in that situation?
@@ -105,7 +108,7 @@ public CircuitHost( | |||
|
|||
// InitializeAsync is used in a fire-and-forget context, so it's responsible for its own | |||
// error handling. | |||
public Task InitializeAsync(ProtectedPrerenderComponentApplicationStore store, CancellationToken cancellationToken) | |||
public Task InitializeAsync(ProtectedPrerenderComponentApplicationStore store, ActivityContext httpContext, CancellationToken cancellationToken) |
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.
Can we name this activityContext? httpContext
has a very well-defined meaning across the codebase and it set the expectation that it's an HttpContext
instance, which this is not.
@@ -51,6 +52,7 @@ public CircuitHost( | |||
RemoteNavigationManager navigationManager, | |||
CircuitHandler[] circuitHandlers, | |||
CircuitMetrics? circuitMetrics, | |||
ComponentsActivitySource? componentsActivitySource, |
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.
The activity source and the metrics shouldn't be nullable, they should be the same as RemoteJsRuntime and other parameters.
We unconditionally register them https://github.com/dotnet/aspnetcore/pull/61609/files#diff-e82512017622b574554e1a1519f2bb3bbb1d8957e5346b3a4cbeb0f10e368c79R64 and
We should throw an exception if they aren't being provided similarly to how we do for other services.
public static ActivityContext CaptureHttpContext() | ||
{ | ||
var parentActivity = Activity.Current; | ||
if (parentActivity is not null && parentActivity.OperationName == "Microsoft.AspNetCore.Hosting.HttpRequestIn" && parentActivity.Recorded) | ||
{ | ||
return parentActivity.Context; | ||
} | ||
return default; | ||
} |
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.
Layering wise this shouldn't go here.
Microsoft.AspNetCore.Components
shoudn't have a dependency on Microsoft.AspNetCore.Hosting
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.
Please be more specific, what do you want to see instead ?
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.
Are you trying to get the activity of the current request here? There is a feature in the features collection for doing that: https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.features.ihttpactivityfeature.activity?view=aspnetcore-9.0
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.
Can I get that from DI without passing HttpContext thru all SignalR layers ? Or from some TLS ?
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 comes from HttpContext. It's in the features collection.
public Activity? StartCircuitActivity(string circuitId, ActivityContext httpContext) | ||
{ | ||
_circuitId = circuitId; | ||
|
||
var activity = ActivitySource.CreateActivity(OnCircuitName, ActivityKind.Internal, parentId: null, null, null); | ||
if (activity is not null) | ||
{ | ||
if (activity.IsAllDataRequested) | ||
{ | ||
if (_circuitId != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.circuit.id", _circuitId); | ||
} | ||
if (httpContext != default) | ||
{ | ||
activity.AddLink(new ActivityLink(httpContext)); | ||
} | ||
} | ||
activity.DisplayName = $"Circuit {circuitId ?? ""}"; | ||
activity.Start(); | ||
_circuitContext = activity.Context; | ||
} | ||
return activity; | ||
} | ||
|
||
public void FailCircuitActivity(Activity? activity, Exception ex) | ||
{ | ||
_circuitContext = default; | ||
if (activity != null && !activity.IsStopped) | ||
{ | ||
activity.SetTag("error.type", ex.GetType().FullName); | ||
activity.SetStatus(ActivityStatusCode.Error); | ||
activity.Stop(); | ||
} | ||
} | ||
|
||
public Activity? StartRouteActivity(string componentType, string route) | ||
{ | ||
if (_httpContext == default) | ||
{ | ||
_httpContext = CaptureHttpContext(); | ||
} | ||
|
||
var activity = ActivitySource.CreateActivity(OnRouteName, ActivityKind.Internal, parentId: null, null, null); | ||
if (activity is not null) | ||
{ | ||
if (activity.IsAllDataRequested) | ||
{ | ||
if (_circuitId != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.circuit.id", _circuitId); | ||
} | ||
if (componentType != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.type", componentType); | ||
} | ||
if (route != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.route", route); | ||
} | ||
if (_httpContext != default) | ||
{ | ||
activity.AddLink(new ActivityLink(_httpContext)); | ||
} | ||
if (_circuitContext != default) | ||
{ | ||
activity.AddLink(new ActivityLink(_circuitContext)); | ||
} | ||
} | ||
|
||
activity.DisplayName = $"Route {route ?? "[unknown path]"} -> {componentType ?? "[unknown component]"}"; | ||
activity.Start(); | ||
_routeContext = activity.Context; | ||
} | ||
return activity; | ||
} |
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 should go on the Server assembly.
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.
If we move it to different class/instance, we would not be able to use _circuitId
and _circuitContext
. And we would have to create lot of semi-public interfaces to pass that info between those classes. Are you sure it's worth it ?
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.
Isn't the CircuitId on a parent activity? (like start circuit or something like that)
All the _httpContext
, _circuitContext
on that event is at the wrong level of abstraction. I don't know if it can be set on the parent activity and flowed into this one, but the route activity should not know about _circuitContext or _httpContext.
To put it in a different way, this method/class should not be responsible for resolving the parent activity to link to. That should be provided somehow. We might need public API for that.
We don't want to keep adding to this list when/if we add new render modes. For example, I imagine WebView migth also want to track its own parent activity and it would require us to add it here.
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.
Let's address this in next PR.
I agree that we should not break existing expectations about layers. On the other hand, I would like to keep this as simple as possible. This is internal class and I don't prefer lofty layered architectures for simple and perf sensitive classes like this one.
public Activity? StartEventActivity(string? componentType, string? methodName, string? attributeName) | ||
{ | ||
var activity = ActivitySource.CreateActivity(OnEventName, ActivityKind.Internal, parentId: null, null, null); | ||
if (activity is not null) | ||
{ | ||
if (activity.IsAllDataRequested) | ||
{ | ||
if (_circuitId != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.circuit.id", _circuitId); | ||
} | ||
if (componentType != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.type", componentType); | ||
} | ||
if (methodName != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.method", methodName); | ||
} | ||
if (attributeName != null) | ||
{ | ||
activity.SetTag("aspnetcore.components.attribute.name", attributeName); | ||
} | ||
if (_httpContext != default) | ||
{ | ||
activity.AddLink(new ActivityLink(_httpContext)); | ||
} | ||
if (_circuitContext != default) | ||
{ | ||
activity.AddLink(new ActivityLink(_circuitContext)); | ||
} | ||
if (_routeContext != default) | ||
{ | ||
activity.AddLink(new ActivityLink(_routeContext)); | ||
} | ||
} | ||
|
||
activity.DisplayName = $"Event {attributeName ?? "[unknown attribute]"} -> {componentType ?? "[unknown component]"}.{methodName ?? "[unknown method]"}"; | ||
activity.Start(); | ||
} | ||
return activity; | ||
} | ||
|
||
public static void FailEventActivity(Activity? activity, Exception ex) | ||
{ | ||
if (activity != null && !activity.IsStopped) | ||
{ | ||
activity.SetTag("error.type", ex.GetType().FullName); | ||
activity.SetStatus(ActivityStatusCode.Error); | ||
activity.Stop(); | ||
} | ||
} | ||
|
||
public static async Task CaptureEventStopAsync(Task task, Activity? activity) | ||
{ | ||
try | ||
{ | ||
await task; | ||
activity?.Stop(); | ||
} | ||
catch (Exception ex) | ||
{ | ||
FailEventActivity(activity, ex); | ||
} | ||
} | ||
} |
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 probably needs to go into a different class ComponentEventActivitySource
(or any other name, RendererEventMetrics or something like that)
@@ -770,6 +813,7 @@ private void ProcessRenderQueue() | |||
|
|||
_isBatchInProgress = true; | |||
var updateDisplayTask = Task.CompletedTask; | |||
var batchStartTimestamp = ComponentMetrics != null && ComponentMetrics.IsBatchEnabled ? Stopwatch.GetTimestamp() : 0; |
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.
Don't remember the exact syntax, but you can write it like below
var batchStartTimestamp = ComponentMetrics != null && ComponentMetrics.IsBatchEnabled ? Stopwatch.GetTimestamp() : 0; | |
var batchStartTimestamp = ComponentMetrics is { IsBatchEnabled: true } ? Stopwatch.GetTimestamp() : 0; |
|
||
if (_renderer.ComponentMetrics != null && _renderer.ComponentMetrics.IsParametersEnabled) | ||
{ | ||
_componentTypeName = component.GetType().FullName; |
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.
Is this perf related or "convenience" related.
Adding a field to ComponentState is not great, since this increments the cost of each component that gets created (which your app can have thousands of).
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 for perf. Because we need that type _componentTypeName on all parameter updates on each component instance. (when this metric is enabled)
I will merge this so that we could demo the feature soon. There are few pending issues, which I will address on next PR. |
Blazor metrics
new meter
Microsoft.AspNetCore.Components
aspnetcore.components.navigation
- Total number of route changes.aspnetcore.components.event_handler
- Duration of processing browser event including business logic.new meter
Microsoft.AspNetCore.Components.Lifecycle
aspnetcore.components.update_parameters
- Duration of processing component parameters including business logic.aspnetcore.components.render_batch
- Duration of rendering batch including browser round-trip.meter
Microsoft.AspNetCore.Components.Server.Circuits
aspnetcore.components.circuit.active
- Number of active circuits in memory.aspnetcore.components.circuit.connected
- Number of circuits connected to client.aspnetcore.components.circuit.duration
- Duration of circuit lifetime and their total count.Blazor activity tracing
Microsoft.AspNetCore.Components
Microsoft.AspNetCore.Components.CircuitStart
:Circuit {circuitId}
aspnetcore.components.circuit.id
Microsoft.AspNetCore.Components.RouteChange
:Route {route} -> {componentType}
aspnetcore.components.circuit.id
,aspnetcore.components.type
,aspnetcore.components.route
Microsoft.AspNetCore.Components.HandleEvent
:Event {attributeName} -> {componentType}.{methodName}
aspnetcore.components.circuit.id
,aspnetcore.components.type
,aspnetcore.components.method
,aspnetcore.components.attribute.name
,error.type
Feedback
IMeterFactory
to be available in DITODO - Metrics need to be documented at https://learn.microsoft.com/en-us/aspnet/core/log-mon/metrics/built-in
Out of scope
Contributes to #53613
Contributes to #29846
Feedback for #61516
Related open-telemetry/semantic-conventions#2235