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 7 commits
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
36 changes: 14 additions & 22 deletions ...es/core/elasticsearch/core-elasticsearch-client-server-internal/src/agent_manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,35 +44,27 @@ describe('AgentManager', () => {
expect(httpsAgent).toEqual(mockedHttpsAgent);
});

it('provides Agents with a valid default configuration', () => {
const agentManager = new AgentManager();
const agentFactory = agentManager.getAgentFactory();
agentFactory({ url: new URL('http://elastic-node-1:9200') });
expect(HttpAgent).toBeCalledTimes(1);
expect(HttpAgent).toBeCalledWith({
keepAlive: true,
keepAliveMsecs: 1000,
maxFreeSockets: 256,
maxSockets: 256,
scheduling: 'lifo',
});
});

it('takes into account the provided configurations', () => {
const agentManager = new AgentManager({ maxFreeSockets: 32, maxSockets: 2048 });
const agentManager = new AgentManager();
const agentFactory = agentManager.getAgentFactory({
maxSockets: 1024,
maxTotalSockets: 1024,
scheduling: 'fifo',
});
agentFactory({ url: new URL('http://elastic-node-1:9200') });
expect(HttpAgent).toBeCalledTimes(1);
const agentFactory2 = agentManager.getAgentFactory({
maxFreeSockets: 10,
scheduling: 'lifo',
});
agentFactory2({ url: new URL('http://elastic-node-2:9200') });
expect(HttpAgent).toBeCalledTimes(2);
expect(HttpAgent).toBeCalledWith({
keepAlive: true,
keepAliveMsecs: 1000,
maxFreeSockets: 32,
maxSockets: 1024,
maxTotalSockets: 1024,
scheduling: 'fifo',
});
expect(HttpAgent).toBeCalledWith({
maxFreeSockets: 10,
scheduling: 'lifo',
});
});

it('provides Agents that match the URLs protocol', () => {
Expand All @@ -86,7 +78,7 @@ describe('AgentManager', () => {
expect(HttpsAgent).toHaveBeenCalledTimes(1);
});

it('provides the same Agent iif URLs use the same protocol', () => {
it('provides the same Agent if URLs use the same protocol', () => {
const agentManager = new AgentManager();
const agentFactory = agentManager.getAgentFactory();
const agent1 = agentFactory({ url: new URL('http://elastic-node-1:9200') });
Expand Down
26 changes: 6 additions & 20 deletions packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/agent_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,11 @@
* Side Public License, v 1.
*/

import { Agent as HttpAgent } from 'http';
import { Agent as HttpAgent, AgentOptions } from 'http';
rudolf marked this conversation as resolved.
Show resolved Hide resolved
import { Agent as HttpsAgent } from 'https';
import { ConnectionOptions, HttpAgentOptions } from '@elastic/elasticsearch';
import { ConnectionOptions } from '@elastic/elasticsearch';

const HTTPS = 'https:';
const DEFAULT_CONFIG: HttpAgentOptions = {
keepAlive: true,
keepAliveMsecs: 1000,
maxSockets: 256,
maxFreeSockets: 256,
scheduling: 'lifo',
};

export type NetworkAgent = HttpAgent | HttpsAgent;
export type AgentFactory = (connectionOpts: ConnectionOptions) => NetworkAgent;
Expand All @@ -39,12 +32,12 @@ export class AgentManager {
// Stores Http Agent instances
private httpStore: Set<HttpAgent>;

constructor(private agentOptions: HttpAgentOptions = DEFAULT_CONFIG) {
constructor() {
this.httpsStore = new Set();
this.httpStore = new Set();
}

public getAgentFactory(agentOptions?: HttpAgentOptions): AgentFactory {
public getAgentFactory(agentOptions?: AgentOptions): AgentFactory {
// a given agent factory always provides the same Agent instances (for the same protocol)
// we keep references to the instances at factory level, to be able to reuse them
let httpAgent: HttpAgent;
Expand All @@ -53,13 +46,7 @@ export class AgentManager {
return (connectionOpts: ConnectionOptions): NetworkAgent => {
if (connectionOpts.url.protocol === HTTPS) {
if (!httpsAgent) {
const config = Object.assign(
{},
DEFAULT_CONFIG,
this.agentOptions,
agentOptions,
connectionOpts.tls
);
Comment on lines -60 to -66
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually there was several levels:

  1. DEFAULT_CONFIG static defaults in code
  2. this.agentOptions optional constructor paramater that can set defaults for any agent created by the agent factories of this agent manager
  3. finally the kibana.yml defaults that are passed into the agent factories when they are created.

So now we just have (3)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const config = Object.assign({}, agentOptions, connectionOpts.tls);
httpsAgent = new HttpsAgent(config);
this.httpsStore.add(httpsAgent);
dereferenceOnDestroy(this.httpsStore, httpsAgent);
Expand All @@ -69,8 +56,7 @@ export class AgentManager {
}

if (!httpAgent) {
const config = Object.assign({}, DEFAULT_CONFIG, this.agentOptions, agentOptions);
httpAgent = new HttpAgent(config);
httpAgent = new HttpAgent(agentOptions);
this.httpStore.add(httpAgent);
dereferenceOnDestroy(this.httpStore, httpAgent);
}
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
3 changes: 3 additions & 0 deletions ...s/core/elasticsearch/core-elasticsearch-client-server-internal/src/cluster_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { ElasticsearchClientConfig } from '@kbn/core-elasticsearch-server';
import { ClusterClient } from './cluster_client';
import { DEFAULT_HEADERS, getDefaultHeaders } from './headers';
import { AgentManager } from './agent_manager';
import { duration } from 'moment';

const createConfig = (
parts: Partial<ElasticsearchClientConfig> = {}
Expand All @@ -27,6 +28,8 @@ const createConfig = (
sniffOnConnectionFault: false,
sniffInterval: false,
maxSockets: Infinity,
maxIdleSockets: 200,
idleSocketTimeout: duration('30s'),
compression: false,
requestHeadersWhitelist: ['authorization'],
customHeaders: {},
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": "PT1M",
"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: '60s' }),
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": "PT1M",
"ignoreVersionMismatch": false,
"logFetchCount": 10,
"logQueries": false,
"maxIdleSockets": 256,
"maxSockets": Infinity,
"pingTimeout": "PT30S",
"requestHeadersWhitelist": Array [
Expand Down