-
Notifications
You must be signed in to change notification settings - Fork 227
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
fix: span url hostname not ip #2039
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
Tests failing in |
Per discussions, we've removed the "pass everything through a |
lib/instrumentation/http-shared.js
Outdated
// let url | ||
// try { | ||
// url = new URL(`${protocol}//${host}${port}${req.path}`) | ||
// } catch (e) { | ||
// return undefined | ||
// } |
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.
Drop these.
Fixes #2035
This PR add a
getUrlFromRequestAndOptions
function to thelib/instrumentation/http-shared.js
module. Thehttp-shared
module also uses this new function in place of the oldhttp-request-to-url
module to set theurl
property on a span.The
getUrlFromRequestAndOptions
function accepts aClientRequest
object and a generic options object and uses the information from those objects to return the same final URL that the ClientRequest will use. The options object is eitherThe options object passed to the
request
methodOr, in the case of
http.request
's second formthe
options
object is merged with the URL information fromurl
to create a new object.In addition to the usual test suite, this was tested across all supported node versions with the following sludge-hammer of a bash script.