fix(node): Ensure correct URL is passed to ignoreIncomingRequests callback
#12929
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...kages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreOutgoingRequests.js
Dismissed
Show dismissed
Hide dismissed
| */ | ||
| export function startExpressServerAndSendPortToRunner(app: Express): void { | ||
| const server = app.listen(0, () => { | ||
| export function startExpressServerAndSendPortToRunner(app: Express, port: number | undefined = undefined): void { |
There was a problem hiding this comment.
small adjustment to our node integration tests framework: In the express apps, we can not optionally pass a fixed port which makes running the app locally a lot less painful 😅
| @@ -103,7 +103,7 @@ export const instrumentHttp = Object.assign( | |||
| }, | |||
|
|
|||
| ignoreIncomingRequestHook: request => { | |||
| const url = getRequestUrl(request); | |||
| const url = request.url; | |||
There was a problem hiding this comment.
just checking to be sure, is the url here the full URL, including protocol? so e.g. http://...? As we want these to be full URLs I guess always, if possible.
There was a problem hiding this comment.
@mydea url is NOT the full url, it is instead just the path (and maybe the query params? EDIT: definitely has query params too.).
the node:IncomingMessage type doesn't have a protocol, hostname, or port attr.
- For protocol you'd have to do something like access
req.socket.encryptedand infer true === https - For hostname you'd have to parse the request headers for a Host header, falling back to req.socket.address() if it doesn't exist.
- For port you'd have to access
req.socket.address().port, I think.
So all that to say, sadly it's difficult to reconstruct the full url. Thoughts?
Screenshot:
There was a problem hiding this comment.
@mydea sorry i saw you moved your comment, so I just moved mine before I saw you replied 😅
I'll let you move your reply here so the comments aren't fragmented.
There was a problem hiding this comment.
Yeah, I feared that. IMHO this is OK, but we should maybe reflect this in the jsdoc/variables used for ignoreIncomingRequests, e.g. make it ignoreIncomingRequests(urlPath: string) or something like this?
Sounds reasonable to me but sadly this is a breaking change, no?
There was a problem hiding this comment.
Just changing the name of the variable is fine. Will make the change and update JSDoc and docs to reflect it. "Luckily" this never worked in v8 in the first place (afaict) so we're likely not behaviourally breaking folks.
There was a problem hiding this comment.
Regarding docs update: I'll do this in combination with the changes from #12930
size-limit report 📦
|
Co-authored-by: Francesco Novy <[email protected]>
This PR fixes an oversight in our Node
httpIntegration. It looks like we assumed that therequestobject being passed toignoreIncomingRequestHookandignoreOutgoingRequestHookwas of the same type. However, it's not:requestis of typeIncomingMessageinignoreIncomingRequestHookrequestis of typeRequestOptionsinignoreOutgoingRequestHookIn both cases though, we tried to obtain the url to pass into the
ignoreIncoming|outgoingRequestscallback via ourgetUrlhelper function. SinceIncomingMessagedoesn't have any of the properties required bygetUrl, we always passed///to theignoreIncomingRequestscallback.This PR fixes the bug by simply taking the
request.urlproperty instead and adds integration tests to properly test the two options.We could do more here, for example
makepass theignoreIncomingRequestHookfully/partially overwriteable like we did withrequestHookor via_experimentalConfigrequestobject as an additional param to the user-facing function but this should be done in a follow-up PR.fixes #12913