[Core] Don't expose Elasticsearch client as Observable

[rudolf]

https://github.com/elastic/kibana/blob/6042a01f385078587e8e09418ef07f69ec2dadf7/rfcs/text/0007_lifecycle_unblocked.md#3-core-should-not-expose-apis-as-observables

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[mshustov]

In this PR we can separate:

• internal & public interfaces.
  That would allow us to continue using observables for request handler context.
  [RFC][skip-ci] Prevent plugins from blocking Kibana startup #45796 (comment)
  Do we want to follow this pattern? On the one hand, I'm confused that we handle scoped clients differently in API and context.
  If we assume that a plugin calling elasticsearch API couldn't be affected by ES client update, why it's so important for a request handler context?

interface ElasticsearchPublicInterface {
  createClient: (type: string, clientConfig?..) => ICustomClusterClient;
  adminClient: IClusterClient;
  dataClient: IClusterClient;
}
interface ElasticsearchInternalInterface {
  legacy: {config$: ..};
  adminClient$: Observable<IClusterClient>;
  dataClient$: Observable<IClusterClient>;
}

• separate IClusterClient & ICustomClusterClient interfaces. If a plugin controls cluster client creation, it should clean up afterward. However, that's not true for the adminClient & dataClient, which are controlled by the core and plugins must not manipulate their state. Closes Plugins can close Elasticsearch clients and break other plugins #48963

interface IClusterClient {
  callAsInternalUser: (...) => any;
  asScoped: (req) => IScopedClusterClient;
}

interface ICustomClusterClient extends IClusterClient{
  close: () => void;
}

• optional. we have to duplicate the logic for adminClient& dataClient. Does it make sense to switch to the sindle client already now? Closes Core Elasticsearch: remove "data" and "admin" clients and expose single client #49870

// SetupContract before
adminClient: {
  callAsInternalUser(){...} ,
  asScoped(request) {
    return {
      callAsInternalUser(){...}
      callAsCurrentUser(){...}
    },
}
dataClient: {
  callAsInternalUser(){...} , 
  asScoped(request) {
    return {
      callAsInternalUser(){...}
      callAsCurrentUser(){...}
    },
}
// SetupContract after
  callAsInternalUser(){...}
  asScoped(request) {
    return {
      callAsInternalUser(){...}
      callAsCurrentUser(){...}
    }
  }

[rudolf]

In this PR we can separate:

• internal & public interfaces.
  That would allow us to continue using observables for request handler context.
  #45796 (comment)
  Do we want to follow this pattern? On the one hand, I'm confused that we handle scoped clients differently in API and context.
  If we assume that a plugin calling elasticsearch API couldn't be affected by ES client update, why it's so important for a request handler context?

I agree, I don't see any reason we should give a different client to the request context than what we give to plugins.

From context rfc:

3. Functionality in a handler should be "fixed" for the life of that
   handler's context rather than changing configuration under the hood in
   mid-execution. For example, while Elasticsearch clients can technically
   be replaced throughout the course of the Kibana process, an HTTP route
   handler should be able to depend on their being a consistent client for its
   own shorter lifespan.

I think the reason this was specified was so that code in a context handler doesn't need to deal with observables or promises, a context handler should get a ready-to-use client. @joshdover what do you think?

interface ElasticsearchPublicInterface {
  createClient: (type: string, clientConfig?..) => ICustomClusterClient;
  adminClient: IClusterClient;
  dataClient: IClusterClient;
}
interface ElasticsearchInternalInterface {
  legacy: {config$: ..};
  adminClient$: Observable<IClusterClient>;
  dataClient$: Observable<IClusterClient>;
}

• separate IClusterClient & ICustomClusterClient interfaces. If a plugin controls cluster client creation, it should clean up afterward. However, that's not true for the adminClient & dataClient, which are controlled by the core and plugins must not manipulate their state. Closes Plugins can close Elasticsearch clients and break other plugins #48963

interface IClusterClient {
  callAsInternalUser: (...) => any;
  asScoped: (req) => IScopedClusterClient;
}

interface ICustomClusterClient extends IClusterClient{
  close: () => void;
}

I like this suggestion 👍

• optional. we have to duplicate the logic for adminClient& dataClient. Does it make sense to switch to the sindle client already now? Closes Core Elasticsearch: remove "data" and "admin" clients and expose single client #49870

// SetupContract before
adminClient: {
  callAsInternalUser(){...} ,
  asScoped(request) {
    return {
      callAsInternalUser(){...}
      callAsCurrentUser(){...}
    },
}
dataClient: {
  callAsInternalUser(){...} , 
  asScoped(request) {
    return {
      callAsInternalUser(){...}
      callAsCurrentUser(){...}
    },
}
// SetupContract after
  callAsInternalUser(){...}
  asScoped(request) {
    return {
      callAsInternalUser(){...}
      callAsCurrentUser(){...}
    }
  }

As something low-priority, it's probably not worth doing if it's a lot of work, but it will reduce the code complexity, so if it's low effort it seems like a good bonus.

[mshustov]

I agree, I don't see any reason we should give a different client to the request context than what we give to plugins.
I think the reason this was specified was so that code in a context handler doesn't need to deal with observables or promises, a context handler should get a ready-to-use client. @joshdover what do you think?

I merged the current implementation. We can remove the difference later.

[joshdover]

If we assume that a plugin calling elasticsearch API couldn't be affected by ES client update, why it's so important for a request handler context?

Yeah I don't think the observable is important to the request handler context provider any more. However, I do think we should still have the request context provider to do the asScoped work for plugins, simply to provide for a safer default client for request handlers to reach for.