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

Monitoring: legacy ES client usages blocking migration of the licensing plugin #92185

Closed
pgayvallet opened this issue Feb 22, 2021 · 7 comments · Fixed by #92143
Closed

Monitoring: legacy ES client usages blocking migration of the licensing plugin #92185

pgayvallet opened this issue Feb 22, 2021 · 7 comments · Fixed by #92143
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Monitoring Stack Monitoring team

Comments

@pgayvallet
Copy link
Contributor

pgayvallet commented Feb 22, 2021

So, I tried to migrate the licensing plugin to use the new elasticsearch client (#92143)

I'm currently blocked by the monitoring plugin usage of createLicensePoller:

  • The plugin is still using the deprecated createLicensePoller API from the setup method instead of the one from start
  • The plugin is creating the legacy ES client using the deprecated core.elasticsearch.legacy.createClient API during its setup phase (there is no equivalent setup API to create new ES clients)

Overall, these two points are causing some huge pain to adapt the call to licensing.createLicensePoller, as creating the new ES client from monitoring's setup method can no longer be synchronous, meaning that we need to either:

  • Have the monitoring licensingService be created during start (and passed to the routes created during setup via reference
  • Have the monitoring licensingService receive a promise of an ES client and change its APIs from synchronous to asynchronous
  • Cleanup the MonitoringPlugin.getLegacyShim (no disrespect, but) atrocities and use request handler context to propagate the monitoring licensing service down to the routes.

const cluster = (this.cluster = instantiateClient(
config.ui.elasticsearch,
this.log,
core.elasticsearch.legacy.createClient
));

// Start our license service which will ensure
// the appropriate licenses are present
this.licenseService = new LicenseService().setup({
licensing: plugins.licensing,
monitoringClient: cluster,
config,
log: this.log,
});

One workaround to unblock the migration of the licensing plugin to the new ES client could be to add a new deprecated createLegacyLicensePoller API that would use a ILegacyClusterClient instead of IClusterClient.

cc @elastic/stack-monitoring-ui @restrry

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Monitoring Stack Monitoring team labels Feb 22, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@mshustov
Copy link
Contributor

mshustov commented Mar 4, 2021

Cleanup the MonitoringPlugin.getLegacyShim (no disrespect, but) atrocities and use request handler context to propagate the monitoring licensing service down to the routes.

It'd say it's time to get rid of the legacy wrapper. Monitoring plugin must migrate to the new ES client before v8.0 anyway #83910 , which will force the plugin to move ES client instantiation to start lifecycle.

@chrisronline @igoristic Is this work planned any time soon? Is it possible to refactor licensingService to unblock @pgayvallet work?

@chrisronline
Copy link
Contributor

@restrry I'm investigating solutions in #93620

@chrisronline
Copy link
Contributor

@restrry #93620 should handle unblocking the license service. We'll do the work to refactor the legacy createClient usage in a separate PR.

@simianhacker
Copy link
Member

The plugin is still using the deprecated createLicensePoller API from the setup method instead of the one from start

Is there a replacement API we should be using that will accept a IClusterClient? There seems to be a discrepancy between the type definition and the implementation.

  createLicensePoller: (
    clusterClient: IClusterClient,
    pollingFrequency: number
  ) => { license$: Observable<ILicense>; refresh(): Promise<ILicense> };

vs

  private createLicensePoller(clusterClient: ILegacyClusterClient, pollingFrequency: number) {

OR should I just update createLicensePoller and fetchLicense to use the new IClusterClient interface?

@mshustov
Copy link
Contributor

@simianhacker should be done in #92143 @pgayvallet any ETA for this?

@pgayvallet
Copy link
Contributor Author

any ETA for this?

There wasn't, but if we're unblocked, I will try to do it asap.

Need to take a look to check if #93620 solves all we need to unblock the licensing migration. Planning to take a look this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Monitoring Stack Monitoring team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants