fix default timeouts and ignored requestTimeout

[ich199]

A couple of default NodeJS and HAPI timeouts were causing errors on long running Elasticsearch queries > 120 seconds.

Closes #6331

[elasticsearch-release]

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

[epixa]

@ich199 Can you issue a PR with these changes against master? We apply changes to master and then "backport" them to previous version feature branches. If this were to only be merged into 4.x, then it will simply regress when we release 5.0.

[epixa]

jenkins, test it

[w33ble]

Is there a reason for adding another 100ms to the configured timeout value?

[ich199]

I had put this in to prevent a race condition and allow the web es client (https://github.com/ich199/kibana/blob/4.x/src/ui/public/es.js) to return/display the correct requestTimeout error before the socket was closed by the Kibana server process. Whether it is actually needed, I'm not sure.

[w33ble]

The race condition is a valid concern, but honestly, if the request is timing out, I think we should just let it time out and leave it at that. Added any amount of time padding there, there's still no guarantee we're going to see an error, it could still just timeout.

[w33ble]

@ich199 I set up a proxy to inject huge delays in requests to ES, and ran in to another problem with the health check. I opened a PR against your branch: https://github.com/ich199/kibana/pull/1

Over 3 minutes to load Discover since it's a 30s wait for every request, but it worked when I bumped the requestTimeout value up to 40000. With the changes in that PR, and pending the couple of questions I had on this one, I think this looks good.

I also noticed that there are a ton of merge conflicts against master. Don't worry about opening a new PR, I'll fix master by hand once this is merged. Fortunately, the changes are pretty small.

[w33ble]

I guess I spoke a bit too soon. Seems the node socket timeout is still an issue once you cross the 2 minute mark.

This PR sets the time on h2o2, which doesn't affect the socket timeout, which defaults to 2 minutes. The request options you linked to originally seems to be the answer, but they don't seem to actually do anything - maybe I'm using it wrong though...

Once I get this sorted, I'll probably append the fix to that PR I opened against your branch.

[ich199]

In my initial testing I hard coded a timeout value in the connectionOptions object in https://github.com/ich199/kibana/blob/4.x/src/server/http/index.js which definitely didn't error after 2 minutes and the expected results were returned after approx 3 minutes. e.g.
var connectionOptions = { host: config.get('server.host'), port: config.get('server.port'), state: { strictHeader: false }, routes: { cors: config.get('server.cors'), payload: { maxBytes: config.get('server.maxPayloadBytes') }, timeout: { socket: 300000 } } };

So, I think the route options do work. It's probably the way I have tried to set this value after the routes have been set up.

Unfortunately, I couldn't find a way of accessing the config.get('elasticsearch.RequestTimeout') value at this point as the Elasticsearch plugin is loaded at a later time. So, the line this.server.listener.timeout = this.config.get('elasticsearch.requestTimeout') in KbnServer.js in the commit ich199@86eb9d6?diff=unified should be setting the socket timeout, the same as the above timeout.socket parameter. However, I changed the location of setting this value in the commit ich199@a385245 Maybe this has stopped it working?

The other timeout being set, which you mentioned, is to fix the default 3 minute es h2o2 hapi proxy timeout, which I discovered on a long running query which took over 3 minutes to return results.

[w33ble]

Yeah, the timeout: { socket: whatever } is the setting we want here, and we should be able to add that to the proxy route as well, if I understand the docs correctly. And I'm fairly confident I'm using it correctly with h2o2, since I was getting validation errors with some of the values I used.

Note that the connectionOptions you pointed to here don't apply to the proxy path though (the /elasticsearch route), just internal Kibana routes. So we don't want to use the elasticsearch.requestTimeout value here, unless both are being applied - I don't think that's the case, but I'll dig some more.

[w33ble]

@ich199 I updated the PR with the socket timeout setting. With that, slow responses from Elasticsearch are now properly handled.

Take a look at the PR https://github.com/ich199/kibana/pull/1, merge it if it looks good to you and it'll show up in this PR.

After that, just need to address the + 100 timeout and requestTimeout default mentioned above. I'd like to see both reverted (and the yml file fixed) personally, but if you're dead-set on not doing that, I can reach out for a second opinion. I can also open another PR on your branch with the changes.

I think we can get this merged today.

[ich199]

@w33ble that all looks good to me, I've merged those changes into my pr. I've just reverted the default and padding too. .

[epixa]

jenkins, test it

[w33ble]

LGTM, thanks @ich199

I want to have @spalger take a quick look at this too, since some of these changes are mine and technically haven't been reviewed by anyone else internally yet. Once I get the go-ahead from him, I'll merge this and get the changes applied to master as well.

[spalger]

hmm, this doesn't feel right

[spalger]

Primary issue is that this is using the elasticsearch plugins's config in KbnServer. Ideally, this would be something that happens at the route level, no? The elasticsearch plugin could define the timeout for the proxy route to match it's requestTimeout configuration.

[spalger]

Oh, looks like you're already doing that. Then you should just remove this.

[w33ble]

Good point. I'll take care of that.

[w33ble]

Since there were a couple more small changes needed, I opened #6377 to take care of them

Thanks again for the PR @ich199