Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add maxIdleSockets and idleSocketTimeout to Elasticsearch config #142019

Merged
merged 12 commits into from
Oct 10, 2022
Merged

Add maxIdleSockets and idleSocketTimeout to Elasticsearch config #142019

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8ccc8b9
Add maxIdleSockets and idleSocketTimeout to Elasticsearch config
rudolf Sep 27, 2022
8e60541
Simplify agent manager
rudolf Sep 28, 2022
81da2bd
Fix types
rudolf Sep 28, 2022
32b084f
Fix types
rudolf Sep 28, 2022
ba6b180
Reduce idleSocketTimeout default to 60s
rudolf Sep 29, 2022
3e9f483
Merge branch 'main' into es-socket-config
rudolf Sep 29, 2022
45d6bc6
Fix tests
rudolf Sep 29, 2022
1ed37a4
Update docs/setup/settings.asciidoc
rudolf Sep 30, 2022
d564270
Merge branch 'main' into es-socket-config
kibanamachine Oct 3, 2022
e7158e1
Address review comments
rudolf Oct 3, 2022
cc7d962
Merge branch 'main' into es-socket-config
rudolf Oct 5, 2022
8a6fce2
Merge branch 'main' into es-socket-config
kibanamachine Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion docs/setup/settings.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,18 @@ configuration is effectively ignored when <<csp-strict, `csp.strict`>> is enable
*Default: `true`*

[[elasticsearch-maxSockets]] `elasticsearch.maxSockets`::
The maximum number of sockets that can be used for communications with elasticsearch.
The maximum number of sockets that can be used for communications with {es}.
*Default: `Infinity`*


[[elasticsearch-maxIdleSockets]] `elasticsearch.maxIdleSockets`::
The maximum number of idle sockets to keep open between {kib} and {es}. If more sockets become idle, they will be closed.
*Default: `256`*

[[elasticsearch-idleSocketTimeout]] `elasticsearch.idleSocketTimeout`::
The timeout for idle sockets kept open between {kib} and {es}. If the socket is idle for longer than this timeout, it will be closed. If you have a transparent proxy between {kib} and {es} be sure to set this value lower than or equal to the proxy's timeout.
*Default: `9m`*
rudolf marked this conversation as resolved.
Show resolved Hide resolved

`elasticsearch.customHeaders`::
| Header names and values to send to {es}. Any custom headers cannot be
overwritten by client-side headers, regardless of the
Expand Down
48 changes: 40 additions & 8 deletions ...es/core/elasticsearch/core-elasticsearch-client-server-internal/src/client_config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ const createConfig = (
customHeaders: {},
compression: false,
maxSockets: Infinity,
maxIdleSockets: 256,
idleSocketTimeout: duration(9, 'minutes'),
sniffOnStart: false,
sniffOnConnectionFault: false,
sniffInterval: false,
Expand All @@ -41,15 +43,13 @@ describe('parseClientOptions', () => {
);
});

it('specifies `headers.maxSockets` Infinity and `keepAlive` true by default', () => {
it('specifies `maxTotalSockets` Infinity and `keepAlive` true by default', () => {
const config = createConfig({});

expect(parseClientOptions(config, false, kibanaVersion)).toEqual(
expect(parseClientOptions(config, false, kibanaVersion).agent).toEqual(
expect.objectContaining({
agent: {
keepAlive: true,
maxSockets: Infinity,
},
keepAlive: true,
maxTotalSockets: Infinity,
})
);
});
Expand Down Expand Up @@ -125,12 +125,44 @@ describe('parseClientOptions', () => {
false,
kibanaVersion
);
expect(options.agent).toHaveProperty('maxSockets', 1024);
expect(options.agent).toHaveProperty('maxTotalSockets', 1024);
rudolf marked this conversation as resolved.
Show resolved Hide resolved
});

it('defaults to `Infinity` if not specified by the config', () => {
const options = parseClientOptions(createConfig({}), false, kibanaVersion);
expect(options.agent).toHaveProperty('maxSockets', Infinity);
expect(options.agent).toHaveProperty('maxTotalSockets', Infinity);
});
});

describe('`maxIdleSockets` option', () => {
rudolf marked this conversation as resolved.
Show resolved Hide resolved
it('uses the specified config value', () => {
const options = parseClientOptions(
createConfig({ maxIdleSockets: 1024 }),
false,
kibanaVersion
);
expect(options.agent).toHaveProperty('maxFreeSockets', 1024);
});

it('defaults to `256` if not specified by the config', () => {
const options = parseClientOptions(createConfig({}), false, kibanaVersion);
expect(options.agent).toHaveProperty('maxFreeSockets', 256);
});
});

describe('`idleSocketTimeout` option', () => {
it('uses the specified config value', () => {
const options = parseClientOptions(
createConfig({ idleSocketTimeout: duration(1000, 's') }),
false,
kibanaVersion
);
expect(options.agent).toHaveProperty('timeout', 1_000_000);
});

it('defaults to `9m` if not specified by the config', () => {
const options = parseClientOptions(createConfig({}), false, kibanaVersion);
expect(options.agent).toHaveProperty('timeout', 9 * 60 * 1000);
});
});

Expand Down
9 changes: 6 additions & 3 deletions packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/client_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@
import { ConnectionOptions as TlsConnectionOptions } from 'tls';
import { URL } from 'url';
import { Duration } from 'moment';
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';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { getDefaultHeaders } from './headers';

export type ParsedClientOptions = Omit<ClientOptions, 'agent'> & { agent: HttpAgentOptions };
export type ParsedClientOptions = Omit<ClientOptions, 'agent'> & { agent: AgentOptions };

/**
* Parse the client options from given client config and `scoped` flag.
Expand All @@ -38,8 +39,10 @@ export function parseClientOptions(
// fixes https://github.com/elastic/kibana/issues/101944
disablePrototypePoisoningProtection: true,
agent: {
maxSockets: config.maxSockets,
maxTotalSockets: config.maxSockets,
Copy link
Contributor Author

@rudolf rudolf Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

keepAlive: config.keepAlive ?? true,
timeout: getDurationAsMs(config.idleSocketTimeout),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maxFreeSockets: config.maxIdleSockets,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
compression: config.compression,
};
Expand Down
2 changes: 2 additions & 0 deletions ...es/core/elasticsearch/core-elasticsearch-server-internal/src/elasticsearch_config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ test('set correct defaults', () => {
"hosts": Array [
"http://localhost:9200",
],
"idleSocketTimeout": "PT9M",
"ignoreVersionMismatch": false,
"maxIdleSockets": 256,
"maxSockets": Infinity,
"password": undefined,
"pingTimeout": "PT30S",
Expand Down
14 changes: 14 additions & 0 deletions packages/core/elasticsearch/core-elasticsearch-server-internal/src/elasticsearch_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export const configSchema = schema.object({
defaultValue: 'http://localhost:9200',
}),
maxSockets: schema.number({ defaultValue: Infinity, min: 1 }),
maxIdleSockets: schema.number({ defaultValue: 256, min: 1 }),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

idleSocketTimeout: schema.duration({ defaultValue: '9m' }),
Copy link
Contributor Author

@rudolf rudolf Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Screenshot 2022-07-28 at 11 16 26

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beware that your implementation is still setting '9m'

compression: schema.boolean({ defaultValue: false }),
username: schema.maybe(
schema.string({
Expand Down Expand Up @@ -304,6 +306,16 @@ export class ElasticsearchConfig implements IElasticsearchConfig {
*/
public readonly maxSockets: number;

/**
* The maximum number of idle sockets to keep open between Kibana and Elasticsearch. If more sockets become idle, they will be closed.
*/
public readonly maxIdleSockets: number;

/**
* The timeout for idle sockets kept open between Kibana and Elasticsearch. If the socket is idle for longer than this timeout, it will be closed.
*/
public readonly idleSocketTimeout: Duration;

/**
* Whether to use compression for communications with elasticsearch.
*/
Expand Down Expand Up @@ -409,6 +421,8 @@ export class ElasticsearchConfig implements IElasticsearchConfig {
this.serviceAccountToken = rawConfig.serviceAccountToken;
this.customHeaders = rawConfig.customHeaders;
this.maxSockets = rawConfig.maxSockets;
this.maxIdleSockets = rawConfig.maxIdleSockets;
this.idleSocketTimeout = rawConfig.idleSocketTimeout;
this.compression = rawConfig.compression;
this.skipStartupConnectionCheck = rawConfig.skipStartupConnectionCheck;

Expand Down
2 changes: 2 additions & 0 deletions packages/core/elasticsearch/core-elasticsearch-server/src/client/client_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export interface ElasticsearchClientConfig {
customHeaders: Record<string, string>;
requestHeadersWhitelist: string[];
maxSockets: number;
maxIdleSockets: number;
idleSocketTimeout: Duration;
compression: boolean;
sniffOnStart: boolean;
sniffOnConnectionFault: boolean;
Expand Down
10 changes: 10 additions & 0 deletions packages/core/elasticsearch/core-elasticsearch-server/src/elasticsearch_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ export interface IElasticsearchConfig {
*/
readonly maxSockets: number;

/**
* The maximum number of idle sockets to keep open between Kibana and Elasticsearch. If more sockets become idle, they will be closed.
*/
readonly maxIdleSockets: number;

/**
* The timeout for idle sockets kept open between Kibana and Elasticsearch. If the socket is idle for longer than this timeout, it will be closed.
*/
readonly idleSocketTimeout: Duration;

/**
* Whether to use compression for communications with elasticsearch.
*/
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/monitoring/server/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,11 @@ describe('config schema', () => {
"healthCheck": Object {
"delay": "PT2.5S",
},
"idleSocketTimeout": "PT9M",
"ignoreVersionMismatch": false,
"logFetchCount": 10,
"logQueries": false,
"maxIdleSockets": 256,
"maxSockets": Infinity,
"pingTimeout": "PT30S",
"requestHeadersWhitelist": Array [
Expand Down