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

Adds new elasticsearch client to telemetry plugin #78046

Merged

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Sep 21, 2020

Summary

Resolves #75384
Support for the legacy Elasticsearch client will be removed from 7.14.

Full migration to the new client involves several steps.

There are two options we have to provide the new ES client before support for the legacy client is removed:

Either completely replace callCluster with the new client's implementation or, as suggested in #74840,
add the new client to the collectors:

collector.fetch(callCluster, esClient) => {...}

The advantage of option 2 is that plugins can migrate over time.

It also gives some room to explore the effect of the new behavior (e.g. request timeout changes[0]) and breaking changes[1]

This PR implements adding the new the client and using it internally in local and local-xpack usage collection. The new client is exposed to usage collectors in their fetch methods through an additional argument in bulkFetch:

  public bulkFetch = async (
    callCluster: LegacyAPICaller,
    esClient: ElasticsearchClient,
    collectors: Map<string, Collector<any, any>> = this.collectors
  ) => {...}

There will be follow up PRs for:

  • Using the new client to in Monitoring collection
  • Refactoring each plugin's collector to use the new client
  • Exposing scoped saved objects client in collector's fetch method

Notes
The new es Client is only available from coreStart. The change to the way we register collections needs to provide a client getter rather than the object straight away (similar to the way the saved objects client is handled in the kibana_usage_collection plugin).

[0] elasticsearch.requestTimeout config value:
In the legacy client, the timeout determined the timeout before closing the connection on each request. In the new client, the request is automatically retried 3 times, meaning the request won't actually timeout until 4 * requestTimeout has elapsed.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Copy link
Contributor

@chrisronline chrisronline left a 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!

// The new client should be inititalized with a similar config to `this.cluster` but, since we're not using
// the new client in Monitoring Telemetry collection yet, setting the local client allos progress for now.
// We will update the client in a follow up PR.
this.telemetryElasticsearchClient = elasticsearch.client;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisronline I've made the change you wanted 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers removed the request for review from a team September 24, 2020 22:03
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers dismissed afharo’s stale review September 25, 2020 01:32

changes have been addressed

@TinaHeiligers TinaHeiligers merged commit d512b5a into elastic:master Sep 25, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
* master: (226 commits)
  [Enterprise Search] Added Logic for the Credentials View (elastic#77626)
  [CSM] Js errors (elastic#77919)
  Add the @kbn/apm-config-loader package (elastic#77855)
  [Security Solution] Refactor useSelector (elastic#75297)
  Implement tagcloud renderer (elastic#77910)
  [APM] Alerting: Add global option to create all alert types (elastic#78151)
  [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939)
  TypeScript cleanup in visualizations plugin (elastic#78428)
  Lazy load metric & mardown visualizations (elastic#78391)
  [Detections][EQL] EQL rule execution in detection engine (elastic#77419)
  Update tutorial-full-experience.asciidoc (elastic#75836)
  Update tutorial-define-index.asciidoc (elastic#75754)
  Add support for runtime field types to mappings editor. (elastic#77420)
  [Monitoring] Usage collection (elastic#75878)
  [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316)
  [Security Solution][Resolver] Update @timestamp formatting (elastic#78166)
  [Security Solution] Fix app layout (elastic#76668)
  [Security Solution][Resolver] 2 new functions to DAL (elastic#78477)
  Adds new elasticsearch client to telemetry plugin (elastic#78046)
  skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 78046 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 29, 2020
@TinaHeiligers TinaHeiligers added the backport:skip This commit does not require backporting label Sep 29, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 29, 2020
@TinaHeiligers TinaHeiligers removed the backport:skip This commit does not require backporting label Oct 5, 2020
TinaHeiligers added a commit that referenced this pull request Oct 5, 2020
)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@TinaHeiligers TinaHeiligers deleted the usage-collection-with-new-esClient branch October 14, 2020 15:35
@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 1, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Usage Collection] Add the new elasticsearch client to the usage collection and telemetry plugins
6 participants