Skip to content

Fix client metrics recording on round trip error #7146

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alimikegami
Copy link

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?

@alimikegami alimikegami requested review from dmathieu and a team as code owners April 3, 2025 13:12
Copy link

linux-foundation-easycla bot commented Apr 3, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

dmathieu commented Apr 7, 2025

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.

@alimikegami
Copy link
Author

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.

Copy link

codecov bot commented Apr 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.8%. Comparing base (5e1f920) to head (e604a87).
Report is 70 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #7146    +/-   ##
======================================
  Coverage   75.8%   75.8%            
======================================
  Files        207     206     -1     
  Lines      19484   19258   -226     
======================================
- Hits       14783   14612   -171     
+ Misses      4265    4213    -52     
+ Partials     436     433     -3     
Files with missing lines Coverage Δ
instrumentation/net/http/otelhttp/transport.go 95.3% <100.0%> (+0.5%) ⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@dmathieu dmathieu left a comment

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?

metricOpts := t.semconv.MetricOptions(metricAttributes)

// set transport record metrics
t.recordMetrics(ctx, requestStartTime, metricData, metricOpts)
Copy link
Member

Choose a reason for hiding this comment

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

How about calling this before the error check, in a defer statement?

@alimikegami
Copy link
Author

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!

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.

2 participants