[Metrics] error.type doesn't report Exception.FullName

[antonfirsov]

http.client.request.duration's error.type contains Exception.Name for cases which do not map to HttpRequestError values, which is inconsistent with aspnet's implementation that reports Exception.FullName -- see dotnet/aspnetcore#51084.

The wording of the .NET Metrics spec, is somewhat ambivalent:

When error.type attribute is reported, it contains one of HTTP Request errors, a full exception type, or a string representation of received status code.

As #93254 (comment) pointed out, this could also mean to "use HttpRequestException instead of just 'Http' or 'HttpException'". Moreover, reporting the full name doesn't bring any value, the set of exceptions we can see here is limited, collisions are not expected.

I assume this is not the case with aspnet, where exceptions may come from an arbitrarily large set. My recommendation would be to leave and document the current behaviors as-is.

@JamesNK @noahfalk we should make a decision ASAP.

[ghost]

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

Issue Details

---

http.client.request.duration's error.type contains Exception.Name for cases which do not map to HttpRequestError values, which inconsistent with aspnet's implementation that reports Exception.FullName -- see dotnet/aspnetcore#51084.

The wording of the .NET Metrics spec, is somewhat ambivalent:

When error.type attribute is reported, it contains one of HTTP Request errors, a full exception type, or a string representation of received status code.

As #93254 (comment) pointed out, this could also mean to "use HttpRequestException instead of just 'Http' or 'HttpException'". Moreover, reporting the full name doesn't bring any value, the set of exceptions we can see here is limited, collisions are not expected.

I assume this is not the case with aspnet, where exceptions may come from an arbitrarily large set. My recommendation would be to leave and document the current behaviors as-is.

@JamesNK @noahfalk we should make a decision this ASAP.

Author:	antonfirsov
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[lmolkova]

No opinion here, just adding a few points:

• changing attribute value later would NOT be breaking from OTel perspective
• it might(?) be possible to get custom exceptions if users provide custom certificate check callback (maybe there are other cases?)

[noahfalk]

My preference would be to use FullName. I recognize it doesn't provide much value in terms of error disambiguation on this metric, but I do think it could provide value for consistency across metrics. If an engineer was writing queries to process metrics from different parts of .NET it would be a simpler mental model for them if the exception names reported in error.type are always FullName rather than some metrics use Name and others use FullName.

[JamesNK]

+1 for consistency.

[antonfirsov]

Ok. Gonna add this to #93254, and raise a separate PR for the HTTP part.

[MihaZupan]

Are we saying FullName is the right choice here? We could also change ASP.NET to use Name to get consistency instead :)

[antonfirsov]

@MihaZupan I considered proposing that option, but I assume ASP.NET logs FullName because they have to handle arbitrary exceptions and want to avoid ambiguity caused by name collisions. If it's not the case (unlikely), we could have a discussion whether using Name is better accross the .NET stack, and change aspnet instead, but IMO we are too late in the release cycle to carry out this discussion in an async way and implement+bacport ASP.NET changes. To avoid risking the sync-up for GA, I prefer to proceed with the current plan.

@JamesNK can you elaborate on your choice of using FullName so we have higher confidence that we are doing the right thing?

[JamesNK]

have to handle arbitrary exceptions and want to avoid ambiguity caused by name collisions

Yes. A TaskException could mean the built-in exception or a custom TaskException exception thrown from a calendar app that allows people to schedule tasks. Those exceptions would be intermingled.

error.type doesn't mention whether the exception type must be fully qualified. I'll poke the semantic conventions people to clarify the spec. I do see that exception.type does explicitly say the type should be fully qualified.

[antonfirsov]

Fixed by #93414 in release/8.0, and by #93254 in main (9.0).