-
-
Notifications
You must be signed in to change notification settings - Fork 519
Correctly parse IPv6 addresses in Net::HTTP instrumentation #2180
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2180 +/- ##
==========================================
+ Coverage 97.28% 97.31% +0.02%
==========================================
Files 99 99
Lines 3688 3690 +2
==========================================
+ Hits 3588 3591 +3
+ Misses 100 99 -1
|
Work in progress. Seems like older Rubies do not like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, it looks great 👍 I just have a nitpick on test.
@@ -1,6 +1,7 @@ | |||
# frozen_string_literal: true | |||
|
|||
require "net/http" | |||
require "resolv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add it to the net/http
patch instead as it's required by it, not HTTPTransport
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/facepalm my bad, one sec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@st0012, cleaned things up!
Let's see how CI gods feel about it. |
Co-authored-by: Stan Lo <[email protected]>
Summary
This pull request adds a
Net::HTTP
instrumentation fix forextract_request_info
that makes it parseIPv6
addresses correctly.Fixes #2163.
Thank you @Rotario for reporting and suggesting a fix!
Changes