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

[release/7.0-staging] Add status code and exception info to System.Net.Http events #84806

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Apr 13, 2023

Backport of #84036 to 7.0.
Contributes to #83734.

Customer Impact

The networking stack is already instrumented with EventSource events that allow telemetry consumers to observe when requests are made and where they are spending time. Aside from a timestamp, these events often provide no or very few extra details.

On .NET Framework, a different EventSource provider exists that exposes information like the HTTP response status code. This provider does not exist on .NET Core+, and as a result, customers migrating their services from Framework are losing diagnostics information (e.g. XBox Live service).

This PR adds the response status code and exception message to existing events to cover that gap.

Testing

I added targeted CI tests that confirm new parameters are included in events.

Risk

Minimal.
Impacted code paths are all only reachable when telemetry is enabled.
With confirmation from @brianrob and @noahfalk, modifications to existing events in the manner done here are safe and non-breaking for existing telemetry consumers.

@MihaZupan MihaZupan added this to the 7.0.x milestone Apr 13, 2023
@MihaZupan MihaZupan self-assigned this Apr 13, 2023
@ghost
Copy link

ghost commented Apr 13, 2023

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

Issue Details

Backport of #84036 to 7.0.
Contributes to #83734.

Includes the following changes:

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

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

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

Does not include the new RequestFailedDetailed event added by #84036 as its usefulness is limited before 8.0 due to #84712.

Customer Impact

TODO

Testing

I added targeted CI tests that confirm new parameters are included in events.

Risk

TODO

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 7.0.x

@MihaZupan
Copy link
Member Author

All test failures are known according to Build Analysis.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@karelz karelz added the Servicing-consider Issue for next servicing release review label Apr 27, 2023
@MihaZupan
Copy link
Member Author

Approved by Tactics (Steve Carroll) on May 1st via email. Marking as servicing-approved.

@MihaZupan MihaZupan added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 2, 2023
@MihaZupan MihaZupan modified the milestones: 7.0.x, 7.0.6, 7.0.7 May 3, 2023
@MihaZupan MihaZupan merged commit 17d84d7 into dotnet:release/7.0-staging May 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants