fix(instrumentation-fetch): Handling null-body-status responses

[m0sa]

Which problem is this PR solving?

Fixes incorrect handling of null-body-status responses, and adds tests to ensure this doesn't go un-noticed again.

Fixes #6036

Short description of the changes

Fixing null-body-status response handling in instrumentation-fetch.

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?

Error was reproduced in unit tests and then fixed.

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: m0sa / name: Samo Prelog (20b8698, 4b09457, 6ffb168, 88f1fbe, 99e4837, ca8f2ee, 32c82fd)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.14%. Comparing base (46e4841) to head (99e4837).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6037   +/-   ##
=======================================
  Coverage   95.14%   95.14%           
=======================================
  Files         315      316    +1     
  Lines        9161     9166    +5     
  Branches     2022     2023    +1     
=======================================
+ Hits         8716     8721    +5     
  Misses        445      445           

see 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wolfgangcodes]

Other tests make assertions that the spans were emitted rather then asserting on the response. Something like:

   assert.strictEqual(exportedSpans.length, 1);

Suggested change

	it('should be handled correctly', async () => {
	assert.strictEqual(response?.status, 204);
	});
	it('204 (No Content) will correctly end the span', async () => {
	assert.strictEqual(exportedSpans.length, 1);
	});

We also have untested functionality for other null-body content. 101, 205, 304 we could test those as well.

[m0sa]

I added separate test cases for each status code.

I did find a weird edge-case in 101 that IDK if is already handled correctly. I would expect a span to be exported, with the correct error code, but this either doesn't happen in tests due to how it's set up, or it actually wouldn't happen. @wolfgangcodes any ideas?

The fetch.ts endSpanOnError callback does happen in that case, but somehow the span is not there for inspection.

[wolfgangcodes]

Thanks for adding those!

I don't have any immediate thoughts on what might be happening with 101s, but I like your approach of implementing what we can test easily right now for for common success responses.

We can always follow up with better support for 101s if they crop up or if we have more time to think about it!

I approved to show my support, but we'll need add'l approvals from the JS folks.

[david-luna]

I assume these statuses are the ones with null body according to a spec. But my experience tells me some apis may not follow these specs and return body anyways. I wonder if it's okay to not process the body in this scenario

[m0sa]

The new Response({ /* non-null-wrapped-body */ }, { status: 204 /* or 101, 205, 304 */ }) constructor always throws:

The only way of processing it further would mean avoid reconstructing it / preserving a code path that was in place prior to #5894

[david-luna]

well, backends could be written in different languages and those may let the dev pass a payload. According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status we should not expect payloads for these statuses so I'm fine. If there is interest in reading the body even with these cases we can add it later.

[david-luna]

LGTM. I'll wait a bit so others can give feedback

cc: @open-telemetry/javascript-approvers

[otelbot]

Thank you for your contribution @m0sa! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.