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 #85729

Closed
MihaZupan opened this issue May 3, 2023 · 1 comment · Fixed by #88853
Closed

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

MihaZupan opened this issue May 3, 2023 · 1 comment · Fixed by #88853
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@MihaZupan
Copy link
Member

Moving the remaining items out of #83734 to make tracking easier (the changes in the original issue were backported to 6.0 and 7.0).

We should consider including more information in the System.Net.Http events:

  • Request information on RequestHeadersStart.
    While we log a lot of request information in RequestStart, we may end up sending multiple requests on the wire (redirects, retries) as part of a single RequestStart/Stop pair, and there is currently no way to tell from events where those requests are being made.
    Also 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).
    The following is the same parameter signature as RequestStart with the exception of omitting the HttpVersionPolicy since it doesn't have meaning at this layer (we already know exactly what version we're using).
    -void RequestHeadersStart();
    +void RequestHeadersStart(string scheme, string host, int port, string pathAndQuery, byte versionMajor, byte versionMinor);
    Alternatively, the connection-level information (scheme, host, port, version) could be left out of this event, and instead be exposed in a way where the user could map connections to requests. E.g.
    -void ConnectionEstablished(byte versionMajor, byte versionMinor)
    -void ConnectionClosed(byte versionMajor, byte versionMinor)
    +void ConnectionEstablished(byte versionMajor, byte versionMinor, long connectionId, string scheme, string host, int port, string? remoteEndPoint)
    +void ConnectionClosed(byte versionMajor, byte versionMinor, long connectionId)
    +void RequestHeadersStart(long connectionId);
  • A new event indicating a redirect.
    This should include the new Uri we are going to try redirecting to. I don't have a strong opinion on including the whole Uri vs separating the scheme/host/port/path...
    +void Redirect(string redirectUri);
@MihaZupan MihaZupan added this to the Future milestone May 3, 2023
@ghost
Copy link

ghost commented May 3, 2023

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

Issue Details

Moving the remaining items out of #83734 to make tracking easier (the changes in the original issue were backported to 6.0 and 7.0).

We should consider including more information in the System.Net.Http events:

  • Request information on RequestHeadersStart.
    While we log a lot of request information in RequestStart, we may end up sending multiple requests on the wire (redirects, retries) as part of a single RequestStart/Stop pair, and there is currently no way to tell from events where those requests are being made.
    Also 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).
    The following is the same parameter signature as RequestStart with the exception of omitting the HttpVersionPolicy since it doesn't have meaning at this layer (we already know exactly what version we're using).
    -void RequestHeadersStart();
    +void RequestHeadersStart(string scheme, string host, int port, string pathAndQuery, byte versionMajor, byte versionMinor);
    Alternatively, the connection-level information (scheme, host, port, version) could be left out of this event, and instead be exposed in a way where the user could map connections to requests. E.g.
    -void ConnectionEstablished(byte versionMajor, byte versionMinor)
    -void ConnectionClosed(byte versionMajor, byte versionMinor)
    +void ConnectionEstablished(byte versionMajor, byte versionMinor, long connectionId, string scheme, string host, int port, string? remoteEndPoint)
    +void ConnectionClosed(byte versionMajor, byte versionMinor, long connectionId)
    +void RequestHeadersStart(long connectionId);
  • A new event indicating a redirect.
    This should include the new Uri we are going to try redirecting to. I don't have a strong opinion on including the whole Uri vs separating the scheme/host/port/path...
    +void Redirect(string redirectUri);
Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: Future

carlossanlop added a commit to carlossanlop/runtime that referenced this issue May 8, 2023
Adding more details to our servicing documentation, particularly around the check-servicing-labels CI leg.

Co-authored-by: Juan Hoyos <[email protected]>
carlossanlop added a commit that referenced this issue May 9, 2023
Adding more details to our servicing documentation, particularly around the check-servicing-labels CI leg.

Co-authored-by: Juan Hoyos <[email protected]>
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2023
@karelz karelz added the enhancement Product code improvement that does NOT require public API changes/additions label Jul 16, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 18, 2023
@MihaZupan MihaZupan modified the milestones: Future, 8.0.0 Jul 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 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.

2 participants