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

fix: http 400 status is not error on server #2904

Merged
merged 5 commits into from
Feb 16, 2022

Conversation

nordfjord
Copy link
Contributor

@nordfjord nordfjord commented Feb 16, 2022

The specification here https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#status.

Says:

For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER and MUST be set to Error in case of SpanKind.CLIENT

Changes

For significant contributions please make sure you have completed the following items:

@nordfjord nordfjord requested a review from a team February 16, 2022 03:22
@reyang
Copy link
Member

reyang commented Feb 16, 2022

Thanks @nordfjord! Would you help to update the changelog?

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming changelog is updated.
It might be nice to include some test cases.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #2904 (d234277) into main (a2c1b6e) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2904      +/-   ##
==========================================
+ Coverage   84.20%   84.36%   +0.15%     
==========================================
  Files         252      252              
  Lines        8895     8896       +1     
==========================================
+ Hits         7490     7505      +15     
+ Misses       1405     1391      -14     
Impacted Files Coverage Δ
src/OpenTelemetry.Api/Internal/SpanHelper.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 88.60% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 86.42% <100.00%> (ø)
...tp/Implementation/HttpHandlerDiagnosticListener.cs 71.57% <100.00%> (ø)
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 75.00% <0.00%> (+2.77%) ⬆️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 89.42% <0.00%> (+2.88%) ⬆️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs 62.50% <0.00%> (+12.50%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (+14.28%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (+22.72%) ⬆️

@nordfjord
Copy link
Contributor Author

Thanks, I've added the changelog entry 🎉 .

Happy to change the entry if necessary as well.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for catching this!

@cijothomas
Copy link
Member

LGTM - need to fix the link in changelog and add one for aspnet, and we are good to merge.

@cijothomas
Copy link
Member

Merging. Only modifying Instrumentation libraries, and not Core components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants