[Usage collection] Collect non-default kibana configs by Bamieh · Pull Request #97368 · elastic/kibana

Bamieh · 2021-04-16T11:31:35Z

Config Usage Collector

The config usage collector reports non-default kibana configs.

All non-default configs except booleans and numbers will be reported as [redacted] unless otherwise specified via config.exposeToUsage in the plugin config descriptor.

import { schema, TypeOf } from '@kbn/config-schema';
import { PluginConfigDescriptor } from 'src/core/server';

export const configSchema = schema.object({
  usageCounters: schema.object({
    enabled: schema.boolean({ defaultValue: true }),
    retryCount: schema.number({ defaultValue: 1 }),
    bufferDuration: schema.duration({ defaultValue: '5s' }),
  }),
  uiCounters: schema.object({
    enabled: schema.boolean({ defaultValue: true }),
    debug: schema.boolean({ defaultValue: schema.contextRef('dev') }),
  }),
  maximumWaitTimeForAllCollectorsInS: schema.number({
    defaultValue: DEFAULT_MAXIMUM_WAIT_TIME_FOR_ALL_COLLECTORS_IN_S,
  }),
});

export const config: PluginConfigDescriptor<ConfigType> = {
  schema: configSchema,
  exposeToUsage: {
    uiCounters: true,
    usageCounters: {
      bufferDuration: true,
    },
    maximumWaitTimeForAllCollectorsInS: false,
  },
};

In the above example setting uiCounters: true in the exposeToUsage property marks all configs under the path uiCounters as safe. The collector will send the actual non-default config value when setting an exact config or its parent path to true.

Settings the config path or its parent path to false will explicitly mark this config as unsafe. The collector will send [redacted] for non-default configs when setting an exact config or its parent path to false.

Example output of the collector

{
  "kibana_config_usage": {
    "xpack.apm.serviceMapTraceIdBucketSize": 30,
    "elasticsearch.username": "[redacted]",
    "elasticsearch.password": "[redacted]",
    "plugins.paths": "[redacted]",
    "server.port": 5603,
    "server.basePath": "[redacted]",
    "server.rewriteBasePath": true,
    "logging.json": false,
    "usageCollection.uiCounters.debug": true
  }
}

elasticmachine · 2021-04-16T16:22:42Z

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

elasticmachine · 2021-04-16T16:22:43Z

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

Bamieh · 2021-04-16T18:13:48Z

@elasticmachine merge upstream

Bamieh · 2021-04-16T19:46:53Z

@elasticmachine merge upstream

Bamieh · 2021-04-18T09:06:20Z

@elasticmachine merge upstream

afharo

Overall, it looks great! Good job @Bamieh! I just added some NITs and 2 potential issues we might want to look at before merging this PR:

startsWith check
array of objects

pgayvallet

Overall looking great.

A few remarks and questions

pgayvallet · 2021-04-19T14:13:27Z

-    "exclude": []
+    "exclude": [
+      "src/plugins/kibana_usage_collection/server/collectors/config_usage/register_config_usage_collector.ts"
+    ]


Why was that necessary?

I've opened a follow up issue here: #97599.

TLDR;

Explicitly typing every config in the schema is not practical. We'll be experimenting with flattened_type ES type and runtime fields to query this collector. Both are not yet supported by the collection schema types.

If we had to explicitly specify every kibana config in the schema we'd need to improve our telemetry tooling to automatically do this for devs. Although flattened_type seems promising enough for this use case.

pgayvallet · 2021-04-19T14:35:11Z

+  public getExposedPluginConfigsToUsage() {
+    return this.pluginConfigUsageDescriptors;
+  }


Why can't this be added to the internal setup contract instead of creating a new method?

this function is called in the start function before plugins.start is called. I can expose it from setup but then i'd have to pass it from setup to start via this inside server.

pgayvallet · 2021-04-19T16:00:22Z

+    if (typeof isSafe === 'boolean') {
+      return { explicitlyMarked: true, isSafe };
+    }


I'm wondering (if I understood the model correctly): with TS support, can this condition actually be false?

Correct TS should not allow this case to happen since we only allow boolean leafs. We cannot rely on TS type safety alone since devs can always // @ts-ignore. This would also guard from unexpected changes to the type by mistake in the future (along with tests).

pgayvallet · 2021-04-19T16:07:59Z

+        // @ts-expect-error
+        service.getMarkedAsSafe = mockGetMarkedAsSafe;


Optional: I think CoreUsageDataService.getMarkedAsSafe can be made static, right? If so, we could extract it to allow proper module mocking.

…ion/changed_config

pgayvallet

LGTM

afharo

LGTM! Thank you for this, @Bamieh!

afharo · 2021-04-20T11:18:34Z

+  merge(
+    get(fullSchema, 'properties.stack_stats.properties.kibana.properties.plugins'),
    telemetrySchema.plugins
  );


kibanamachine · 2021-04-20T15:02:22Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`core`	2174	2176	+2

History

💔 Build #120910 failed 46e3c43
💔 Build #120898 failed a324d29
💔 Build #120895 failed 3c32db0
💚 Build #120370 succeeded 0f1a3ae
💔 Build #120328 failed 365154c

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-04-20T15:05:11Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Ahmad Bamieh <ahmadbamieh@gmail.com>

Bamieh added 4 commits April 16, 2021 12:32

config usage service + collector

18356ef

merge master + exclude schema

e98a5fe

add more tests

50d8ef0

add api_integration tests

de4c161

Bamieh requested a review from afharo April 16, 2021 11:33

Bamieh added 4 commits April 16, 2021 16:26

type check + docs

115736f

update snapshots

57d39eb

fix spacing

545412f

fix x-pack tests

bca8b7e

Bamieh marked this pull request as ready for review April 16, 2021 16:22

Bamieh requested a review from a team as a code owner April 16, 2021 16:22

Merge branch 'master' into usage_collection/changed_config

466f4eb

Merge branch 'master' into usage_collection/changed_config

365154c

Merge branch 'master' into usage_collection/changed_config

0f1a3ae

afharo reviewed Apr 19, 2021

View reviewed changes

pgayvallet reviewed Apr 19, 2021

View reviewed changes

Bamieh added 3 commits April 20, 2021 10:06

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

6790635

…ion/changed_config

core review fixes

9fd517f

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

3c32db0

…ion/changed_config

Bamieh requested a review from a team as a code owner April 20, 2021 10:23

Bamieh requested review from afharo and pgayvallet April 20, 2021 10:23

pgayvallet approved these changes Apr 20, 2021

View reviewed changes

update docs

a324d29

afharo approved these changes Apr 20, 2021

View reviewed changes

Bamieh added 2 commits April 20, 2021 14:49

type check

46e3c43

fix integration tests unit tests 😅

c3a3e11

Bamieh enabled auto-merge (squash) April 20, 2021 14:09

Bamieh merged commit 0f45381 into elastic:master Apr 20, 2021

Bamieh deleted the usage_collection/changed_config branch April 20, 2021 15:03

kibanamachine mentioned this pull request Apr 20, 2021

[7.x] [Usage collection] Collect non-default kibana configs (#97368) #97653

Merged

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 20, 2021

[Usage collection] Collect non-default kibana configs (elastic#97368)

b40df13

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

lukeelmers added Feature:Telemetry and removed Team:KibanaTelemetry labels Oct 1, 2021

		// @ts-expect-error
		service.getMarkedAsSafe = mockGetMarkedAsSafe;

Conversation

Bamieh commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Config Usage Collector

Uh oh!

elasticmachine commented Apr 16, 2021

Uh oh!

elasticmachine commented Apr 16, 2021

Uh oh!

Bamieh commented Apr 16, 2021

Uh oh!

Bamieh commented Apr 16, 2021

Uh oh!

Bamieh commented Apr 18, 2021

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

Bamieh Apr 20, 2021 • 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!

Uh oh!

pgayvallet Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

Bamieh Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pgayvallet Apr 19, 2021 • 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!

Bamieh Apr 20, 2021 • 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!

Uh oh!

pgayvallet Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

afharo Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 20, 2021

💚 Build Succeeded

Metrics [docs]

API count

History

Uh oh!

kibanamachine commented Apr 20, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Bamieh commented Apr 16, 2021 •

edited

Loading

Bamieh Apr 20, 2021 •

edited

Loading

pgayvallet Apr 19, 2021 •

edited

Loading

Bamieh Apr 20, 2021 •

edited

Loading