HTTP 400 status code should not set span status to error on servers

[nordfjord]

Which problem is this PR solving?

according to the spec 400-499 should be left UNSET in case of server span kind

specifically

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.

See the relevant spec section here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#status

Short description of the changes

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Test A

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Einar Norðfjörð (a8499c2)

[codecov]

Codecov Report

Merging #2789 (2b41e79) into main (b749b0d) will increase coverage by 0.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2789      +/-   ##
==========================================
+ Coverage   93.11%   93.41%   +0.29%     
==========================================
  Files         152      162      +10     
  Lines        4912     5559     +647     
  Branches     1044     1175     +131     
==========================================
+ Hits         4574     5193     +619     
- Misses        338      366      +28     

Impacted Files	Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts	94.46% <0.00%> (-0.66%)	⬇️
...es/opentelemetry-instrumentation-http/src/utils.ts	99.00% <0.00%> (-0.06%)	⬇️
...ation-xml-http-request/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts	100.00% <0.00%> (ø)
packages/exporter-trace-otlp-http/src/types.ts	100.00% <0.00%> (ø)
...s/exporter-trace-otlp-http/src/OTLPExporterBase.ts	100.00% <0.00%> (ø)
...exporter-metrics-otlp-http/src/transformMetrics.ts	95.65% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	97.00% <0.00%> (ø)
packages/exporter-trace-otlp-http/src/util.ts	100.00% <0.00%> (ø)
... and 2 more

[dyladan]

I remember some conversations with specification a while ago that in http clients no http status should cause the span to be an error because the request itself was actually successful. An error would imply some sort of connection failure or crash or similar. Have you looked into the spec to see if there is any guidance on this?

[nordfjord]

I remember some conversations with specification a while ago that in http clients no http status should cause the span to be an error because the request itself was actually successful. An error would imply some sort of connection failure or crash or similar. Have you looked into the spec to see if there is any guidance on this?

See the quoted section in the pr. It says client spans that get a 400 response should set the span status to error

[dyladan]

OK that works for me. Looks like you have some lint fixes but otherwise I don't have any major objection to the change.

[legendecas]

@nordfjord CI has failed. Would you mind giving it a fix?

[nordfjord]

@nordfjord CI has failed. Would you mind giving it a fix?

Thanks for the reminder 🤦. Yes I think it's fixed now.

[dyladan]

LGTM % minor readability suggestion