-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Introduce Elasticsearch service. #28344
Introduce Elasticsearch service. #28344
Conversation
Pinging @elastic/kibana-platform |
1fd23ee
to
ca86e0e
Compare
e7283b9
to
d6bbdb2
Compare
@@ -139,6 +140,40 @@ export class ConfigService { | |||
); | |||
} | |||
|
|||
if (ConfigClass.deprecations !== 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.
note: initial version of config deprecations support for new platform configs. Since this PR is presumably targeting 7.0.0 we may just drop already deprecated properties in ES plugin removing the immediate need in this and rather handle deprecation support in the follow-up.
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.
That'd be ideal!
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.
Created PR #29496
* @param req Request the `ScopedClusterClient` instance will be scoped to. | ||
*/ | ||
public asScoped(req: { headers?: Headers } = {}) { | ||
// It'd have been quite expensive to create and configure client for every incoming |
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.
note: I tried to explain in comment why scoped cluster client was implemented this way. I spent some time learning how ES plugin works currently and is being used and couldn't find a better way to achieve that, but happy to hear your suggestions.
Not very happy with this method being here though, still experimenting...
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.
Thanks for the description here, that's really helpful. I bet we could come up with a more sophisticated way to enable this, but the implementation you have here (along with the description) is pretty straightforward at least, so I'm not losing any sleep if this goes in.
* not only entries from standard `elasticsearch.*` yaml config, but also some Elasticsearch JS | ||
* client specific options like `keepAlive` or `plugins` (that eventually will be deprecated). | ||
*/ | ||
export type ElasticsearchClientConfig = Pick<ConfigOptions, 'keepAlive' | 'log' | 'plugins'> & |
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.
note: I'd drop log
and plugins
as soon as possible and provide more controlled way of extending client if we still want to support this.
| 'username' | ||
| 'password' | ||
> & { | ||
pingTimeout?: ElasticsearchConfig['pingTimeout'] | ConfigOptions['pingTimeout']; |
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.
note: we use Duration
type in new platform for all timeouts that we get from configs and I personally find it much nicer than plain numbers, so I'm including support for Duration
as well.
requestTimeout?: ElasticsearchConfig['requestTimeout'] | ConfigOptions['requestTimeout']; | ||
sniffInterval?: ElasticsearchConfig['sniffInterval'] | ConfigOptions['sniffInterval']; | ||
ssl?: Partial<ElasticsearchConfig['ssl']>; | ||
loggerContext?: string; |
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.
note: the only driver behind this loggerContext
is monitoring that uses logger tag (monitoring-ui
) that is different from cluster type (monitoring
). I don't like it, and rather always use "cluster type" as additional logger context/tag. It feels like I can break something by replacing monitoring-ui
with monitoring
, but I'll double check with monitoring team later and hopefully get rid of loggerContext
.
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.
@elastic/stack-monitoring how important is it for monitoring to have monitoring-ui
logging tag instead of just monitoring
(https://github.com/elastic/kibana/blob/master/x-pack/plugins/monitoring/common/constants.js#L12) for elastic search cluster client? Right now monitoring is the only plugin that uses logging tag that is different from the cluster name it creates (via plugins.elasticsearch.createCluster('monitoring')
) and we need to support additional property in ES service just for that. Would you be open to using just monitoring
logging tag instead?
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.
cc: @chrisronline
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 we can change it? @pickypg any historical insight here?
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 even know why we used monitoring-ui
versus monitoring
. So it seems safe to me.
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.
Great, do you need our help with that work @azasypkin?
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.
Great, do you need our help with that work @azasypkin?
@chrisronline, I'm going to just change value of LOGGING_TAG to monitoring
and update tests that depend on that. If that's fine for you then I don't need anything apart from review for the monitoring part once PR is ready :)
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.
Perfect. Sounds good!
|
||
// Original `ConfigOptions` defines `ssl: object` so we need something more specific. | ||
/** @internal */ | ||
export type ExtendedConfigOptions = ConfigOptions & |
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: this will go away as soon as we have proper types from es-js package.
.toPromise(); | ||
|
||
return { | ||
bwc: { config: defaultConfig }, |
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.
note: for console
plugin and healthcheck (only healthCheckDelay
)
return { | ||
bwc: { config: defaultConfig }, | ||
|
||
apiVersion: defaultConfig.apiVersion, |
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.
note: these are for ES plugin that exposes these fields to the client side, also timelion uses shardTimeout
for some reason.
|
||
createClient: (type: string, clientConfig: Partial<ElasticsearchClientConfig> = {}) => { | ||
return this.createClusterClient(type, { | ||
...(defaultConfig as ElasticsearchClientConfig), |
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.
note: I've noticed that in most of the places where we call createCluster
we pass default ES config and I don't really see any reason to not use this one to fill the gaps if consumer doesn't specify something. Thoughts?
ec1eb35
to
536f7d4
Compare
@@ -48,7 +48,7 @@ function numberToDuration(numberMs: number) { | |||
return momentDuration(numberMs); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
||
import { | ||
ProxyConfigCollection, | ||
getElasticsearchProxyConfig, | ||
createProxyRoute | ||
} from './server'; | ||
|
||
function filterHeaders(originalHeaders, headersToKeep) { |
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.
note: since it's the only place in Kibana that consumed filterHeaders
and setHeaders
I just moved it here and removed from ES plugin public interface.
Ideally we shouldn't have this logic in two places though...
src/server/config/complete.js
Outdated
@@ -74,6 +74,8 @@ export default async function (kbnServer, server, config) { | |||
return kbnServer.config; | |||
}); | |||
|
|||
server.decorate('server', 'core', kbnServer.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.
note: just a temporary location, need to find a better place to expose 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.
Is kbnServer.core
all of the capabilities we pass through via the legacy service?
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.
Yes, I used it as catch-all place for core -> legacy stuff (easy to grep for places that rely on core), do you think we should split into let's say kbnServer.elasticsearch
and friends?
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.
No, I think this makes sense. When we pass plugin capabilities through, I'd assume we'd pass them as a sibling property to core
, so it kind of mimics the core
/plugins
convention in the new platform.
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 left a couple questions, but I think this is really close.
'config sourced from: monitoring cluster' | ||
]); | ||
}); | ||
}); | ||
|
||
describe('Custom Headers Configuration', () => { | ||
it('Adds xpack.monitoring.elasticsearch.customHeaders if connected to production cluster', () => { | ||
it('Does not ad xpack.monitoring.elasticsearch.customHeaders if connected to production cluster', () => { |
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('Does not ad xpack.monitoring.elasticsearch.customHeaders if connected to production cluster', () => { | |
it('Does not add xpack.monitoring.elasticsearch.customHeaders if connected to production cluster', () => { |
retest |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
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.
LGTM
I didn't manually test this with the latest master merge, and this has conflicts at the moment. Otherwise, this looks good.
💚 Build Succeeded |
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.
LGTM for Stack Monitoring! Thanks for making these changes!
💚 Build Succeeded |
7.x/7.1.0: 5156829 |
💔 Build Failed |
💔 Build Failed |
In the scope of this PR I'm going to introduce
ElasticsearchService
and related classes based on the legacy Elasticsearch plugin. We'll still keep the plugin and interface it exposes for the BWC reasons, but internally it should rely on start contract provided byElasticsearchService
.Initially this PR will include additional changes in
@kbn/config-schema
and deprecation handling code that is required for this migration to happen, but I'll move them into separate PRs when everything in place and works as expected.Quite a lot of places are relying on the custom "cluster clients" via providing "plugin" config property so we'll need to keep this escape hatch.
NOTE: One should use--xpack.reporting.enabled=false
to test this PR for now.Related to #12442
Fixes #18841
Dev Docs
Introduce Elasticsearch service
We've introduced an Elasticsearch service to the new experimental plugin platform. This service allows new platform plugins to get
elasticsearch-js
clients for querying Elasticsearch on the server.To access the new Elasticsearch service from a new platform plugin:
Clients are exposed via an Observable interface to reflect the possibility that the connection configuration changes while the server is running.