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/6.0-staging] Add status code and exception info to System.Net.Http events #84807

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Apr 13, 2023

Backport of #84036 to 6.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 6.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 6.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: 6.0.x

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.

@MihaZupan MihaZupan 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
Copy link
Member Author

@carlossanlop we are free to merge these now, right?

@carlossanlop
Copy link
Member

carlossanlop commented May 3, 2023

we are free to merge these now, right?

Yes. PR owners are free to merge their backport PRs targeting the release/X.0-staging any time of the month. No need to wait for branding anymore. Just be mindful of the code complete date, since that's what determines which Release will get your fix. In the case of this PR, if you merge it now, it will go into the June Release. The code complete date is May 15th.

For future reference, you can use this checklist:

  • If affecting feature code: Approved by Tactics.
  • If Approved by Tactics, or it is a test-only or an infra-only change: The Servicing-approved label is applied, which lets the check-service-labels CI leg to pass.
  • Signed-off by area owner.
  • CI is green, or failures are investigated and determined to be unrelated to this change.
  • If the affected assemblies publish OOB packages, the required OOB package authoring changes are included in this PR.

@carlossanlop carlossanlop modified the milestones: 6.0.x, 6.0.18 May 3, 2023
@MihaZupan MihaZupan merged commit 809b82e into dotnet:release/6.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.

3 participants