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

[Usage Collection] Improves Collector fetch API #79595

Merged
merged 52 commits into from
Oct 13, 2020

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Oct 5, 2020

Why has my team been pinged?

This is a breaking change in the Kibana telemetry usage collectors' fetch method and required refactoring collectors in all the plugins that currently use callCluster.

Notes to reviewers:

All you need to do is check that the changes to your plugin usage collector (and unit tests) are ok.

PR details

Handles step 3 in #74840 (comment).

Summary:
Replaces callCluster and esClient as the arguments to collectors' fetch method with { callCluster, esClient }.

Description:
As part of migrating over to the new Elasticsearch client, we provided the new client as an additional parameter to the fetch method in collectors and pass the client down as an additional collection config parameter.

To use the new client forces plugin owners to provide the old client as well, which is not ideal.

This work refactors the way in which we provide the clients from individual arguments to an object, allowing plugins to select the client they wish to use.

Before:

const myCollector = usageCollection.makeUsageCollector<Usage>({
  type: 'MY_USAGE_TYPE',
  schema: {
    my_objects: {
      total: 'long',
    },
  },
  fetch: async (callCluster: APICluster, esClient: IClusterClient) => {
    return {
      my_objects: {
        total: SOME_NUMBER
      }
    };
  },
});

After:

interface CollectorFetchClients = {
  callCluster: LegacyAPICaller,
  esClient?: ElasticsearchClient
}

const myCollector = usageCollection.makeUsageCollector<Usage>({
  type: 'MY_USAGE_TYPE',
  schema: {
    my_objects: {
      total: 'long',
    },
  },
  fetch: async (collectorFetchClients: CollectorFetchClients) => {
    return {
      my_objects: {
        total: SOME_NUMBER
      }
    };
  },
});

This is a breaking change and required updates to all collectors using callCluster.

Checklist

For maintainers

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Canvas changes lgtm

@TinaHeiligers TinaHeiligers requested a review from Bamieh October 8, 2020 22:33
Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Checked Uptime changes, telemetry logging appears to be working as I'd expect. LGTM

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

AppArch changes LGTM

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Security solution changes look good to me, thanks!

@@ -10,6 +10,7 @@ import { PluginsSetup } from '../plugin';
import { KibanaFeature } from '../../../features/server';
import { ILicense, LicensingPluginSetup } from '../../../licensing/server';
import { pluginInitializerContextConfigMock } from 'src/core/server/mocks';
import { createCollectorFetchContextMock } from 'src/plugins/usage_collection/server/usage_collection.mock';
Copy link
Member

Choose a reason for hiding this comment

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

question: same question about importing from 'src/plugins/usage_collection/server/mocks' here as well.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Oct 13, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

KibanaApp code review only, LGTM!

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Creates a timeline.Timelines Creates a timeline

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has failed 9 times on tracked branches: https://github.com/elastic/kibana/issues/79389

CypressError: `cy.wait()` could not find a registered alias for: `@timeline`.
You have not aliased anything yet.
    at aliasNotFoundFor (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:150289:22)
    at $Cy.getAlias (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:150252:7)
    at waitForXhr (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:159965:25)
    at http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:160054:14
    at tryCatcher (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:10325:23)
    at MappingPromiseArray._promiseFulfilled (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:7445:38)
    at MappingPromiseArray.PromiseArray._iterate (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:8647:31)
    at MappingPromiseArray.init (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:8611:10)
    at MappingPromiseArray._asyncInit (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:7414:10)
    at _drainQueueStep (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:5036:12)
    at _drainQueue (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:5025:9)
    at Async.../../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:5041:5)
    at Async.drainQueues (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:4911:14)
From Your Spec Code:
    at Context.eval (http://localhost:6111/__cypress/tests?p=cypress/integration/timeline_creation.spec.ts:13497:35)

Metrics [docs]

✅ unchanged

History

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

@TinaHeiligers TinaHeiligers merged commit 70a9164 into elastic:master Oct 13, 2020
TinaHeiligers added a commit that referenced this pull request Oct 13, 2020
@TinaHeiligers TinaHeiligers deleted the combine-esClients-args 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 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.