Add status code and exception info to System.Net.Http events

[MihaZupan]

Contributes to #83734

This PR adds 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);

Copying the reasoning behind having the status code on both RequestStop and ResponseHeadersStop from #83734 (comment):

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.

@dotnet/ncl please confirm that the added parameters make sense (not just to me) -- this is akin to adding a public API in that we don't want to break it in the future (we can add more parameters, but wouldn't want to remove them for example).

[ghost]

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

Issue Details

---

Contributes to #83734

This PR adds the following parameters to existing events:

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

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

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

Copying the reasoning behind having the status code on both RequestStop and ResponseHeadersStop from #83734 (comment):

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.

@dotnet/ncl please confirm that the added parameters make sense (not just to me) -- this is akin to adding a public API.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	area-System.Net.Http
Milestone:	8.0.0

[brianrob]

A couple of questions, but overall looks good.

[danmoseley]

Not directly related to this change, but are these kinds of events already exposed in a first class way to open telemetry? I'm just wondering whether we expect we'd need work in this codebase analogous to what @JamesNK is doing in dotnet/aspnetcore#46834

[noahfalk]

Not directly related to this change, but are these kinds of events already exposed in a first class way to open telemetry? I'm just wondering whether we expect we'd need work in this codebase analogous to what @JamesNK is doing in dotnet/aspnetcore#46834

The events are exposed in the sense that OpenTelemetry instrumentation could create an EventListener and receive them if it wants to. Currently OTel's HttpClient instrumentation doesn't do that as it utilizes some DianosticSource events instead: https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Instrumentation.Http.

[MihaZupan]

@dotnet/ncl PTAL, the change is now:

• Adding int statusCode to RequestStop and ResponseHeadersStop
• Adding string exceptionMessage to RequestFailed
• Adding a new event RequestFailedDetailed(string exception) under a dedicated keyword opt-in

[ManickaP]

LGTM

[MihaZupan]

@noahfalk mind taking another look at this one if it matches what you'd expect? This is also the first time for networking telemetry that we're making use of Keywords/Version, I just followed existing patterns there w.r.t. naming.

[noahfalk]

Functionally this looks fine to me, my only remaining concern is have we validated with Jeff that this meets his needs? For example I could speculate he may not want the overhead/risk of the detailed error message with full stack trace, but perhaps he would still want the Exception type name as part of the error information?

[MihaZupan]

but perhaps he would still want the Exception type name as part of the error information?

I'll double-check offline, but with HttpClient the exception type name alone is not useful - it should be either TaskCanceledException or HttpRequestException. You can deduce whether it was the former based on the message.

[MihaZupan]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).