[bug] Should SetStatus() set the description if an exception occurs during ASP.NET Core HTTP request processing?

[martincostello]

Component

OpenTelemetry.Instrumentation.AspNetCore

Package Version

Package Name	Version
OpenTelemetry.Instrumentation.AspNetCore	1.15.1

Runtime Version

net10.0

Description

While working on #3867 and #3808 I noticed a discrepancy in behaviour between the instrumentation here and the built-in instrumentation coming as part of ASP.NET Core 11 (dotnet/aspnetcore#64851).

Here, when an exception occurs we do not set the activity's status description:

opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 328 in 129fa90

activity.SetStatus(ActivityStatusCode.Error);

I believe this is based on the following in the HTTP semantic conventions (source):

Don't set the span status description if the reason can be inferred from http.response.status_code.

However, in ASP.NET Core 11 preview 2, the status description is set here if an exception occurring during processing:

if (exception != null)
{
    activity.SetTag(HostingTelemetryHelpers.AttributeErrorType, exception.GetType().FullName);
    activity.SetStatus(ActivityStatusCode.Error, exception.Message);
}

This discrepancy was flushed out in #3867 by this test:

opentelemetry-dotnet-contrib/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs

Lines 118 to 120 in 129fa90

	// Instrumentation is not expected to set status description
	// as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode
	Assert.Null(activity.StatusDescription);

---

The question here is: which is correct?

I can see an argument both ways.

On the one hand it could be set when there's an exception as we have more context, and an error occurred. The HTTP status code that was set because of it is just an implementation detail (and if unhandled, is likely to be HTTP 500 anyway), so it's more useful to the application's operator to know the actual reason for the error.

On the other hand, there's always going to be an HTTP status code sent to the client in an exception occurs during processing, so we could set nothing and leave it to be inferred.

I think it would make sense for the behaviour to be consistent between the two, whether that's with a change here to add the detail, or a change in dotnet/aspnetcore for 11.0 that matches the interpretation here.

/cc @JamesNK

Steps to Reproduce

See this code in #3867:

opentelemetry-dotnet-contrib/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs

Lines 118 to 125 in b659bb3

	// TODO Is ASP.NET Core 11 doing the right thing here?
	#if NET11_0_OR_GREATER
	Assert.Equal(statusCode is 503 ? "exception description" : null, activity.StatusDescription);
	#else
	// Instrumentation is not expected to set status description
	// as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode
	Assert.Null(activity.StatusDescription);
	#endif

Expected Result

The behaviour is consistent between the ASP.NET Core instrumentation and ASP.NET Core 11.

Actual Result

ASP.NET Core 11 sets the status description, the ASP.NET Core instrumentation does not.

Additional Context

No response

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[JamesNK]

Feel free to change ASP.NET Core if it makes sense.

[martincostello]

dotnet/aspnetcore#65825