Add separate activities for Hub methods

[BrennanConroy]

First part of https://github.com/dotnet/aspnetcore/issues/51557

Follows conventions from https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md

Haven't added server.address yet since it looks like we'd need to copy a chunk of code from Kestrel to do it properly: https://github.com/dotnet/aspnetcore/blob/027c60168383421750f01e427e4f749d0684bc02/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelMetrics.cs#L308

---

Hub methods are separate spans:

---

Streaming has events:

[davidfowl]

@BrennanConroy can you show an end to end trace with the dashboard with connect/disconnect and multiple invocations?

[BrennanConroy]

@BrennanConroy can you show an end to end trace with the dashboard with connect/disconnect and multiple invocations?

Isn't that what the first picture in the PR description shows?

[samsp-msft]

Why are the resource names in your pictures GUIDs? That is more confusing than it should be.

[BrennanConroy]

Why are the resource names in your pictures GUIDs? That is more confusing than it should be.

I was using the preview4 Aspire dashboard docker container. Updating to preview6 cleaned it up.

[BrennanConroy]

Any other feedback?

[JamesNK]

I looked at what the ASP.NET Core activity does and it calls SetEndTime. Is this necessary or a relic of an older pattern?

https://github.com/dotnet/aspnetcore/blob/e92576946861e949597ab02ebc4215c7da8bcee5/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L518-L523

cc @noahfalk @tarekgh

[JamesNK]

Should Activity.SetStatus be called when there is an unhandled error and set an error status? I don't see that happening anywhere with the Microsoft.AspNetCore activity, but I'm guessing that OpenTelemetry handles changing the status in its final output based on listening to other ASP.NET Core error events.

We want to avoid needing extra logic in OpenTelemetry so this should probably be built into SignalR.

Edit: Also, error.type attribute should be set to Exception.GetType().FullName on unhandled exception.

[noahfalk]

I'm guessing that OpenTelemetry handles changing the status in its final output based on listening to other ASP.NET Core error events

Yep, it does. This is based on the DiagnosticSource exception notifications.

[noahfalk]

Is this necessary or a relic of an older pattern?

I think that was to preserve an invariant that the Activity Duration and EndTime would be set prior to delivering the DiagnosticListener callback. Since this code isn't doing any diagnostic listener callbacks (which is just fine) that SetEndTime() should be unnecessary. Activity.Stop() will take care of it.

[BrennanConroy]

Should Activity.SetStatus be called when there is an unhandled error and set an error status? I don't see that happening anywhere with the Microsoft.AspNetCore activity, but I'm guessing that OpenTelemetry handles changing the status in its final output based on listening to other ASP.NET Core error events.

We want to avoid needing extra logic in OpenTelemetry so this should probably be built into SignalR.

Edit: Also, error.type attribute should be set to Exception.GetType().FullName on unhandled exception.

Updated to set the Error status when an exception occurs, and to set the error.type tag.

[dotnet-policy-service[bot]]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.