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.

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

Create HTTP Agent manager #137748

Merged
merged 39 commits into from
Sep 14, 2022
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
4265fe3
Create HTTP Agent factory
gsoldevila Aug 1, 2022
b3a2d1c
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 2, 2022
2f0550b
Properly extract agent options
gsoldevila Aug 2, 2022
24e731d
Use independent Agent for preboot
gsoldevila Aug 2, 2022
bf19f70
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 2, 2022
941f5db
Create AgentManager to obtain factories
gsoldevila Aug 3, 2022
ca8d434
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 3, 2022
93d033c
Make client type mandatory, fix outdated mocks
gsoldevila Aug 4, 2022
0e8591c
Temporarily force new Agent creation
gsoldevila Aug 4, 2022
582e4e7
Revert changes in utils
gsoldevila Aug 4, 2022
11431a3
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 4, 2022
a3b153d
Add correct defaults for Agent Options, support proxy agent.
gsoldevila Aug 4, 2022
46232e7
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 4, 2022
f891b0a
Forgot to push package.json
gsoldevila Aug 4, 2022
8270d13
Add hpagent dependency in BUILD.bazel
gsoldevila Aug 4, 2022
a8b7915
Get rid of hpagent (proxy param is not exposed in kibana.yml)
gsoldevila Aug 25, 2022
0954407
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 25, 2022
210a326
Remove hpagent from BUILD.bazel
gsoldevila Aug 25, 2022
df464cc
Use different agents for normal Vs scoped client
gsoldevila Aug 26, 2022
9acb1dc
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 26, 2022
41fe166
Fix Agent constructor params
gsoldevila Aug 29, 2022
1aa1a4c
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 29, 2022
8149c53
Fix incorrect access to err.message
gsoldevila Aug 29, 2022
e2b53d6
Use separate Agent for scoped client
gsoldevila Aug 29, 2022
fca3af2
Create different agents for std vs scoped
gsoldevila Aug 29, 2022
0d0ba54
Provide different Agent instances if config differs
gsoldevila Aug 29, 2022
5dcb57a
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 29, 2022
5038277
Create a new Agent for each ES Client
gsoldevila Aug 30, 2022
0ff81aa
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Aug 30, 2022
01a4d17
Restructure agent store. Add UTs
gsoldevila Sep 5, 2022
53a25f1
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Sep 5, 2022
afe9f35
Remove obsolete comment
gsoldevila Sep 5, 2022
5aae94b
Simplify AgentManager store structure (no type needed)
gsoldevila Sep 8, 2022
74a4886
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Sep 8, 2022
72d8388
Fine tune client_config return type
gsoldevila Sep 8, 2022
290bcff
Misc enhancements following PR comments
gsoldevila Sep 9, 2022
86b14b9
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Sep 9, 2022
6331e35
Fix missing param in cli_setup/utils
gsoldevila Sep 9, 2022
9437590
Merge branch 'main' into kbn-137734-http-agent-factory
gsoldevila Sep 14, 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
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
export { ScopedClusterClient } from './src/scoped_cluster_client';
export { ClusterClient } from './src/cluster_client';
export { configureClient } from './src/configure_client';
export { AgentManager } from './src/agent_manager';
export { getRequestDebugMeta, getErrorMessage } from './src/log_query_and_deprecation';
export {
PRODUCT_RESPONSE_HEADER,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import { AgentManager } from './agent_manager';
import { Agent as HttpAgent } from 'http';
import { Agent as HttpsAgent } from 'https';

jest.mock('http');
jest.mock('https');

describe('AgentManager', () => {
let logger: ReturnType<typeof loggingSystemMock.createLogger>;

beforeEach(() => {
logger = loggingSystemMock.createLogger();
});

afterEach(() => {
const HttpAgentMock = HttpAgent as jest.Mock<HttpAgent>;
const HttpsAgentMock = HttpsAgent as jest.Mock<HttpsAgent>;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I think these two lines can be extracted to the top level describeblock, or even to depth 0 of the file.

HttpAgentMock.mockClear();
HttpsAgentMock.mockClear();
});

describe('provides agent factories', () => {
it('with a valid default configuration', () => {
const agentManager = new AgentManager(logger);
const agentFactory = agentManager.getAgentFactory('test');
agentFactory({ url: new URL('http://elastic-node-1:9200') });
expect(HttpAgent).toBeCalledTimes(1);
expect(HttpAgent).toBeCalledWith({
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I would mock the return of HttpAgent / HttpsAgent and assert the return value of agentFactory(...) accordingly.

keepAlive: true,
keepAliveMsecs: 50000,
maxFreeSockets: 256,
maxSockets: 256,
scheduling: 'lifo',
});
});

it('takes into account the configuration provided in the constructor', () => {
const agentManager = new AgentManager(logger);
const agentFactory = agentManager.getAgentFactory('test', {
maxSockets: 1024,
scheduling: 'fifo',
});
agentFactory({ url: new URL('http://elastic-node-1:9200') });
expect(HttpAgent).toBeCalledTimes(1);
expect(HttpAgent).toBeCalledWith({
keepAlive: true,
keepAliveMsecs: 50000,
maxFreeSockets: 256,
maxSockets: 1024,
scheduling: 'fifo',
});
});

it('which are different at each call', () => {
const agentManager = new AgentManager(logger);
const agentFactory1 = agentManager.getAgentFactory('test');
const agentFactory2 = agentManager.getAgentFactory('test');
expect(agentFactory1).not.toEqual(agentFactory2);
});

it('throws an error when an Agent factory is requested using undici params', () => {
const agentManager = new AgentManager(logger);
expect(() => {
agentManager.getAgentFactory('anotherTest', { keepAliveTimeout: 2000 });
}).toThrowError();
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: toThrowErrorMatchingInlineSnapshot()

});

describe('when configured with a custom factory', () => {
it('uses the provided factory', () => {
const customAgent = new HttpsAgent({ maxSockets: 32 });
expect(HttpsAgent).toBeCalledTimes(1);
const factory = () => customAgent;
const agentManager = new AgentManager(logger);
const agentFactory = agentManager.getAgentFactory('test', factory);
const agent = agentFactory({ url: new URL('http://elastic-node-1:9200') });
expect(HttpsAgent).toBeCalledTimes(1);
expect(agent).toEqual(customAgent);
});
});

describe('one agent factory', () => {
it('provides Agents that match the URLs protocol', () => {
const agentManager = new AgentManager(logger);
const agentFactory = agentManager.getAgentFactory('test');
agentFactory({ url: new URL('http://elastic-node-1:9200') });
expect(HttpAgent).toHaveBeenCalledTimes(1);
expect(HttpsAgent).toHaveBeenCalledTimes(0);
agentFactory({ url: new URL('https://elastic-node-3:9200') });
expect(HttpAgent).toHaveBeenCalledTimes(1);
expect(HttpsAgent).toHaveBeenCalledTimes(1);
});

it('provides the same Agent iif URLs use the same protocol', () => {
const agentManager = new AgentManager(logger);
const agentFactory = agentManager.getAgentFactory('test');
const agent1 = agentFactory({ url: new URL('http://elastic-node-1:9200') });
const agent2 = agentFactory({ url: new URL('http://elastic-node-2:9200') });
const agent3 = agentFactory({ url: new URL('https://elastic-node-3:9200') });
const agent4 = agentFactory({ url: new URL('https://elastic-node-4:9200') });

expect(agent1).toEqual(agent2);
expect(agent1).not.toEqual(agent3);
expect(agent3).toEqual(agent4);
});

it('dereferences an agent instance when the agent is closed', () => {
const agentManager = new AgentManager(logger);
const agentFactory = agentManager.getAgentFactory('test');
const agent = agentFactory({ url: new URL('http://elastic-node-1:9200') });
// eslint-disable-next-line dot-notation
expect(agentManager['agentStore']['test']['http:'][0]).toEqual(agent);
agent.destroy();
// eslint-disable-next-line dot-notation
expect(agentManager['agentStore']['test']['http:'][0]).toBeUndefined();
});
});

describe('two agent factories', () => {
it('never provide the same Agent instance even if they use the same type', () => {
const agentManager = new AgentManager(logger);
const agentFactory1 = agentManager.getAgentFactory('test');
const agentFactory2 = agentManager.getAgentFactory('test');
const agent1 = agentFactory1({ url: new URL('http://elastic-node-1:9200') });
const agent2 = agentFactory2({ url: new URL('http://elastic-node-1:9200') });
expect(agent1).not.toEqual(agent2);
Comment on lines +119 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: that's true in a test environment with

jest.mock('http');
jest.mock('https');

I would expect both to be undefined given the constructor has been mocked without specifying return values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest seems to be mocking the class automatically, and returning an instance that mocks all its methods too.

});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { Agent as HttpAgent } from 'http';
import { Agent as HttpsAgent } from 'https';
import { ConnectionOptions, HttpAgentOptions, UndiciAgentOptions } from '@elastic/elasticsearch';
import { Logger } from '@kbn/logging';

const HTTP = 'http:';
const HTTPS = 'https:';

export type Protocol = typeof HTTP | typeof HTTPS;
export type NetworkAgent = HttpAgent | HttpsAgent;
export type AgentFactory = (connectionOpts: ConnectionOptions) => NetworkAgent;

/**
* Allows obtaining Agent factories, which can then be fed into elasticsearch-js's Client class.
* Ideally, we should obtain one Agent factory for each ES Client class.
* This allows using the same Agent across all the Pools and Connections of the Client (one per ES node).
*
* Agent instances are stored internally to allow collecting metrics (nbr of active/idle connections to ES).
*
* Using the same Agent factory across multiple ES Client instances is strongly discouraged, cause ES Client
* exposes methods that can modify the underlying pools, effectively impacting the connections of other Clients.
* @internal
**/
export class AgentManager {
/**
* Store Agent instances by type and protocol, e.g.:
*
* data: {
* 'http:': [agentInstance1, agentInstance2],
* 'https:': [agentInstance3, agentInstance4]
* },
* monitoring: {
* 'http:': [],
* 'https:': [agentInstance5]
* }
*/
private agentStore: Record<string, Record<Protocol, Array<NetworkAgent | undefined>>>;
Copy link
Contributor

@pgayvallet pgayvallet Sep 7, 2022

Choose a reason for hiding this comment

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

I think I would use a Map here instead of a Record, at least for the top-level one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion for or against either of the two, I'll switch to Map.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why there could be several agents per type.protocol.

Like in your example why does data.http have agentInstance1 AND agentInstance2 shouldn't the factory always return the same agent?

It seems like this is because the AgentManager can create multiple factories, one factory per type. But if we're already creating a new agent per type why do we also need a new factory per type?

In my head I was kinda imagining that the agentStore would be something like a WeakSet with type+protocol keys and NetworkAgent values.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #137748 (comment), I think that's the same question

Copy link
Contributor Author

@gsoldevila gsoldevila Sep 8, 2022

Choose a reason for hiding this comment

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

Pierre's right, please refer to the comment above.

At the moment I am logging a warning if multiple factories of the same type are requested.
IMO if we want to ensure there's only one Agent instance per type we can work at a higher level and have a ClusterClient store instead.

FWIW, if I'm not mistaken the different data requests go through the 'data' instance managed internally by the elasticsearch service, so at the end of the day we will not have lots of instances of each type:

  • One data instance, one data-scoped.
  • A bunch of instances of other types (enroll, ping, authenticate, ...) that are used in the interactive mode only.
  • A monitoring instance (used by the monitoring plugin).

Also, note that if we use WeakSets or WeakMaps we can't really iterate through them to aggregate metrics for monitoring on the next step.

Copy link
Contributor Author

@gsoldevila gsoldevila Sep 8, 2022

Choose a reason for hiding this comment

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

Now that I think about it, perhaps we could throw an Exception if we try to fetch more than one factory for the same type. That would greatly simplify the store structure and would encourage users to reuse the existing ES Client instance.

It would cause some tests to fail, as they systematically create instances using the same type, but we might be able to workaround it for tests. I'll give it a try.

UPDATE: I'm afraid we can't really do that without impacting the interactive-setup plugin.

private logger: Logger;

constructor(
logger: Logger,
private defaultConfig: HttpAgentOptions = {
keepAlive: true,
keepAliveMsecs: 50000,
maxSockets: 256,
maxFreeSockets: 256,
scheduling: 'lifo',
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: extract the default config to a constant external to the constructor.

) {
this.logger = logger;
this.agentStore = {};
}

public getAgentFactory(
type: string,
agentOptions?: HttpAgentOptions | UndiciAgentOptions | AgentFactory | false
): AgentFactory {
if (isAgentFactory(agentOptions)) {
// use the user-provided factory directly
return agentOptions;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the client's type def:

export declare type agentFn = (opts: ConnectionOptions) => any;

Nice... Helps a lot...

Do you know what an agentFn is actually supposed to return? Can we wrap the return or fn with a HOF somehow to wire our logic / would it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agentFn is supposed to return Agent instances.
I created the equivalent AgentFactory, and the NetworkAgent types that are a bit more specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it doesn't make sense to create an agent factory and pass in a custom factory, then we should just use the custom factory. I suspect it might be because you wanted to match the Elasticsearch-js config options type which includes being able to specify a factory? But plugins cannot create Elasticsearch-js clients with custom factories because we only expose a limited set of options https://github.com/elastic/kibana/blob/main/packages/core/elasticsearch/core-elasticsearch-server/src/client/client_config.ts#L16-L33

To be defensive, could we just throw if agent options is a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ATM the agent?: HttpAgentOptions | UndiciAgentOptions | agentFn | false; can only be HttpAgentOptions according to how we build it in parseClientOptions.

How about we make the internal API typings more specific and adjusted to reality?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you can restrict the types to exclude agentFn it would work, but given parseClientOptions returns a plain ClientOptions, I don't think you would be able without some type shenanigans or force-casting, so throwing seems fine too (at least preferable than pass-through-ing as the impl currently does).


const agentConfig = assertValidAgentConfig(agentOptions);

// a given agent factory (of a given type) always provides the same Agent instances (for the same protocol)
// we keep the indexes for each protocol, so that we can access them later on, when the factory is invoked
const { httpInstanceIndex, httpsInstanceIndex } = this.initTypeStore(type);

return (connectionOpts: ConnectionOptions): NetworkAgent => {
const agents = this.getAgentInstances(type, connectionOpts);

// when the factory is called for a given connection (and protocol), we use the appropriate index
const agentIndex = isHttps(connectionOpts) ? httpsInstanceIndex : httpInstanceIndex;

let agent = agents[agentIndex];

if (!agent) {
agent = this.createAgent(agentConfig, connectionOpts);

// Allow GC'ing the Agent after it has been terminated
const doDestroy = agent.destroy.bind(agent);
agent.destroy = () => {
doDestroy();
agents[agentIndex] = undefined;
};

// add the new Agent instance to the list of instances of that type
agents[agentIndex] = agent;
}

return agent;
};
}

private initTypeStore(type: string): { httpInstanceIndex: number; httpsInstanceIndex: number } {
let agentsOfType = this.agentStore[type];

if (!agentsOfType) {
agentsOfType = {
[HTTP]: [],
[HTTPS]: [],
};

this.agentStore[type] = agentsOfType;
} else {
this.logger.warn(
`An Agent factory for type ${type} has already been retrieved.
This is not optimal since the existing instance cannot be reused.
Please consider reusing the previously retrieved factory instead`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't it be reused, again? it's because of the destroy call problem, or is it something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, if users request an agent of a given type a second time, they are likely going to build a new ClusterClient() (or ES Client) with it.
If they do, they’ll have different pools with the same underlying agent.
Then, when they dispose of their Client, they will be closing and destroy()ing the underlying Agent, impacting other ES Client instances.

);
}

const httpInstanceIndex = agentsOfType[HTTP].length;
const httpsInstanceIndex = agentsOfType[HTTPS].length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a scenario where these two indices would be different, given we're creating the entry for both protocols every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we're not, we're creating the entry for one or the other protocol depending on the elasticsearch.hosts configuration. We could be connecting to 2 nodes through HTTPS and to one node through HTTP for instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could be connecting to 2 nodes through HTTPS and to one node through HTTP for instance.

Still, in the implementation you're creating one entry per call for a given type in both arrays, isn't that correct? so atm, the length of both is strictly equal, unless I missed something in the impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment is outdated now, but FWIW I was pushing the newly created instances to either the http array or the https array, but not both at the same time.

Now I've simplified the store structure, but we still have the 2 arrays, and the code pushes to one or to the other, but not to both. So we could end up having 3 Agents of type Https and 1 of type Http.

The idea is that we provide the AgentFactory to the ES Client class, who loops through all the elasticsearch.hosts and creates an HttpConnection for each of them.
Internally, each HttpConnection calls the agentFn to obtain an instance of Agent.
Then, depending on the protocol of elasticsearch.hosts[i], the AgentFactory will provide an instance of HttpAgent or one of HttpsAgent, and it will store this instance in the appropriate queue in the manager.

Finally, note that for hosts using the same protocol the same instance will be provided.


agentsOfType[HTTP].push(undefined);
agentsOfType[HTTPS].push(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the problem at stake here, but I'm not a big fan of pushing indefined's into a supposedly typed array here.

I think I would have used a map with a {type}-{protocol}-{index}, and/or kept the current index (indices?) per type stored somewhere to avoid that.

But this is an personal implementation preference here, what you did works (as long as we don't exceed MAX_SAFE_INTEGER agents instantiated for a given type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, pushing undefined just to book a slot in the isn't the nicest code ever, I'll give you that 😛 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realised that type was of no use to the AgentManager ATM, so I updated the PR with a much simpler version of the Agent, which only has a couple of arrays now:

  • one for Http agent instances
  • one for Https agent instances

return { httpInstanceIndex, httpsInstanceIndex };
}

private getAgentInstances(
type: string,
connectionOpts: ConnectionOptions
): Array<NetworkAgent | undefined> {
let protocol: Protocol;

switch (connectionOpts.url.protocol) {
case 'http:':
protocol = HTTP;
break;
case 'https:':
protocol = HTTPS;
break;
default:
throw new Error(`Unsupported protocol: '${connectionOpts.url.protocol}'`);
}

return this.agentStore[type][protocol];
}

private createAgent(agentConfig: HttpAgentOptions, connectionOpts: ConnectionOptions) {
const config = Object.assign({}, this.defaultConfig, agentConfig, connectionOpts.tls);
return isHttps(connectionOpts) ? new HttpsAgent(config) : new HttpAgent(config);
}
}

const isHttps = (connectionOpts: ConnectionOptions): boolean => {
return connectionOpts.url.protocol === HTTPS;
};

const assertValidAgentConfig = (
agentOptions?: HttpAgentOptions | UndiciAgentOptions | false
): HttpAgentOptions => {
if (!agentOptions) {
return {};
} else if (isHttpAgentOptions(agentOptions)) {
return agentOptions;
}

throw new Error('Unsupported agent options: UndiciAgentOptions');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning (would we be able) to support it at some point? IIRC, migrating to Undici is still something we may want, so we're adding an additional road block here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

elastic-transport-js's UndiciConnection does not currently accept agentFn as input configuration, aka it does not accept factories.

Thus, I couldn't simply pass in the factories provided by AgentManager.
Since we're still using standard HttpConnection ATM I didn't want to go too far with my PR.

That's why I made agentManager parameter optional too. If we want to switch to undici we can simply stop passing an agent manager, and undici's default logic for creating its agent will apply.

I haven't gotten into the internals of unidici, but if we then consider appropriate to use agent factories with undici as well, we'll have to:

  1. Support factories in elastic-transport-js's UndiciConnection class.
  2. Update the AgentManager to support undici mode as well.

Copy link
Contributor

@rudolf rudolf Sep 8, 2022

Choose a reason for hiding this comment

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

I think this issue covers everything we want elastic/elasticsearch-js#1740

Ideally we don't want to have to create this factory / single agent workaround, the client should allow us to set max sockets and get diagnostics as part of its public API. So I hope when we move to undici that we'd just remove the AgentManager altogether and get everything out of the box 🌈

};

const isAgentFactory = (
agentOptions?: HttpAgentOptions | UndiciAgentOptions | AgentFactory | false
): agentOptions is AgentFactory => {
return typeof agentOptions === 'function';
};

const isHttpAgentOptions = (
opts: HttpAgentOptions | UndiciAgentOptions
): opts is HttpAgentOptions => {
return (
!('keepAliveTimeout' in opts) &&
!('keepAliveMaxTimeout' in opts) &&
!('keepAliveTimeoutThreshold' in opts) &&
!('pipelining' in opts) &&
!('maxHeaderSize' in opts) &&
!('connections' in opts)
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,13 @@ describe('ClusterClient', () => {
expect(configureClientMock).toHaveBeenCalledTimes(2);
expect(configureClientMock).toHaveBeenCalledWith(config, {
logger,
agentManager: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: add test to ensure that the agent manager is passed down to the configureClient calls when present.

type: 'custom-type',
getExecutionContext: getExecutionContextMock,
});
expect(configureClientMock).toHaveBeenCalledWith(config, {
logger,
agentManager: undefined,
type: 'custom-type',
getExecutionContext: getExecutionContextMock,
scoped: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { ScopedClusterClient } from './scoped_cluster_client';
import { DEFAULT_HEADERS } from './headers';
import { createInternalErrorHandler, InternalUnauthorizedErrorHandler } from './retry_unauthorized';
import { createTransport } from './create_transport';
import { AgentManager } from './agent_manager';

const noop = () => undefined;

Expand All @@ -47,25 +48,33 @@ export class ClusterClient implements ICustomClusterClient {
authHeaders,
getExecutionContext = noop,
getUnauthorizedErrorHandler = noop,
agentManager,
}: {
config: ElasticsearchClientConfig;
logger: Logger;
type: string;
authHeaders?: IAuthHeadersStorage;
getExecutionContext?: () => string | undefined;
getUnauthorizedErrorHandler?: () => UnauthorizedErrorHandler | undefined;
agentManager?: AgentManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

May we want to make this mandatory instead of optional?

Copy link
Contributor Author

@gsoldevila gsoldevila Sep 7, 2022

Choose a reason for hiding this comment

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

Having it optional is an easy way to fallback to the previous behavior (as per my comment above).

Copy link
Contributor

Choose a reason for hiding this comment

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

when an agentManager is not supplied I'd say our clients no longer behave as expected e.g. the meaning of maxSockets will change. It's only core using this constructor so it would be easy to change if we switch to undici.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I'll make it mandatory.

}) {
this.config = config;
this.authHeaders = authHeaders;
this.getExecutionContext = getExecutionContext;
this.getUnauthorizedErrorHandler = getUnauthorizedErrorHandler;

this.asInternalUser = configureClient(config, { logger, type, getExecutionContext });
this.asInternalUser = configureClient(config, {
logger,
type,
getExecutionContext,
agentManager,
});
this.rootScopedClient = configureClient(config, {
logger,
type,
getExecutionContext,
scoped: true,
agentManager,
});
}

Expand Down
Loading