Add maxIdleSockets and idleSocketTimeout to Elasticsearch config by rudolf · Pull Request #142019 · elastic/kibana

rudolf · 2022-09-27T21:56:21Z

Summary

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

github-actions · 2022-09-27T21:56:35Z

Documentation preview:

✨ Changed pages

rudolf · 2022-09-27T21:58:44Z

    disablePrototypePoisoningProtection: true,
    agent: {
-      maxSockets: config.maxSockets,
+      maxTotalSockets: config.maxSockets,


maxSockets only limits the maximum number of sockets per host. What we want, and what our documentation describes is the maximum amount of sockets that Kibana would open to ES. Because there's multiple ES client instances and hence agents, this still doesn't behave 100% as advertised, but at least maxTotalSockets should bring the behaviour somewhat closer to what we advertise. https://nodejs.org/api/http.html#agentmaxtotalsockets

rudolf · 2022-09-27T22:00:06Z

-      maxSockets: config.maxSockets,
+      maxTotalSockets: config.maxSockets,
      keepAlive: config.keepAlive ?? true,
+      timeout: getDurationAsMs(config.idleSocketTimeout),


https://nodejs.org/api/net.html#socketsettimeouttimeout-callback

Looking at the documentation, it seems that Node itself does not close / destroy the socket.
It rather sends a timeout event, and then it's up to the user to end the connection.
Thus, I'm not sure the connections are being closed due to timeout ATM. I'll give it a try in local and get back to you.

We're good, the Agent subscribes to the 'timeout' event and destroys the socket:
https://github.com/nodejs/node/blob/main/lib/_http_agent.js#L402-L414

rudolf · 2022-09-27T22:00:29Z

+      maxTotalSockets: config.maxSockets,
      keepAlive: config.keepAlive ?? true,
+      timeout: getDurationAsMs(config.idleSocketTimeout),
+      maxFreeSockets: config.maxIdleSockets,


https://nodejs.org/api/http.html#agentmaxfreesockets

rudolf · 2022-09-27T22:01:04Z

-import type { ClientOptions, HttpAgentOptions } from '@elastic/elasticsearch';
+import type { ClientOptions } from '@elastic/elasticsearch';
 import type { ElasticsearchClientConfig } from '@kbn/core-elasticsearch-server';
+import { AgentOptions } from 'https';


Although Elasticsearch-js exposes an HttpAgentOptions type, this type is not up to date with the Nodejs type.

rudolf · 2022-09-27T22:02:10Z

    defaultValue: 'http://localhost:9200',
  }),
  maxSockets: schema.number({ defaultValue: Infinity, min: 1 }),
+  maxIdleSockets: schema.number({ defaultValue: 256, min: 1 }),


256 is the Nodejs default so adding a default value for the config just makes this more explicit and decouples us from any changes to Nodejs itself.

rudolf · 2022-09-27T22:05:09Z

  }),
  maxSockets: schema.number({ defaultValue: Infinity, min: 1 }),
+  maxIdleSockets: schema.number({ defaultValue: 256, min: 1 }),
+  idleSocketTimeout: schema.duration({ defaultValue: '9m' }),


the services forwarder has a timeout of 10minutes. But a race condition is possible where the proxy sends a close but there's some delay in the close packet reaching Kibana and at the same time Kibana might start a new request. So to be on the safe side we have a margin of > requestTimeout (30s by default) so that Kibana would close any idle sockets before the proxy might try to close them.

On cloud we can configure Kibana to default to 9minutes, but because this would be a behaviour change and might cause problems if there's a transparent proxy I choose to default to 60s.

(This image shows the old services-forwarder socket timeout of 30s)

Beware that your implementation is still setting '9m'

rudolf · 2022-09-28T10:41:09Z

Apart from the configuration options this introduces a behaviour change in that sockets now have a 60s timeout instead of Elasticsearch-js's 30s default. Because we default to elasticsearch.maxSockets: Infinity nodejs won't actually re-use the sockets so this shouldn't impact the vast majority of clusters.

Tested this locally by creating load using https://github.com:elastic/kibana-k6-benchmarking and monitoring sockets with:

Kibana <-> Elasticsearch socket count:
watch "lsof -n -i -s | grep -e 'node.*127\.0\.0\.1:wap-wsp' | wc -l"

K6 <-> Kibana socket count:
watch "lsof -n -i -s | grep -e 'node.*127\.0\.0\.1:esmagent' | wc -l"

Some observations from the tests. When Kibana has a large amount of open sockets, the socket numbers continuously jump up and down by several hundreds e.g. 1500 sockets and 2s later 1200 sockets. This is because the default maxIdleSockets is 256 so when there's e.g. 2000 sockets open it quickly happens that more than 256 of them become free and then we close a ton of sockets just to re-open them a second or so later.

When I set a maxIdleSockets value that's really high (e.g. same as maxSockets or > 2000) then the amount of open sockets are much more stable. And this basically means sockets will only be closed if they're idle. Which seems preferable to me. If there's a sudden spike in traffic it means that for the next idleSocketTimeout there will be a large amount of unecessary sockets but it's not a huge issue in and of itself.

