Add information to indicate a SignalR request is long running

[shirhatti]

To allow APMs (e.g., Application Insights) to treat long running SignalR requests differently from the their request statistics, we should set some information (I presume via an Activity.Tag?) on current Activity to indicate a request is long running.

Edit: After more discussion with App Insights, we think headers will work.

cc @cijothomas

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[halter73]

We're also considering using response headers (and possibly request headers if we want to wait to flush response headers for long polling or something) to indicate that a request is expected to be long-running. An upside to using a header instead of (or maybe in addition to) adding info to the current Activity is that it could also be used by reverse proxies.

Long-Running: true

[shirhatti]

My current proposal is that we don't take the current PR to add a response header. Given that Application Insights didn't find it useful and I know of nobody else clamoring for it, I don't think it's worth it.

I have two proposals on how to proceed here listed in the order of my preference:

Close the span prematurely

For example if /websocket is an endpoint that results in a connection upgrade, I'm now considering the HTTP request completed after the response headers indicating the upgrade have been flushed. Any subsequent data sent over the websocket wouldn't be part of the duration of the initial span (Activity) created for the HTTP request.

When SignalR now uses the established WebSocket as a channel, it can create a new Span for each RPC invocation (with possibly a Span link to the original associated with the incoming HTTP request). While this bit this optional for now, this is how envision it working if we went all the way and made SignalR span-aware.

endpoints.MapGet("/", async context =>
{
    using var webSocket = await context.WebSockets.AcceptWebSocketAsync();
    context.Features.Get<IActivityFeature>().Activity.Stop();
    await webSocket.ReceiveAsync(...);
    // do more stuff
});

For non-websocket transports, close the span whenever the determination is made that this request will be long-running.

While this helps ActivityListener based instrumentation, Application Insights may not benefit. While they will see shortened (in duration) spans, they don't subscribe to the ActivityListener based Activity stopped callback. They'll only see it when the DiagnosticsListener callback is fired when the long-lived WebSocket is closed/dies.

Add a new attribute (Activity.AddTag())

Invent a new attribute, not present in the HTTP Semantic conventions.
This may just result in crud since:

• No APM vendor will ever elect to use this; OR
• The HTTP semantic convention decides to actually address this and create a new attribute at which point we'll be stuck emitting both

endpoints.MapGet("/", async context =>
{
    using var webSocket = await context.WebSockets.AcceptWebSocketAsync();
    context.Features.Get<IActivityFeature>().Activity.AddTag("http.long_running", true);
    await webSocket.ReceiveAsync(...);
    // do more stuff
});

NOTE: The Semantic convention specification does define attributes for RPC, but these are for a spans created per-RPC invocation. There is nothing defined for the span associated with creating the channel used by the RPC framework.

[shirhatti]

@noahfalk Do you know the GitHub aliases of other APM vendors? Can we tag them on here for feedback?

[noahfalk]

@davmason would know the best : )

[davmason]

Off the top of my head @pjanotti @macrogreg @zacharycmontoya @discostu105 @d-schneider are all people I engage with frequently. It would make sense to add a post at dotnet/runtime#9305 too if you want more APM vendors to see it.

[shirhatti]

@halter73, @BrennanConroy, @davidfowl, and I met to discuss this.

We've decided:

• Not to emit a response header
• Not to close the span prematurely. It leads to loss of fidelity in data. Though we agreed that in context of a RPC, the request that establishes the underlying channel for RPCs isn't the causal parent, there's no need to nerf it. If and when we have per-RPC call instrumentation, we'll change the causal parent for such Activities to the remote parent.
• We'll be adding a new Activity Tag (i.e., span attribute) to the Activity to indicate that the request is long running. Given that the OpenTelemetry semantic conventions don't have a defined tag, we'll be inventing a new tag (we welcome feedback here!)
• We'll add documentation/samples showcasing how to write a OpenTelemetry filtering processor to drop Activities with the specified tag.

👀 @cijothomas @rajkumar-rangaraj