-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Create HTTP Agent manager #137748
Conversation
@@ -172,13 +174,16 @@ export class ElasticsearchService | |||
clientConfig: Partial<ElasticsearchClientConfig> = {} | |||
) { | |||
const config = mergeConfig(baseConfig, clientConfig); | |||
|
|||
// tweak config to inject agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review: remove this obsolete comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:doit:
Pinging @elastic/kibana-core (Team:Core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda hard to say without concrete usages being added in the PR, but overall the implementation looks good to me.
A bunch of questions and NITs before a second pass:
}: { | ||
config: ElasticsearchClientConfig; | ||
logger: Logger; | ||
type: string; | ||
authHeaders?: IAuthHeadersStorage; | ||
getExecutionContext?: () => string | undefined; | ||
getUnauthorizedErrorHandler?: () => UnauthorizedErrorHandler | undefined; | ||
agentManager?: AgentManager; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -83,11 +83,13 @@ describe('ClusterClient', () => { | |||
expect(configureClientMock).toHaveBeenCalledTimes(2); | |||
expect(configureClientMock).toHaveBeenCalledWith(config, { | |||
logger, | |||
agentManager: undefined, |
There was a problem hiding this comment.
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.
* 'https:': [agentInstance5] | ||
* } | ||
*/ | ||
private agentStore: Record<string, Record<Protocol, Array<NetworkAgent | undefined>>>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
@@ -172,13 +174,16 @@ export class ElasticsearchService | |||
clientConfig: Partial<ElasticsearchClientConfig> = {} | |||
) { | |||
const config = mergeConfig(baseConfig, clientConfig); | |||
|
|||
// tweak config to inject agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:doit:
>; | ||
} | ||
|
||
const createPrebootContractMock = () => { | ||
const prebootContract: MockedElasticSearchServicePreboot = { | ||
config: { hosts: [], credentialsSpecified: false }, | ||
createClient: jest.fn(), | ||
createClient: jest.fn((type: string) => elasticsearchClientMock.createCustomClusterClient()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: no need to specify the type: string
parameter if unused to match the signature (same L70)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS complains with the following message if I don't:
Type '[]' is not assignable to type '[type: string, clientConfig?: Partial<ElasticsearchClientConfig> | undefined]'.
Source has 0 element(s) but target requires 1.ts(2322)
index.d.ts(226, 14): The expected type comes from property 'createClient' which is declared here on type 'MockedElasticSearchServicePreboot'
@@ -126,7 +126,7 @@ export class SavedObjectsSyncService { | |||
|
|||
return taskInstance; | |||
} catch (e) { | |||
this.log.error(`Error running task: ${SAVED_OBJECTS_SYNC_TASK_ID}, `, e?.message() ?? e); | |||
this.log.error(`Error running task: ${SAVED_OBJECTS_SYNC_TASK_ID}, `, e?.message ?? e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, not really.
It's a bug in the code that prevents from logging the caught error though, and causes a new Exception to be thrown.
I uncovered it during some tests.
I can create a separate PR if needed. @elastic/ml-ui WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a typo.
I'm happy for this to be fixed in this PR, no need to create a new one.
Thanks for spotting and fixing it.
const HttpAgentMock = HttpAgent as jest.Mock<HttpAgent>; | ||
const HttpsAgentMock = HttpsAgent as jest.Mock<HttpsAgent>; |
There was a problem hiding this comment.
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 describe
block, or even to depth 0 of the file.
agentFactory({ url: new URL('http://elastic-node-1:9200') }); | ||
expect(HttpAgent).toBeCalledTimes(1); | ||
expect(HttpAgent).toBeCalledWith({ |
There was a problem hiding this comment.
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.
const agentManager = new AgentManager(logger); | ||
expect(() => { | ||
agentManager.getAgentFactory('anotherTest', { keepAliveTimeout: 2000 }); | ||
}).toThrowError(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: toThrowErrorMatchingInlineSnapshot()
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM
* 'https:': [agentInstance5] | ||
* } | ||
*/ | ||
private agentStore: Record<string, Record<Protocol, Array<NetworkAgent | undefined>>>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, onedata-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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the direction this is going, it's already a lot simpler 🎉
packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/agent_manager.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisited implementation looks way better to me. LGTM on my side, but we should wait for @rudolf's approval too.
A few nits:
return new ClusterClient({ | ||
config, | ||
logger: this.coreContext.logger.get('elasticsearch'), | ||
type, | ||
authHeaders: this.authHeaders, | ||
getExecutionContext: () => this.executionContextClient?.getAsHeader(), | ||
getUnauthorizedErrorHandler: () => this.unauthorizedErrorHandler, | ||
agentManager: this.agentManager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: update
Line 124 in 3508350
describe('#createClient', () => { |
ClusterClient
constructor
): ClientOptions & { agent: HttpAgentOptions } { | ||
const clientOptions: ClientOptions & { agent: HttpAgentOptions } = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I would introduce an explicit type instead of an inline def
export type ParsedClientOptions = Omit<ClientOptions, 'agent'> & { agent: HttpAgentOptions }
packages/core/elasticsearch/core-elasticsearch-client-server-internal/src/agent_manager.ts
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Fixes #137734
Part of #134362
The PR defines a class to better control which instances of HTTP(s)
Agent
are created.