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

Skip counter measurement when start listening while processing #48045

Merged
merged 9 commits into from
May 18, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented May 3, 2023

Someone could start listening to counters after something we want to measure (e.g. a connection) has started. This is a problem for some counters:

  • Counter tag enrichment feature is only added to features if counters are enabled. It would be unexpected for the feature not to be set, listening to start, and the counter to be recorded to.
  • Up/down counters, such as the current connections, must be paired together. If the connection starts without a listener, the counter isn't increased. If a listening starts, we don't want to reduce the counter by 1 because it will go negative.

Hosting already worked. It calculates counters being enabled when a connection starts.

Kestrel needed a lot of work. This PR adds a metric context that figures out what counters are enabled when a connection starts and uses those values for the life of the connection.

It's tough to flow a metrics context throughout a connection. This PR uses an internal feature that Kestrel always sets.

@JamesNK
Copy link
Member Author

JamesNK commented May 3, 2023

@halter73 Addresses your comment here: #47758 (review)

SignalR metrics is still to do.

@amcasey
Copy link
Member

amcasey commented May 5, 2023

I think the goal here is to make each operation either report metrics or not report metrics for its entire duration, is that right?

@JamesNK JamesNK force-pushed the jamesnk/metrics-listen-halfway branch from ab00e05 to 7bd1a95 Compare May 10, 2023 05:50
@JamesNK JamesNK added the area-signalr Includes: SignalR clients and servers label May 10, 2023
@JamesNK
Copy link
Member Author

JamesNK commented May 10, 2023

SignalR done.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Thanks! This seems much less prone to user error.

@JamesNK
Copy link
Member Author

JamesNK commented May 17, 2023

@halter73 @Tratcher Could you take a look when you have some time? I'd like to double check the internal feature Kestrel adds to flow the metrics context - IConnectionMetricsContextFeature - is fine.

Co-authored-by: Chris Ross <Tratcher@Outlook.com>
@JamesNK JamesNK enabled auto-merge (squash) May 18, 2023 06:41
@JamesNK JamesNK merged commit 31341cd into main May 18, 2023
@JamesNK JamesNK deleted the jamesnk/metrics-listen-halfway branch May 18, 2023 08:13
@ghost ghost added this to the 8.0-preview5 milestone May 18, 2023
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants