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

Elasticsearch config run-time updates #40188

Closed
mshustov opened this issue Jul 2, 2019 · 7 comments
Closed

Elasticsearch config run-time updates #40188

mshustov opened this issue Jul 2, 2019 · 7 comments
Labels
chore Feature:New Platform stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Jul 2, 2019

In New Platform createCluster, a part of ElasticSearchService, requires to pass the whole config to create a new client. This leads to config leakage.
Probably that was done because the config is a stream and

  • Either we should rely only on the very first value, as other cluster clients do. in this case, createCluster returns created client immediately. already implemented
  • Or expose the result of createCluster method as another stream of clients. That makes all the calling code asynchronous and may complicate migration.

@elastic/kibana-platform do we want to support client recreation when elasticsearch config was updated? we need to unify all client contracts and decided whether we expose them as streams or imperative sync calls.

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Jul 2, 2019
@epixa
Copy link
Contributor

epixa commented Jul 2, 2019

do we want to support client recreation when elasticsearch config was updated?

For the legacy elasticsearch-js, I think we have to do this to support log rotation. As far as I know (but I could be wrong), logging configuration for the legacy esjs client is handled via constructor arguments, so when someone sends a SIGINTSIGHUP to Kibana to reload log configuration using something like logrotate, we actually need to create a new ES client.

This won't be necessary when things switch over to the new elasticsearch-js client which doesn't have built in logging and instead relies on you hooking into an event emitter.

@mshustov mshustov changed the title CreateCluster shouldn't require the whole Elasticsearch config Elasticsearch config run-time updates Jul 3, 2019
@azasypkin
Copy link
Member

For the legacy elasticsearch-js, I think we have to do this to support log rotation. As far as I know (but I could be wrong), logging configuration for the legacy esjs client is handled via constructor arguments, so when someone sends a SIGINT to Kibana to reload log configuration using something like logrotate, we actually need to create a new ES client.

IIRC, during SIGHUP we reload only logging.* config that will indirectly affect ClusterClient (and hence underlying ES JS client) logging since we rely on Good/even-better under the hood, but that doesn't require a new ES client and we don't re-create it.

The only ES client specific logging option is elasticsearch.logQueries and it's not updated during SIGHUP currently.

@epixa
Copy link
Contributor

epixa commented Jul 3, 2019

If that is indeed the case, then ignore my comment.

@mshustov
Copy link
Contributor Author

mshustov commented Jul 5, 2019

What config options are subjects for updating @epixa @joshdover ? If it is only logQueries, perhaps the right approach is to reflect the fact that clients cannot be re-created with new config and expose client instances as a part of Elasticsearch Service contract:

- legacy: { config$ }
- adminClient$: clients$.pipe(map(clients => clients.adminClient)),
- dataClient$: clients$.pipe(map(clients => clients.dataClient)),
+ legacy: { config }
+ adminClient,
+ dataClient,

And support logQueries update separately.
Legacy version of elasticsaech-js library supports passing custom Logger, which we can setup to receive logQueries$ stream:

function getLoggerClass(log: Logger, logQueries$) {
  return class ElasticsearchClientLogging {
    private logQueries: boolean;
    constructor(){
      this.logQueries = false;
      logQueries$.subscribe(logQueries => this.logQueries = logQueries)
    }

@mshustov
Copy link
Contributor Author

mshustov commented Jul 5, 2019

How new elasticsearch-js implements logging https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/7.x/observability.html

@azasypkin
Copy link
Member

Side note: it doesn't matter too much at this point, but once/if we migrate to the new logging system/config we may not need elasticsearch.logQueries at all and instead rely on different "logging contexts" supported by the core logging sub-system and get reload-during-SIGHUP feature for free:

# equals to elasticsearch.logQueries: true
loggers:
  - context: elasticsearch-queries
    level: trace

# equals to  elasticsearch.logQueries: false
loggers:
  - context: elasticsearch-queries
    level: off

@mshustov
Copy link
Contributor Author

it doesn't matter too much at this point, but once/if we migrate to the new logging system/config we may not need elasticsearch.logQueries at all and instead rely on different "logging contexts" supported by the core logging sub-system and get reload-during-SIGHUP feature for free:

created #41956

@joshdover joshdover added the stale Used to mark issues that were closed for being stale label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:New Platform stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants