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.

Sign up for GitHub

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

Jump to bottom

Add long running activity tag for SignalR connections #32084

Merged
merged 4 commits into from
Jun 7, 2021
Merged

Add long running activity tag for SignalR connections #32084

merged 4 commits into from
Jun 7, 2021

Conversation

BrennanConroy
Copy link
Member

Fixes #29699

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Apr 23, 2021
@BrennanConroy BrennanConroy added this to the 6.0-preview5 milestone Apr 23, 2021
@davidfowl
Copy link
Member

davidfowl commented Apr 23, 2021 •
edited
Loading

Should this be moved to the websocket middleware instead of signalr's websocket transport.

@shirhatti
Copy link
Contributor

I thought we we're doing to the header in addition to setting a tag on the activity?

@shirhatti
Copy link
Contributor

@cijothomas I assume this will be sufficient for AI to exclude long-running SigalR requests from regular request statistics?

@davidfowl
Copy link
Member

I thought we we're doing to the header in addition to setting a tag on the activity?

Why?

@rajkumar-rangaraj
Copy link

rajkumar-rangaraj commented Apr 23, 2021 •
edited
Loading

@cijothomas I assume this will be sufficient for AI to exclude long-running SigalR requests from regular request statistics?

@shirhatti This will help in AI to exclude long-running requests of Signal-R.

@BrennanConroy

  • Application insights gets notified on new ASP.NET Core / SignalR request when it enters Microsoft.AspNetCore.Hosting.HostingApplicationDiagnostics.BeginRequest. At this point we create a request telemetry object to track the request.
  • When Microsoft.AspNetCore.Hosting.HostingApplicationDiagnostics.RequestEnd is invoked AI gets notified again at this point we update request telemetry with duration and send it to application insights service.

With the current change we have to create and hold the request telemetry in the heap till the RequestEnd is called and drop request telemetry object on the floor. Proposed change will help us suppress the long running requests but there is a perf issue in creating and holding the RequestTelemetry. Is it possible to add HttpContext request header in BeginRequest before start activity is called.

@shirhatti
Copy link
Contributor

Is it possible to add HttpContext request header in BeginRequest before start activity is called.

That's impossible. StartActivity is called way too early for that

@shirhatti
Copy link
Contributor

@davidfowl So that I don't have to use DiagnosticsListener, get the HttpContext and then read the response headers. If I'm an instrumentation library I just want to have all the info on the Activity

@shirhatti
Copy link
Contributor

Also @rajkumar-rangaraj ,we're adding a response header, not a request header 😄

@rajkumar-rangaraj
Copy link

rajkumar-rangaraj commented Apr 23, 2021 •
edited
Loading

Also @rajkumar-rangaraj ,we're adding a response header, not a request header 😄

Yes. I reviewed it. As I wanted an update in BeginRequest asked for change in request header.

@davidfowl
Copy link
Member

Yes. I reviewed it. As I wanted an update in BeginRequest asked for change in request header.

This isn't possible.

@halter73
Copy link
Member

Should this be moved to the websocket middleware instead of signalr's websocket transport.

I thought you might want to make the WebSocket middleware a little more raw. Instrumentation should treat all WebSocket requests as long-running to begin with.

@BrennanConroy
Copy link
Member Author

Updated

@davidfowl
Copy link
Member

nit: Fix the PR title 😄

@BrennanConroy BrennanConroy changed the title Add Long-Running response header to SignalR connections Add long running activity tag for SignalR connections Jun 7, 2021
davidfowl
davidfowl reviewed Jun 7, 2021
View reviewed changes
src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs
@@ -115,6 +115,9 @@ public async Task ExecuteNegotiateAsync(HttpContext context, HttpConnectionDispa

private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connectionDelegate, HttpConnectionDispatcherOptions options, ConnectionLogScope logScope)
{
// set a tag to allow Application Performance Management tools to differentiate long running requests for reporting purposes
Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen APM written out like this before

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know what it stood for and in 1 year I definitely wont remember.

@BrennanConroy BrennanConroy enabled auto-merge (squash) June 7, 2021 18:22
@BrennanConroy BrennanConroy merged commit f22a5c3 into main Jun 7, 2021
@BrennanConroy BrennanConroy deleted the brecon/appin branch June 7, 2021 18:45
@rajkumar-rangaraj rajkumar-rangaraj mentioned this pull request Aug 20, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl davidfowl approved these changes

@shirhatti shirhatti shirhatti approved these changes

@bradygaster bradygaster bradygaster approved these changes

Assignees
No one assigned
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
6.0-preview6
Development

Successfully merging this pull request may close these issues.

Add information to indicate a SignalR request is long running
6 participants
@BrennanConroy @davidfowl @shirhatti @rajkumar-rangaraj @halter73 @bradygaster