Skip to content
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

Propagate trace parent to SignalR hub invocations #57049

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 28, 2024

Addresses #51557. Specifically, the Distributed tracing (trace ids sent between client and server) item.

Follow up to #55439. The previous PR added activities for signalr server invocations. However, the server hub invocation activity never has a parent, so the activity always starts a new trace. This PR updates signalr to get trace information from the hub invocation message headers, which are set by the client, when it creates the activity.

The benefit of this change is you can now see the context of the hub invocation. For example, if web api sends a hub invocation to SignalR, the hub invocation span is nested beneath the web api span.

Changes:

  • SignalR server changed to not clear the request activity when a connection starts. I believe this is an improvement because we want most telemetry on the connection (e.g. logging and OnConnected and OnDisconnected activities) to be correlated to the request activity.
  • SignalR server changed to clear and restore the request activity when invocations are run. These are the pieces of telemetry we don't want to be a child of the request activity.
  • .NET SignalR client sets tracing headers on hub invocation request if there is an activity. Headers are sent with the invocation to the server.
  • SignalR server reads trace headers and creates an activity from the remote parent. Shares logic with ASP.NET Core for creating activity from remote.

Not planned in this PR:

  • Creating an activity for sending the invocation in .NET client.
  • Sending tracing headers from other clients.

Nothing is blocking these from being added in the future.

TODO: Write tests for client. Write integration test for client to server.

@JamesNK JamesNK added the area-signalr Includes: SignalR clients and servers label Jul 28, 2024
@JamesNK JamesNK requested a review from BrennanConroy as a code owner July 28, 2024 23:55
@JamesNK JamesNK requested review from noahfalk and tarekgh July 29, 2024 00:10
@JamesNK
Copy link
Member Author

JamesNK commented Jul 29, 2024

@noahfalk @tarekgh It would be great if you could double check the mechanics of sending/receiving trace headers and activity usage are correct.

/// <summary>
/// Create an activity with details received from a remote source.
/// </summary>
public static Activity? CreateFromRemote(
Copy link
Member Author

Choose a reason for hiding this comment

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

This centralizes and shares ASP.NET Core's code for created an activity from a remote source. Now used by ASP.NET Core hosting and SignalR.

@JamesNK JamesNK force-pushed the jamesnk/signalr-distributed-tracing branch from 4c2649e to cfc30b1 Compare July 29, 2024 10:07
@JamesNK
Copy link
Member Author

JamesNK commented Jul 29, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

I was kind of expecting the Javascript client to be done as well. It's the most used client (.NET is second) and we generally like to add new features to both JS and .NET to have some verification that the concept works across languages.

@JamesNK
Copy link
Member Author

JamesNK commented Jul 30, 2024

I was kind of expecting the Javascript client to be done as well. It's the most used client (.NET is second) and we generally like to add new features to both JS and .NET to have some verification that the concept works across languages.

I've never done OTEL from JavaScript before. I don't know if I'd have time to do it for .NET 9. What would you like to do?

Note, there will probably be some dependencies required for working with OTEL in JavaScript. In .NET we have APIs built into runtime for this stuff. JavaScript requires packages, e.g. https://www.npmjs.com/package/@opentelemetry/core for setting propagated headers. And a higher level OTEL packages for setting a client span if that's also in the JavaScript client.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM as well.

@tarekgh - in a future release we might want to offer an improved overload of ActivitySource.CreateActivity so that callers don't need these workarounds. It looks like one extra parameter to specify HasRemoteParent and a 2nd to allow creating an activity even when there is no listener would probably reduce the calling code by ~75 lines.

@JamesNK
Copy link
Member Author

JamesNK commented Jul 31, 2024

@BrennanConroy I have a PR for adding a client activity source and activities in #57101. That PR uses this one as its target branch.

Please take some time to see if there are any outstanding changes required or whether it is ok to merge. That will allow the client activity source PR to be retargeted to main.

@JamesNK JamesNK force-pushed the jamesnk/signalr-distributed-tracing branch from eff6c7e to 2f4adcc Compare August 2, 2024 01:22
@JamesNK JamesNK enabled auto-merge (squash) August 2, 2024 01:31
@JamesNK JamesNK merged commit 1d88c6c into main Aug 2, 2024
26 checks passed
@JamesNK JamesNK deleted the jamesnk/signalr-distributed-tracing branch August 2, 2024 07:14
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-rc1 milestone Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants