fnproject / ui

User interface for fn project.
Apache License 2.0
89 stars 21 forks source link

Configure request options #45

Closed nigeldeakin closed 5 years ago

nigeldeakin commented 6 years ago

This PR introduces the config module and uses it to allow request options to be specified for when the UI server performs the GET requests to the Fn server.

See the new CONFIG.md file for more information.

(Note that this uses the same config package as the other PR https://github.com/fnproject/ui/pull/44)

skinowski commented 6 years ago

According to requestoptions if pool is a new object every time, maxSockets might not work as intended? Should the requests pass in same pool object across UI to make this work? Or perhaps we could instantiate our own http Agent and use that.

nigeldeakin commented 6 years ago

This is how we're creating the options object:

options = {url: url, qs: params}
options = config.util.extendDeep({},options,config.fnApiOptions)
options = exports.addAuth(options, req)

I would expect config.fnApiOptions to be the same object each time, so it depends on how extendDeep merges this into the options object created in the previous line. The docs at https://github.com/lorenwest/node-config/wiki/Using-Config-Utilities imply that the objects are "merged": "This does not replace deep objects as other extend functions do, but dives into them extending individual elements instead." So it looks to me as if the same pool object is being passed in each time, though I have not actually verified this in a test. It would probably be worth doing so.

This change is presented as a general feature for setting request options but yes, the purpose is actually to allow maxSockets to be configured, to workaround an issue in a particular installation. And setting maxSockets (as in the example) did make a difference,

Some background detail I found:

Before taking anything further it might be worth discussing the issue which prompted this, which is that without reducing the number of sockets, a local UI server was causing the Fn server running on api.fnservice.io to reset connection requests (connection reset) - and not just from clients calling /stats. This change was created to help explore that issue, and perhaps it doesn't need to actually become a permanent feature.

skinowski commented 6 years ago

@nigeldeakin The reset conn case, could you provide more details if you have any? (eg. how or how long did you run ui and fn into this state, etc.)

nigeldeakin commented 6 years ago

I've dug out the details (which I reported on a Slack channel at the time) and pasted them into a new UI issue: https://github.com/fnproject/ui/issues/49