Initial migration from request to got for http-request node #2952
Conversation
| node.metric("size.bytes", msg, res.client.bytesRead); | ||
| } | ||
| got(url,opts).then(res => { | ||
| msg.statusCode = res.statusCode; |
There was a problem hiding this comment.
As I think that @k-toumura said before, we would like to need the starting and ending time of the http request when we investigate logs of Node-RED flow to call REST APIs. We found that the got module is suitable to fill the requirement. Can we add the code like the following to output trace log?
log.trace('http request: nodeid=' + this.id + ' _msgid=' + msg._msgid
+ ' url=' + res.url + ' start=' + res.timings.start + ' end=' + res.timings.end);
There was a problem hiding this comment.
- We can also extend metric logs, such as:
node.metric("duration.millis", msg, res.timings.phases.total);
node.metric("duration.wait.millis", msg, res.timings.phases.wait);
//... and others (dns, tcp, tls, request, firstByte, download)- If httprequest node emit a message that contains entire
timingsobject, following nodes can use this timing values in application (or use them for debugging purpose).
msg.timings = res.timingsThere was a problem hiding this comment.
I'm aware the current code doesn't yet do the metrics reporting the old node did. That is on my list to fix and yes, got makes it much easier to do then the old code.
But I'm not sure about adding timings to the message - we don't do that with any other node. I'm also not keen in exposing things the module gives us directly - because if we ever have to move off the got module, then it's even more work to reproduce the same behaviour.
As it stands, we're still blocked on this PR until we can get proxy support working. The issue i had raised on hpagent has not been answered - so need to look at a different plan for it.
There was a problem hiding this comment.
@knolleary @k-toumura Thank you for your comments. After supporting proxy in the new http request node, we can test it using our corporate proxy.
There was a problem hiding this comment.
could this be optional behind the metrics option in settings ?
There was a problem hiding this comment.
It could, but that doesn't change the fact it's becomes part of the public API of the node and puts us on the hook to ensure its always there
There was a problem hiding this comment.
not so much if it's only there for debug. - yes if it there for daily operation.
|
Progress, I have proxying working but not if there is a redirect to SSL (actually even without the proxy it fails a redirect on my domain) Also it looks to be appending the the proxy port to the end of the URL in |
|
If I apply the fix from delvedor/hpagent#18 then proxying is working for me with a local Squid instance. |
| msg.statusCode = res.statusCode; | ||
| msg.headers = res.headers; | ||
| let hostname = res.req.host; | ||
| if (res.req.protocol === "http:" && res.socket.remotePort !== 80) { |
There was a problem hiding this comment.
This is picking up the port number from the proxy
|
@k-toumura I think I have proxy support working, there is a fork of this branch here if you would like to test |
Get got working with proxies
|
One thing I have noticed is that the hpagent uses the CONNECT verb for all connection, so the proxy never gets to cache and some proxies might be configured not to support CONNECT for none HTTPS connections. Will need to remember to look out for that. There is a hpagent bug asking for cache support (but it's been unanswered since Feb 2021). |
The original version would pick up the proxy servers port number
|
I'm 95% sure the test failures are to do with the hpagent only using CONNECT to request content directly from the original target rather than from the cache |
Fix msg.responseUrl
|
Merging as-is - with more work on the proxy tests to follow |
|
http request node in the current dev branch returns HTTP 400 when using our corporate proxy. The first example using |
|
@kazuhitoyokoi thanks for letting us know. We'll go ahead with the beta release with a warning about the proxy support not being there yet. If there's debugging you can do to help track down a fix, that would be appreciated. |
|
When testing can you try a mix of http and https sites and also let us know how the proxy is setup:
Also is the proxy accessed via http or https? And if possible some wireshark/network capture logs might help |
Proposed changes
This migrates the HTTP Request node to use the
gotmodule in place of the deprecatedrequestmodule - #2481I have got to the point where the unit tests pass except for:
got.I suspect there will be other edge cases where things have changed - particularly around how errors are reported. But this is a good start.