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

error.type attribute value of http.server.request.duration metric #51620

Open
1 task done
vishweshbankwar opened this issue Oct 24, 2023 · 5 comments · Fixed by open-telemetry/opentelemetry-dotnet#4986
Open
1 task done
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@vishweshbankwar
Copy link

vishweshbankwar commented Oct 24, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

The value of error.type attribute on http.server.request.duration does not take the status code in to account as per the spec

If response status code was sent or received and status indicates an error according to HTTP span status definition, error.type SHOULD be set to the status code number (represented as a string), an exception type (if thrown) or a component-specific error identifier.

Src link:

// This exception is only present if there is an unhandled exception.
// An exception caught by ExceptionHandlerMiddleware and DeveloperExceptionMiddleware isn't thrown to here. Instead, those middleware add error.type to custom tags.
if (exception != null)
{
tags.Add("error.type", exception.GetType().FullName);
}

Expected Behavior

error.type is set to Status Code value as per spec.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

.NET8

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Oct 24, 2023
@vishweshbankwar
Copy link
Author

@JamesNK PTAL

@danmoseley
Copy link
Member

@JamesNK we can possibly still take this for GA if necessary.

@JamesNK
Copy link
Member

JamesNK commented Oct 25, 2023

This is not a must-have and can wait.

The missing behavior is error.type should be populated if the server returns a 5xx status code; however, most of the time, a 5xx status code is set from ASP.NET Core error handling. Error handling sets error.type to the exception type name, which takes precedence over the status code. That means it's rare to have a 5xx status code without error.type already having a value.

I think this could wait until .NET 9, and we have a breaking change notification to tell people about the change in behavior between .NET 8 and .NET 9.

@noahfalk @antonfirsov @lmolkova Thoughts?

@noahfalk
Copy link
Member

This is not a must-have and can wait.

Sounds fine to me. 👍

@vishweshbankwar
Copy link
Author

vishweshbankwar commented Oct 25, 2023

Thanks for confirming the plan. For .NET6.0 and .NET7.0 we would keep the same behavior as .NET8.0 in order to avoid breaking users moving from .NET6.0/.NET7.0. (slightly deviating from spec). @trask FYI

cc: @alanwest @CodeBlanch @utpilla

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@JamesNK JamesNK self-assigned this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants