Skip to content

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jun 9, 2025

Changes

  • splits Circuit related tracing into new CircuitActivitySource
  • moves the CircuitStart trace to Microsoft.AspNetCore.Components.Server.Circuits namespace
  • propagates links to other traces via ComponentsActivityLinkStore internal helper
  • removes InternalsVisibleTo and it's consequences
  • disconnects the traces from (SignalR) parent, so that they are top level in OTEL

Contributes to #62145
Contributes to #62254

interactive whole application

without signalR traces
image

side by side with SignalR
image

Links in the circuit trace
image

interactive page in non-interactive application

side by side with SignalR
image

non-interactive page, form submit

image

@pavelsavara pavelsavara added this to the 10.0-preview6 milestone Jun 9, 2025
@pavelsavara pavelsavara self-assigned this Jun 9, 2025
@pavelsavara pavelsavara added the area-blazor Includes: Blazor, Razor Components label Jun 9, 2025
@pavelsavara pavelsavara marked this pull request as ready for review June 10, 2025 12:07
@pavelsavara pavelsavara requested a review from a team as a code owner June 10, 2025 12:07
@lewing lewing requested a review from noahfalk June 10, 2025 19:15
@javiercn
Copy link
Member

non-interactive page, form submit

Not asking to change anything, but this is going to be interesting, since we aren't really going to see the method for the form handling events given that EditForm is the one registering them. It's always going to show up as ...HandleSubmit, but I don't see how we can do better

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.

Looks good, I have an additional suggestion for us to consider, but otherwise looks good.

@pavelsavara pavelsavara requested a review from javiercn June 11, 2025 20:18
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.

Looks great!

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.

Looks great!

@pavelsavara pavelsavara enabled auto-merge (squash) June 11, 2025 21:17
@noahfalk
Copy link
Member

Sorry, I know this is late, but one suggestion I missed before was to have Blazor update the DisplayName of the enclosing SignalR activity. It seems like this would avoid creating extra top-level traces while preserving the unique naming you were hoping for. For example instead of doing (conceptually):

Activity signalRActivity = Activity.Current;
Activity.Current = null;
Activity blazorActivity = ComponentActivitySource.StartActivity(...);
activity.DisplayName = $"Route {route ?? "[unknown path]"} -> {componentType ?? "[unknown component]"}";
DoBlazorWork();
activity.Stop();
Activity.Current = signalRActivity;

You could do:

Activity signalRActivity = Activity.Current;
signalRActivity.DisplayName = $"Blazor Route {route ?? "[unknown path]"} -> {componentType ?? "[unknown component]"}";
DoBlazorWork();

Thoughts?

@javiercn
Copy link
Member

@

Sorry, I know this is late, but one suggestion I missed before was to have Blazor update the DisplayName of the enclosing SignalR activity. It seems like this would avoid creating extra top-level traces while preserving the unique naming you were hoping for. For example instead of doing (conceptually):

Activity signalRActivity = Activity.Current;
Activity.Current = null;
Activity blazorActivity = ComponentActivitySource.StartActivity(...);
activity.DisplayName = $"Route {route ?? "[unknown path]"} -> {componentType ?? "[unknown component]"}";
DoBlazorWork();
activity.Stop();
Activity.Current = signalRActivity;

You could do:

Activity signalRActivity = Activity.Current;
signalRActivity.DisplayName = $"Blazor Route {route ?? "[unknown path]"} -> {componentType ?? "[unknown component]"}";
DoBlazorWork();

Thoughts?

I don't think that would actually work. The reason being that a circuit expands more than one SignalR connection (for example when a client gets disconnected and reconnects). I realize now we might not be handling that scenario entirely correcty.

@pavelsavara pavelsavara merged commit c40323a into main Jun 12, 2025
27 checks passed
@pavelsavara pavelsavara deleted the pavelsavara/diagnostics-cleanup branch June 12, 2025 07:37
@danroth27 danroth27 added the blog-candidate Consider mentioning this in the release blog post label Jul 1, 2025
Copy link
Contributor

@@pavelsavara, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

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 blog-candidate Consider mentioning this in the release blog post

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants