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

[API Proposal]: HttpClient/HttpMessageInvoker metrics #84978

Closed
JamesNK opened this issue Apr 18, 2023 · 19 comments · Fixed by #88893
Closed

[API Proposal]: HttpClient/HttpMessageInvoker metrics #84978

JamesNK opened this issue Apr 18, 2023 · 19 comments · Fixed by #88893
Assignees
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Apr 18, 2023

Background and motivation

System.Net.Http has event counters. In .NET 8 we want to add metrics counters. These will sit side-by-side with event counters for backward compatibility.

Metrics counters add new features (histograms, tags) that allow for richer data to be represented by fewer counters. For example, there are event counters in hosting for total-requests and failed-requests counters. One metrics counter can represent these with a tag to represent the status.

API Proposal

The proposal for this issue covers the meter name, counters name and description and tags. The IDs and names are a public API that can't easily change after shipping.

System.Net.Http

Notes: current-requests and request-duration follow OTel's lead: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-client

http-client-current-requests

Name Instrument Type Unit Description
http-client-current-requests UpDownCounter {request} Number of outbound HTTP requests that are currently active on the client.
Attribute Type Description Examples Presence
method string HTTP request method. GET; POST; HEAD Always
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always

http-client-request-duration

Name Instrument Type Unit Description
http-client-request-duration Histogram s The duration of outbound HTTP requests.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
method string HTTP request method. GET; POST; HEAD Always
status-code int HTTP response status code. 200 Always
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
Custom tags n/a Custom tags added from TBD. organization=contoso n/a

http-client-failed-requests

Name Instrument Type Unit Description
http-client-failed-requests Counter {request} Number of outbound HTTP requests that have failed.
Attribute Type Description Examples Presence
method string HTTP request method. GET; POST; HEAD Always
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always
exception-name string Name of the .NET exception thrown during the request. System.OperationCanceledException If unhandled exception

http-client-current-connections

Name Instrument Type Unit Description
http-client-current-connections UpDownCounter {connection} Number of outbound HTTP connections that are currently active on the client.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always

http-client-current-idle-connections

Name Instrument Type Unit Description
http-client-current-idle-connections UpDownCounter {connection} Number of outbound HTTP connections that are currently idle on the client.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always

http-client-connection-duration

Name Instrument Type Unit Description
http-client-connection-duration Histogram s The duration of outbound HTTP connections.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always
reason string Reason why the connection ended. ??? Always

API Usage

Metrics are collected using libraries, tooling or MeterListener

https://learn.microsoft.com/en-us/dotnet/core/diagnostics/metrics-collection

Alternative Designs

No response

Risks

No response

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http labels Apr 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 18, 2023
@ghost
Copy link

ghost commented Apr 18, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

System.Net.Http has event counters. In .NET 8 we want to add metrics counters. These will sit side-by-side with event counters for backward compatibility.

Metrics counters add new features (histograms, tags) that allow for richer data to be represented by fewer counters. For example, there are event counters in hosting for total-requests and failed-requests counters. One metrics counter can represent these with a tag to represent the status.

API Proposal

The proposal for this issue covers the meter name, counters name and description and tags. The IDs and names are a public API that can't easily change after shipping.

System.Net.Http

Notes: current-requests and request-duration follow OTel's lead: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-client

current-requests

Name Instrument Type Unit Description
current-requests UpDownCounter {request} Number of outbound HTTP requests that are currently active on the client.
Attribute Type Description Examples Presence
method string HTTP request method. GET; POST; HEAD Always
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always

request-duration

Name Instrument Type Unit Description
request-duration Histogram s The duration of outbound HTTP requests.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
method string HTTP request method. GET; POST; HEAD Always
status-code int HTTP response status code. 200 Always
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
Custom tags n/a Custom tags added from TBD. organization=contoso n/a

failed-requests

Name Instrument Type Unit Description
failed-requests Counter {request} Number of outbound HTTP requests that have failed.
Attribute Type Description Examples Presence
method string HTTP request method. GET; POST; HEAD Always
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always
exception-name string Name of the .NET exception thrown during the request. System.OperationCanceledException If unhandled exception

current-connections

Name Instrument Type Unit Description
current-connections UpDownCounter {connection} Number of outbound HTTP connections that are currently active on the client.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always

current-idle-connections

Name Instrument Type Unit Description
current-idle-connections UpDownCounter {connection} Number of outbound HTTP connections that are currently idle on the client.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always

connection-duration

Name Instrument Type Unit Description
connection-duration Histogram s The duration of outbound HTTP connections.
Attribute Type Description Examples Presence
scheme string The URI scheme identifying the used protocol. http; https Always
host string Host identifier of the "URI origin" HTTP request is sent to. localhost Always
port int Port identifier of the "URI origin" HTTP request is sent to. 8080 Added if not default (80 for http or 443 for https)
protocol string HTTP request protocol. HTTP/1.1; HTTP/2; HTTP/3 Always
reason string Reason why the connection ended. ??? Always

API Usage

Metrics are collected using libraries, tooling or MeterListener

https://learn.microsoft.com/en-us/dotnet/core/diagnostics/metrics-collection

Alternative Designs

No response

Risks

No response

Author: JamesNK
Assignees: -
Labels:

api-suggestion, area-System.Net.Http

Milestone: -

@JamesNK
Copy link
Member Author

JamesNK commented Apr 18, 2023

@JamesNK
Copy link
Member Author

JamesNK commented Apr 18, 2023

For reference, here are the current event counters for System.Net.Http - https://learn.microsoft.com/en-us/dotnet/core/diagnostics/available-counters#systemnethttp-counters

There probably should be another couple of metrics counters added to this issue for current queued requests and queue duration. I'm not familiar with the inner workings for client pooling and queuing in System.Net.Client.

@JamesNK
Copy link
Member Author

JamesNK commented Apr 18, 2023

Issue that discusses desired metrics for the HTTP connection pool: Additional metrics for .NET Core and ASP.NET Core #79459

@stephentoub
Copy link
Member

These will sit side-by-side with event counters for backward compatibility.

At what point will we be comfortable dropping the old ones?

request-duration

I believe the duration we currently track is about the lifetime of the HttpClient's SendAsync call. That means it includes the receiving of the response content if HttpCompletionOptions.ResponseContentRead is specified (the default for most APIs), but if HttpCompletionOptions.ResponseHeadersRead is used, the duration would end prior to processing the response body. What is the intent here?

current-connections vs current-idle-connections