elasticmachine · 2022-09-29T13:54:24Z

Pinging @elastic/kibana-core (Team:Core)

weltenwort

monitoring test changes LGTM

rudolf · 2022-10-03T08:55:04Z

@elasticmachine merge upstream

pgayvallet

Overall looking good to me, just have one comment.

As a side note, do we want to allow-list these new settings in Cloud?

pgayvallet · 2022-10-03T09:18:32Z

-          const config = Object.assign(
-            {},
-            DEFAULT_CONFIG,
-            this.agentOptions,
-            agentOptions,
-            connectionOpts.tls
-          );


Not sure to get the exact implications of the changes in this file?

we had a way to create a default agent config passed into the agent manager constructor. But this functionality was never used and sometimes confused my while developing cause it wasn't obvious where the config of the final agent actually came from. It was just a small moment of "what's going on here" but it felt like if we're not using this "feature" then it's easier to reason about the code if we just remove it altogether.

actually there was several levels:

DEFAULT_CONFIG static defaults in code

this.agentOptions optional constructor paramater that can set defaults for any agent created by the agent factories of this agent manager

finally the kibana.yml defaults that are passed into the agent factories when they are created.

So now we just have (3)

Makes sense! (1)'s values are equivalent to NodeJs's defaults, and (2) was not used.

gsoldevila · 2022-10-04T15:44:12Z

Overall looking good to me, just have one comment.

As a side note, do we want to allow-list these new settings in Cloud?

I would say it makes sense, so that we can tweak them once we have metrics about them.
At least, that was the reasoning when we allow-listed maxSockets.

LGTM!

rudolf · 2022-10-10T09:28:59Z

@elasticmachine merge upstream

kibana-ci · 2022-10-10T10:34:01Z

💚 Build Succeeded

Buildkite Build
Commit: 8a6fce2

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-elasticsearch-server`	51	53	+2

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-elasticsearch-server`	99	103	+4
`core`	2689	2691	+2
total			+6

History

💔 Build #78402 failed cc7d962
💚 Build #77517 succeeded e7158e1
💚 Build #77405 succeeded d564270
💛 Build #77001 was flaky 45d6bc6
💔 Build #76867 failed 3e9f483

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…stic#142019) * Add maxIdleSockets and idleSocketTimeout to Elasticsearch config * Simplify agent manager * Fix types * Fix types * Reduce idleSocketTimeout default to 60s * Fix tests * Update docs/setup/settings.asciidoc * Address review comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Add maxIdleSockets and idleSocketTimeout to Elasticsearch config

8ccc8b9

rudolf added Feature:elasticsearch Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Sep 27, 2022

rudolf added the performance label Sep 27, 2022

rudolf commented Sep 27, 2022

View reviewed changes

rudolf added 2 commits September 28, 2022 11:43

Simplify agent manager

8e60541

Fix types

81da2bd

rudolf added 3 commits September 28, 2022 15:30

Fix types

32b084f

Reduce idleSocketTimeout default to 60s

ba6b180

Merge branch 'main' into es-socket-config

3e9f483

rudolf marked this pull request as ready for review September 29, 2022 13:54

rudolf requested review from a team as code owners September 29, 2022 13:54

Fix tests

45d6bc6

rudolf added the release_note:enhancement label Sep 29, 2022

rudolf commented Sep 30, 2022

View reviewed changes

Comment thread docs/setup/settings.asciidoc Outdated

Update docs/setup/settings.asciidoc

1ed37a4

weltenwort approved these changes Sep 30, 2022

View reviewed changes

Merge branch 'main' into es-socket-config

d564270

pgayvallet approved these changes Oct 3, 2022

View reviewed changes

rudolf requested a review from gsoldevila October 3, 2022 11:56

gsoldevila reviewed Oct 3, 2022

View reviewed changes

Comment thread packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/agent_manager.ts Outdated

gsoldevila reviewed Oct 3, 2022

View reviewed changes

Comment thread packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/client_config.test.ts

Address review comments

e7158e1

gsoldevila reviewed Oct 3, 2022

View reviewed changes

Comment thread packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/client_config.test.ts

gsoldevila approved these changes Oct 5, 2022

View reviewed changes

Merge branch 'main' into es-socket-config

cc7d962

rudolf enabled auto-merge (squash) October 5, 2022 14:29

Merge branch 'main' into es-socket-config

8a6fce2

rudolf merged commit 4414692 into main Oct 10, 2022

rudolf deleted the es-socket-config branch October 10, 2022 10:34

kibanamachine added v8.6.0 backport:skip This PR does not require backporting labels Oct 10, 2022

Conversation

rudolf commented Sep 27, 2022

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

github-actions bot commented Sep 27, 2022

Uh oh!

rudolf Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudolf Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudolf commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 29, 2022

Uh oh!

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

rudolf commented Oct 3, 2022

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsoldevila commented Oct 4, 2022

Uh oh!

rudolf commented Oct 10, 2022

Uh oh!

kibana-ci commented Oct 10, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rudolf Sep 27, 2022 •

edited

Loading

rudolf Sep 27, 2022 •

edited

Loading

rudolf commented Sep 28, 2022 •

edited

Loading