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

Adding more information and events to System.Net.Http telemetry #83734

Closed
MihaZupan opened this issue Mar 21, 2023 · 4 comments
Closed

Adding more information and events to System.Net.Http telemetry #83734

MihaZupan opened this issue Mar 21, 2023 · 4 comments
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Mar 21, 2023

While we include a decent amount of information about a request in the RequestStart event, we provide almost no information about what happened with the response (outside of timestamps/latency info for various steps during a request).

void RequestStart(string scheme, string host, int port, string pathAndQuery, byte versionMajor, byte versionMinor, HttpVersionPolicy versionPolicy);
void RequestFailed();

We've heard from customers moving from .NET Framework where they could get information like the response status code from the System.Diagnostics.Eventing.FrameworkEventSource provider, e.g.:

GetResponseStart
id: 9223372032565852628
uri: https://httpbin.org/ip
success: True
synchronous: False

GetResponseStop
id: 9223372032565852628
success: True
synchronous: False
statusCode: 200

I propose we include more information on the following (existing) System.Net.Http events:

  • Status code on RequestStop.
    -void RequestStop()
    +void RequestStop(int statusCode)
  • Status code on ResponseHeadersStop. ResponseHeadersStop in addition to RequestStop because we may not have a status code available in RequestStop in failure cases, and because a single RequestStart may span multiple actual requests internally due to redirects (or custom user handler logic such as retries). Having a status code on the RequestStop is still valuable for users that just care about the result of the HttpClient call, without diving into details of what happened as part of that call. See 003e675#r103790748 for more discussion about this.
    -void ResponseHeadersStop()
    +void ResponseHeadersStop(int statusCode)
  • Exception message & stack trace on RequestFailed.
    -void RequestFailed();
    +void RequestFailed(string exception);
    If we decide on an error code system as part of [API Proposal]: Support error codes for HttpRequestException #76644, we could include that information as a second parameter, or we could rely on the error code becoming a part of the HttpRequestException's message. @antonfirsov is including the HttpRequestError in the message string something you've considered?

The remaining set of potential improvements is now tracked by #85729.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 21, 2023
@ghost
Copy link

ghost commented Mar 21, 2023

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

Issue Details

While we include a decent amount of information about a request in the RequestStart event, we provide almost no information about what happened with the response (outside of timestamps/latency info for various steps during a request).

void RequestStart(string scheme, string host, int port, string pathAndQuery, byte versionMajor, byte versionMinor, HttpVersionPolicy versionPolicy);
void RequestFailed();

We've heard from customers moving from .NET Framework where they could get information like the response status code from the System.Diagnostics.Eventing.FrameworkEventSource provider, e.g.:

GetResponseStart
id: 9223372032565852628
uri: https://httpbin.org/ip
success: True
synchronous: False

GetResponseStop
id: 9223372032565852628
success: True
synchronous: False
statusCode: 200

I propose we include more information on the following (existing) System.Net.Http events:

  • Status code on ResponseHeadersStop. ResponseHeadersStop instead of RequestStop as we may not have a status code in the failure case, and because a single RequestStart may span multiple actual requests internally due to redirects.
    -public void ResponseHeadersStop()
    +public void ResponseHeadersStop(int statusCode)
  • Exception message & stack trace on RequestFailed.
    -public void RequestFailed();
    +public void RequestFailed(string exception);
    If we decide on an error code system as part of [API Proposal]: Support error codes for HttpRequestException #76644, we could include that information as a second parameter, or we could rely on the error code becoming a part of the HttpRequestException's message. @antonfirsov is including the HttpRequestError in the message string something you've considered?
  • Http version on RequestHeadersStart.
    While we log the request version in RequestStart, there is no reliable way to get the protocol version that was actually used as a result of ALPN/version policy from events (in some cases it is implied if you see other events like RequestLeftQueue).
    -public void RequestHeadersStart();
    +public void RequestHeadersStart(byte versionMajor, byte versionMinor);
Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@antonfirsov
Copy link
Member

@antonfirsov is including the HttpRequestError in the message string something you've considered?

It makes sense for me personally, but not sure if it's the preferred practice in .NET. Eg. SocketException.Message does not contain the SocketErrorCode, only the verbose error description. It might be better to include HttpRequestError directly in the telemetry if/when the error proposal will be implemented.

@karelz karelz added this to the 8.0.0 milestone Mar 21, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2023
@karelz karelz added the enhancement Product code improvement that does NOT require public API changes/additions label Mar 21, 2023
@MihaZupan MihaZupan changed the title Add error & status code info to System.Net.Http events Adding more information and events to System.Net.Http telemetry Mar 23, 2023
@MihaZupan
Copy link
Member Author

We've added the following information to the System.Net.Http EventSource in 8.0 (#84036):

The following parameters to existing events:

-void RequestStop()
+void RequestStop(int statusCode)

-void ResponseHeadersStop()
+void ResponseHeadersStop(int statusCode)

-void RequestFailed();
+void RequestFailed(string exceptionMessage)

and a new event under a dedicated keyword

[Event(15, Level = EventLevel.Error, Keywords = Keywords.RequestFailedDetailed)]
void RequestFailedDetailed(string exception);

We've also backported adding the statusCode and exceptionMessage parameters to 6.0.18 (#84807) and 7.0.7 (#84806).

@karelz
Copy link
Member

karelz commented May 27, 2023

Fixed in main (8.0) in PR #84036
Partially fixed in 6.0.18 in PR #84807 and 7.0.7 in PR #84806 (see #83734 (comment) for details).

@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 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

No branches or pull requests

3 participants