Skip to content

[blazor] Diagnostic tracing - feedback #62286

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 18 commits into from
Jun 12, 2025
Merged

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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants