[Telemetry] Server-side Migration to NP by afharo · Pull Request #60485 · elastic/kibana

afharo · 2020-03-18T09:24:02Z

Summary

Migrate the server-side code of the Telemetry plugin to the NP.

Fixes #38765
Fixes #60443

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-03-18T09:24:04Z

Pinging @elastic/pulse (Team:Pulse)

…ropped support for injectVars

afharo · 2020-03-19T09:38:58Z

@elasticmachine merge upstream

elasticmachine · 2020-03-19T09:39:01Z

merge conflict between base and head

…ver-migration-to-np

afharo · 2020-03-19T15:33:46Z

src/legacy/core_plugins/ui_metric/index.ts

@@ -25,9 +25,6 @@ export default function(kibana: any) {
    id: 'ui_metric',


Still keeping this one here to maintain in legacy the ui_metric feature. But it's using the NP logic under the hood. It can safely be removed when getting rid of the legacy folder

chrisronline · 2020-03-19T19:08:03Z

FWIW, the PR for stack monitoring going to NP is nearly ready to merge: #56675

I imagine whoever goes second will need to update as a result and make any necessary changes

afharo · 2020-03-19T19:09:30Z

@chrisronline thanks for the heads up! :)

TinaHeiligers · 2020-03-19T22:32:23Z

src/plugins/telemetry/README.md

 3. Viewing usage data in the Kibana instance of the telemetry cluster (Viewing).

-This plugin is responsible for sending usage data to the telemetry cluster. For collecting usage data, use 
+This plugin is responsible for sending usage data to the telemetry cluster. For collecting usage data, use the [`usageCollection` plugin](../usage_collection/README.md)


Thank you for finishing the sentence!

TinaHeiligers

@afharo I'm still working through this. It's a huge PR but looking good so far.

TinaHeiligers · 2020-03-20T19:52:12Z

src/plugins/telemetry/server/routes/telemetry_usage_stats.ts

+        if (isDev) {
+          // don't ignore errors when running in dev mode
+          throw err;
+        } else if (unencrypted && err.status === 403) {


I'm ok with splitting the ternary out here and making it more explicit.

TinaHeiligers

I ran the code locally and the migration to NP works well!
Nice refactoring of some areas too, great work!
I've left a few comments and nits but otherwise LGTM once merge conflicts are resolved and tests pass again.

TinaHeiligers · 2020-03-20T21:03:41Z

src/plugins/telemetry/server/telemetry_collection/get_kibana.ts

-    .config()
-    .get('pkg.version')
-    .replace(/-snapshot/i, '');
+  const version = serverVersion.replace(/-snapshot/i, ''); // Shouldn't we better maintain the -snapshot so we can differentiate between actual final releases and snapshots?


// Shouldn't we better maintain the -snapshot so we can differentiate between actual final releases and snapshots?

I don't mind either way.

TinaHeiligers · 2020-03-20T21:46:23Z

src/plugins/usage_collection/README.md

+    import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
+    import { CoreSetup } from 'kibana/server';
+
+    class Plugin {


Suggestion: It might be worth adding an empty start method here to show the difference clearly between this example and the example below
e.g.

class Plugin { public setup(core: CoreSetup, plugins: { usageCollection?: UsageCollectionSetup }) { registerMyPluginUsageCollector(plugins.usageCollection); } public start() {} }

TinaHeiligers · 2020-03-20T21:59:41Z

src/plugins/usage_collection/README.md

-to worry about exceeding the 1000-field soft limit in Elasticsearch.
+to worry about exceeding the 1000-field soft limit in Elasticsearch.
+
+The only caveat is that it makes it harder to consume in Kibana when analysing each entry in the array separately. In the telemetry team we are working to find a solution to this. We are building a new way of reporting telemetry called [Pulse](../../../rfcs/text/0008_pulse.md) that will help on making these UI-Metrics easier to consume.


TinaHeiligers · 2020-03-20T22:17:35Z

x-pack/legacy/plugins/monitoring/server/telemetry_collection/get_high_level_stats.test.ts

-      expect(
-        await getHighLevelStats(server, callWith, clusterUuids, start, end, product)
-      ).toStrictEqual(expectedClusters);
+      expect(await getHighLevelStats(callWith, clusterUuids, start, end, product, 1)).toStrictEqual(


nit: I'd prefer an explicit declaration of maxBucketSize=1 being passed in here. Numbers tend to go 'missing' when directly embedded in code.

TinaHeiligers · 2020-03-20T22:18:29Z

x-pack/legacy/plugins/monitoring/server/telemetry_collection/get_high_level_stats.test.ts


      expect(
-        await fetchHighLevelStats(server, callWith, clusterUuids, start, end, product)
+        await fetchHighLevelStats(callWith, clusterUuids, start, end, product, 1)


See comment above

TinaHeiligers · 2020-03-20T22:23:08Z

x-pack/legacy/plugins/monitoring/server/telemetry_collection/register_monitoring_collection.ts

  telemetryCollectionManager.setCollection({
    esCluster: 'monitoring',
    title: 'monitoring',
    priority: 2,


Question: Should we not consider explicitly declaring collection priority in the telemetryCollectionManager rather than embedding priorities in here? That way, we could make the priority order more explicit.

that sounds like a neat suggestion, but I'd rather tackle it in a new issue since it's not related to the migration, itself

TinaHeiligers · 2020-03-20T22:25:52Z

x-pack/plugins/telemetry_collection_xpack/README.md

+
+## Development
+
+See the [kibana contributing guide](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md) for instructions setting up your development environment.


nit: 'See the kibana contributing guide for instructions on how to set up your development environment.'

ha! good call! this is actually an auto-generated message by the node scripts/generate_plugin.js script :)

TinaHeiligers · 2020-03-20T22:26:11Z

x-pack/plugins/telemetry_collection_xpack/README.md

@@ -0,0 +1,9 @@
+# telemetry_collection_xpack
+
+> Add the telemetry collector with the xpack information


Changed the description into something that hopefully explains it better.

TinaHeiligers · 2020-03-20T22:27:44Z

x-pack/plugins/telemetry_collection_xpack/server/plugin.ts

+    telemetryCollectionManager.setCollection({
+      esCluster: 'data',
+      title: 'local_xpack',
+      priority: 1,


See comment about embedding priority collection numbers in code above.

afharo · 2020-03-22T22:21:02Z

@elasticmachine merge upstream

…r approach

…ver-migration-to-np

afharo · 2020-03-23T10:14:18Z

@chrisronline I've merged those changes and applied mine on top. Hopefully, they make sense now.

pgayvallet

Only looked at platform owned files. LGTM.

pgayvallet · 2020-03-23T10:29:01Z

src/legacy/server/i18n/index.ts

+export async function i18nMixin(kbnServer: KbnServer, server: Server, config: KbnServer['config']) {
+  const locale = config.get('i18n.locale') as string;


NIT: KbnServer['config'] -> KbnConfig

pgayvallet · 2020-03-23T10:33:02Z

src/legacy/server/i18n/localization/telemetry_localization_collector.test.ts

  it('returns 0 if no translations registered', async () => {
    const i18nLoaderMock = createI18nLoaderMock({});
-    const count = await getTranslationCount(i18nLoaderMock, 'en');
+    const count = await getTranslationCount(i18nLoaderMock as any, 'en');


NIT: If createI18nLoaderMock cannot be adapted to return the proper signatures, I would at least have it handle the cast to typeof i18nLoader to avoid doing it in every test.

pgayvallet · 2020-03-23T10:34:15Z

src/legacy/server/i18n/localization/telemetry_localization_collector.ts

 import { i18nLoader } from '@kbn/i18n';
 import { size } from 'lodash';
+import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
 import { getIntegrityHashes, Integrities } from './file_integrity';
-import { KIBANA_LOCALIZATION_STATS_TYPE } from '../../../common/constants';
-import { UsageCollectionSetup } from '../../../../../../plugins/usage_collection/server';
+import { KIBANA_LOCALIZATION_STATS_TYPE } from '../constants';
+
 export interface UsageStats {
  locale: string;
  integrities: Integrities;
  labelsCount?: number;


Why is this file moving out of the telemetry plugin?

because it depends on the i18n configuration. In NP, we won't have access to other plugins' config.
Also: the way usage collection works is for every plugin to report its own specific telemetry. So it kind of makes more sense to me for the i18n to report the telemetry from here rather than exposing the config so the telemetry plugin can report it.
But happy to change it if necessary :)

igoristic

Looks good from the Stack Monitoring side 👍

Though I am curious why you're passing down maxBucketSize instead of using config.get('monitoring.ui.max_bucket_size') directly, and should we do that for other config.get('...') values if there are benefits

kibanamachine · 2020-03-23T12:35:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 188b99e

History

💔 Build #35263 failed 8b72f7b
💔 Build #35135 failed 9b654e7
💔 Build #35017 failed f1b6866
💚 Build #34616 succeeded c6e28c0
💔 Build #34601 failed 67ecb3f

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

afharo · 2020-03-23T12:35:58Z

@igoristic thank you for your review.

In NP the support to config.get('...') is dropped in favour of config.ui.max_bucket_size, that can be retrieved like:

export class MyPlugin implements Plugin {
  private readonly config$: Observable<MyPluginConfig>;
  constructor(initializerContext: PluginInitializerContext) {
    this.config$ = initializerContext.config.create();
  }

  public async setup(core: CoreSetup, deps: MyPluginDepsSetup) {
    const config = await this.config$.pipe(take(1)).toPromise();
    const maxBucketSize = config.ui.max_bucket_size;
  }
}

We can provide in the context the full config object if we want to. But I wanted to maintain some separation of concerns since only 1 config property was needed at this point :)

Bamieh

LGTM, i did pull the code locally but I havent done thorough testing over the changes.

* [Telemetry] Migration to NP * Telemetry management advanced settings section + fix import paths + dropped support for injectVars * Fix i18nrc paths for telemetry * Move ui_metric mappings to NP registerType * Fixed minor test tweaks * Add README docs (elastic#60443) * Add missing translation * Update the telemetryService config only when authenticated * start method is not a promise anymore * Fix mocha tests * No need to JSON.stringify the API responses * Catch handleOldSettings as we used to do * Deal with the forbidden use case in the optIn API * No need to provide the plugin name in the logger.get(). It is automatically scoped + one missing CallCluster vs. APICaller type replacement * Add empty start method in README.md to show differences with the other approach * Telemetry collection with X-Pack README * Docs update * Allow monitoring collector to send its own ES client * All collections should provide their own ES client * PR feedback * i18n NITs from kibana-platform feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* [Telemetry] Migration to NP * Telemetry management advanced settings section + fix import paths + dropped support for injectVars * Fix i18nrc paths for telemetry * Move ui_metric mappings to NP registerType * Fixed minor test tweaks * Add README docs (#60443) * Add missing translation * Update the telemetryService config only when authenticated * start method is not a promise anymore * Fix mocha tests * No need to JSON.stringify the API responses * Catch handleOldSettings as we used to do * Deal with the forbidden use case in the optIn API * No need to provide the plugin name in the logger.get(). It is automatically scoped + one missing CallCluster vs. APICaller type replacement * Add empty start method in README.md to show differences with the other approach * Telemetry collection with X-Pack README * Docs update * Allow monitoring collector to send its own ES client * All collections should provide their own ES client * PR feedback * i18n NITs from kibana-platform feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

elasticmachine · 2021-10-01T21:40:34Z

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

[Telemetry] Migration to NP

67f7f92

afharo added release_note:roadmap v8.0.0 v7.7.0 labels Mar 18, 2020

afharo added 2 commits March 18, 2020 17:29

Telemetry management advanced settings section + fix import paths + d…

0d3d506

…ropped support for injectVars

Fix i18nrc paths for telemetry

8d454b8

afharo added 9 commits March 19, 2020 09:41

Merge branch 'master' of github.com:elastic/kibana into telemetry/ser…

6cd9752

…ver-migration-to-np

Move ui_metric mappings to NP registerType

c4a8113

Fixed minor test tweaks

c613d82

Add README docs (elastic#60443)

9c93f3b

Add missing translation

c0526dc

Update the telemetryService config only when authenticated

6e728d6

start method is not a promise anymore

ef3aa88

Fix mocha tests

9574702

No need to JSON.stringify the API responses

0c3c6f8

afharo commented Mar 19, 2020

View reviewed changes

afharo marked this pull request as ready for review March 19, 2020 16:16

afharo requested review from a team as code owners March 19, 2020 16:16

afharo requested a review from a team March 19, 2020 16:16

afharo added 2 commits March 19, 2020 16:52

Catch handleOldSettings as we used to do

67ecb3f

Deal with the forbidden use case in the optIn API

c6e28c0

TinaHeiligers reviewed Mar 19, 2020

View reviewed changes

TinaHeiligers reviewed Mar 20, 2020

View reviewed changes

TinaHeiligers approved these changes Mar 20, 2020

View reviewed changes

elasticmachine and others added 5 commits March 22, 2020 18:21

Merge branch 'master' into telemetry/server-migration-to-np

8b72f7b

Add empty start method in README.md to show differences with the othe…

f258c23

…r approach

Telemetry collection with X-Pack README

0e71855

Merge branch 'master' of github.com:elastic/kibana into telemetry/ser…

95b4fad

…ver-migration-to-np

Docs update

155ab5e

afharo requested a review from a team as a code owner March 23, 2020 09:11

afharo added 3 commits March 23, 2020 09:40

Allow monitoring collector to send its own ES client

c91ab96

All collections should provide their own ES client

876cd0e

PR feedback

e4a5c63

pgayvallet approved these changes Mar 23, 2020

View reviewed changes

lizozom approved these changes Mar 23, 2020

View reviewed changes

i18n NITs from kibana-platform feedback

188b99e

igoristic approved these changes Mar 23, 2020

View reviewed changes

afharo mentioned this pull request Mar 23, 2020

[skip-ci] Fix CODEOWNERS paths for the Pulse team #60944

Merged

Bamieh approved these changes Mar 23, 2020

View reviewed changes

afharo merged commit 452193f into elastic:master Mar 23, 2020

afharo deleted the telemetry/server-migration-to-np branch March 23, 2020 18:49

afharo mentioned this pull request Mar 23, 2020

[7.x] [Telemetry] Server-side Migration to NP (#60485) #60965

Merged

lukeelmers added Feature:Telemetry Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Oct 1, 2021

lukeelmers removed the Team:KibanaTelemetry label Oct 1, 2021

		@@ -25,9 +25,6 @@ export default function(kibana: any) {
		id: 'ui_metric',


		## Development

		See the [kibana contributing guide](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md) for instructions setting up your development environment.

		@@ -0,0 +1,9 @@
		# telemetry_collection_xpack

		> Add the telemetry collector with the xpack information

		export async function i18nMixin(kbnServer: KbnServer, server: Server, config: KbnServer['config']) {
		const locale = config.get('i18n.locale') as string;

Conversation

afharo commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Mar 18, 2020

Uh oh!

afharo commented Mar 19, 2020

Uh oh!

elasticmachine commented Mar 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisronline commented Mar 19, 2020

Uh oh!

afharo commented Mar 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo commented Mar 22, 2020

Uh oh!

afharo commented Mar 23, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igoristic left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 23, 2020

💚 Build Succeeded

History

Uh oh!

afharo commented Mar 23, 2020

Uh oh!

Bamieh left a comment

afharo commented Mar 18, 2020 •

edited

Loading

TinaHeiligers Mar 20, 2020 •

edited

Loading