Fix client metrics recording on round trip error by alimikegami · Pull Request #7146 · open-telemetry/opentelemetry-go-contrib (original) (raw)
This pull request fixes client metrics recording on round trip error
Related issues: #6940
Does this approach correctly address the issue with round trip errors?
Should I add specific tests to verify metric recording (http.client.duration) during round trip errors?
The committers listed above are authorized under a signed CLA.
- ✅ login: alimikegami / name: Alim Ikegami (1f27a0c, c0daa42, a12084e, e604a87, 8bf794a, 5fae8b3, 2c42ebe, 18a32cb, a2476c8, fcc71ee, 08233ad, 7cda07a, 5c0dd92, 7bc8b76, 435ce71, 9a9d726, 2d10c70, 99c76ec)
- ✅ login: pellared / name: Robert Pająk (645ebc0, 09a51c2, 47d12a1, 0d7034d, fc7196a)
Duplicating code like isn't really maintainable long-term. We should be able to have the metrics recorded, with no code duplication. Possibly, by exporting the metric into its own method.
Also, this needs to be tested.
Thank you for the review @dmathieu
What do you think of my latest commit? I’ve added a method to record metrics and removed most of the unnecessary code duplication. Thank you.
Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 81.5%. Comparing base (1d195a9) to head (47d12a1).
Report is 1 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@ ## main #7146 +/- ##
Coverage 81.5% 81.5%
Files 198 198
Lines 17952 17961 +9
- Hits 14634 14643 +9
Misses 2918 2918
Partials 400 400
| Files with missing lines | Coverage Δ | |
|---|---|---|
| instrumentation/net/http/otelhttp/transport.go | 95.1% <100.0%> (+0.2%) | ⬆️ |
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test?
Thank you for the review @dmathieu
I've added the test and refactored the metric recording to use defer, removing the remaining code duplication.
Please check my latest commit. Thank you!
@alimikegami the CI has been failing since your last commit. Could you fix it?
@alimikegami the CI has been failing since your last commit. Could you fix it?
I'll fix it as soon as possible!
Hi @dmathieu,
I've run the tests on my local machine, but I’m not seeing the http.client.request.body.size metric that appears in the CI test results. Instead, I'm seeing http.client.request.size. The same also applies to the duration metric.
Here's the test output from my local environment:
Do you know what might be causing the difference in metric names between the CI environment and my local machine?
Thank you for the explanation, @dmathieu. It cleared up my confusion. Please kindly review my test and implementation! I have fixed the failing tests. Thank you!
Co-authored-by: Damien Mathieu 42@dmathieu.com
I've committed the CHANGELOG update and removed the recordMetrics method. Please kindly review the changes @dmathieu. Thank you!
Hi @dmathieu, I’ve moved the entry to the unreleased section. Would it be possible to get this merged, or is there anything I need to do on my end? Thank you!
We need a second approval.
….com/alimikegami/opentelemetry-go-contrib into fix/6940-otelhttp-client-metrics-error
I've refactored the code according to your suggestions, @pellared. Please have a look at my latest commit. Thank you!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmathieu, can you take another look please?
The latest changes appear to break the tests.
I am not sure why but some tests are missing http.response.status_code attribute.
I've fixed the failing tests, please kindly check my latest commit. Thank you!
@dmathieu @pellared
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})