If a single metric for current-connections can be used to differentiate based on protocol (such that we don't need separate ones for http1/http2/etc.), why do current-connections and current-idle-connections each need their own rather than it just being a boolean tag on the single current-connections metric?

@JamesNK
Copy link
Member Author

JamesNK commented Apr 18, 2023

At what point will we be comfortable dropping the old ones?

Dropping event counters will be tough because there isn't a way to indicate they're obsolete (@noahfalk correct me if I'm wrong). Tooling will happily work up to the point where they're removed and then suddenly break.

but if HttpCompletionOptions.ResponseHeadersRead is used, the duration would end prior to processing the response body

IMO it's bad behavior. I'd like the Activity (aka span) and request duration to continue until the end of the response body. One way to ensure .NET HTTP client telemetry lines up with the general community would be to check how OpenTelemetry measures request time.

why do current-connections and current-idle-connections each need their own rather than it just being a boolean tag on the single current-connections metric?

The "current-xxx" counters are UpDownCounter<long>. They are increased by 1 when a thing starts and decreased by 1 when the thing ends. The start and end operations must have the same tags. A tag to flag a connection as idle doesn't fit because we don't know a connection is going to end as idle when it starts.

Instead, you need something like:

  • Connection starts. Increase current-connections by 1.
  • Connection becomes idle. Increase current-idle-connections by 1.
  • Connection ends. Decrease current-idle-connections and current-connections each by 1.

Now that I've described it like that, the description for current-connections should probably make it clear that it's a total count of active and idle connections.

@antonfirsov antonfirsov removed the untriaged New issue has not been triaged by the area owner label Apr 18, 2023
@antonfirsov antonfirsov added this to the 8.0.0 milestone Apr 18, 2023
@noahfalk
Copy link
Member

Dropping event counters will be tough because there isn't a way to indicate they're obsolete (@noahfalk correct me if I'm wrong). Tooling will happily work up to the point where they're removed and then suddenly break.

Correct, there is no programmatic obsoletion mechanic. I think we would treat their removal similar to a behavioral breaking change in an API. My hope is that leaving the existing EventCounters in place for a long time imposes very little burden on us. Any energy for fixes/improvements can be directed at the Meter scenario instead. Most tools either already support Meters or will do so in the near future.

@davidfowl
Copy link
Member

Now that we're using both, hopefully we're forced to come up with a way to deprecate the event counters. @noahfalk it would be good to write up a similar doc above for the runtime counters (I know they are much harder to implement) 😄 .

@davidfowl
Copy link
Member

davidfowl commented Apr 19, 2023

I just realized something @JamesNK, for HttpClient there can be many instances in the application. What's the thought process around how we handle this with meters? I think these need to be "nameable" from the outside so that other consumers can determine the meter name for their APIs.

@JamesNK
Copy link
Member Author

JamesNK commented Apr 19, 2023

There is an internal email thread discussing ideas for customizing the meter on HttpClient/HttpMessageInvoker. I haven't created an issue for it yet.

One idea is a simple HttpClient.Meter property that must be set before the client is first used. That meter is used to create the counters for the HttpClient. In the DI layer, HttpClientFactory can set the Meter property with a meter created by Microsoft.Extensions.Metrics. That would make it easy to unit-test all the clients associated with that DI container.

I don't think the meters for different HttpClient instances would have different names, but an idea I've discussed with Noah is the ability to support default tags on a meter. Whenever a meter's counters record values, the default tags for the meter are automatically added.

The source of this idea was a way for ASP.NET Core to identify metrics when there are multiple hosts in one process. But it also works well for identifying metrics results from individual HttpClient instances.

Update: An issue was created for it: Support for adding default tags to specific meter and/or instruments #84321

@davidfowl
Copy link
Member

davidfowl commented Apr 19, 2023

I don't think the meters for different HttpClient instances would have different names, but an idea I've discussed with Noah is the ability to support default tags on a meter. Whenever a meter's counters record values, the default tags for the meter are automatically added.

Really? I feel like it should default to adding a host dimension to all counters. At least then you could identify the target of the information. That might be enough to cover most basic cases no? (though not bullet proof).

@JamesNK
Copy link
Member Author

JamesNK commented Apr 19, 2023

All the counters above have a host tag.

@davidfowl
Copy link
Member

Could that mitigate some of the problem?

@samsp-msft
Copy link
Member

Now that we're using both, hopefully we're forced to come up with a way to deprecate the event counters. @noahfalk it would be good to write up a similar doc above for the runtime counters (I know they are much harder to implement) 😄 .

Maybe we should move the counters to another library(s) that can be optionally referenced - its the name that is used rather than a binary interface for the class right? The counter library would in-turn will listen to the metrics and fire the corresponding counters for them.

@samsp-msft
Copy link
Member

Could that mitigate some of the problem?

No - you could have multiple HttpClient instances for the same host - it all depends on how new happy the code is using HttpClient.

@samsp-msft
Copy link
Member

AFAICR there is a counter for completed requests in ASP.NET Core, should there be one for HttpClient so you have a count of the number of requests that have been made? If we do that, should there be a RequestsFailed and RequestsCompleted, or a RequestsCompleted that includes the response code, and error message, replacing RequestsFailed?

@samsp-msft
Copy link
Member

We don't currently provide information about connections and connection duration - for h2/h3 scenarios. Would it make sense to have a histogram for connection duration - the unusual thing being that longer is better?

@JamesNK
Copy link
Member Author

JamesNK commented Apr 19, 2023

A connection duration counter is already in the issue: connection-duration

@noahfalk
Copy link
Member

Now that we're using both, hopefully we're forced to come up with a way to deprecate the event counters

At this point I think the useful thing to do is update our messaging to encourage using Meters and work with tool partners to adopt the newer features (both of these things are already happening in a broad sense, not specific to metrics from any one .NET library). This should naturally migrate most common scenarios over time.

I am not a fan of doing anything more drastic at this point such as actively advocating removing EventCounters code from .NET libraries that already have it. Doing anything that aggressive feels needlessly disruptive and its not clear it offers much value. If a library owner believes maintaining EventCounters code is too much overhead it is still their perogative to force breaking changes on all their users. For a 3rd party library with low back-compat concern that might be a fine move, but for a library that cares about back-compat IMO that wouldn't be a good tradeoff at this point.

@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 4, 2023
@MihaZupan MihaZupan self-assigned this Jul 13, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants