xray client: return an error if the HTTP request failed

[jaedle]

No description provided.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jaedle / name: Dennis Wielepsky (c4ac4eb, 82112a5, 8b93a94, 54c8883, 7230e92, 193cf0c, dde95ba, 42069a5, 4aff441, 714147d, 1c24da8, 1d6d944, d572daa)
• ✅ login: dmathieu / name: Damien Mathieu (21a1bf7)
• ✅ login: dashpole / name: David Ashpole (52ce933)

[dmathieu]

This needs a changelog entry

[jaedle]

This needs a changelog entry

I added a changelog entry, I hope that fits the current style. If not, I would appreciate suggestions, thanks!

[codecov]

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 66.8%. Comparing base (9207da4) to head (d572daa).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
samplers/aws/xray/internal/client.go	88.8%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5718   +/-   ##
=====================================
  Coverage   66.8%   66.8%           
=====================================
  Files        206     206           
  Lines      13207   13211    +4     
=====================================
+ Hits        8827    8833    +6     
+ Misses      4112    4111    -1     
+ Partials     268     267    -1     

Files with missing lines	Coverage Δ
samplers/aws/xray/internal/client.go	86.9% <88.8%> (+6.0%)	⬆️

[dmathieu]

cc @wangzlei as the soon to be owner of this instrumentation.

[jaedle]

I reworked the error messages and added test cases / assertions.

Some edge cases might be hard to test / not worth the effort like creating http requests.

I hope you can find some time to review the changes or to comment if there is a need for changes. 🙏

[dmathieu]

Your PR now does more than just checking for non-200 HTTP status code.
Other fixes should be in new PRs, not in this one.

[jaedle]

Thanks for the feedback.

Following you gave a thumbs up on my comment above, I thought that would be appropriate.

If that’s fine for you, I would work on that within this PR.

Sorry for my confusion.

[dmathieu]

My comment was about assertion of errors in tests.
You're also changing the errors content, and changing the way the response body is closed now.

[jaedle]

My comment was about assertion of errors in tests. You're also changing the errors content, and changing the way the response body is closed now.

Rolled back in a moment, thanks.

[jaedle]

Sorry for the change, the old way of dealing with response bodies was restored.

[jaedle]

Hey 👋

Any chance to get this merged and released realistically in foreseeable future?

[dmathieu]

This needs a second review from a @open-telemetry/go-approvers

[jaedle]

Anything I can do from my side to speed things up? It's already 3 month in review.

[dashpole]

Sorry. We don't have any codeowners for the component, so reviews and such are likely to be much slower.

[jaedle]

Sorry. We don't have any codeowners for the component, so reviews and such are likely to be much slower.

Thanks for the quick response.