[Advanced Settings] Introducing telemetry #82860

majagrubic · 2020-11-06T16:28:18Z

Summary

As a part of #58747 , we'd like to add a metric around how many times users opt out of the new data grid. Since this is done through advanced settings, we'd need to track settings change there. My proposal is to introduce a new metric parameter to UiSettingsParam interface. Then, in advanced settings, for each setting that is interested in a metric, we emit that metric.

This PR:

introduces metric param to UiSettingsParams interface
introduces trackUiMetric to Advanced Settings plugin
shows how this would work on a Discover modifyColumns setting

Since EuiDataGrid for master is not yet in master, there's no need to pollute the master with that particular setting. If the approach is good, it will be easy to add this once it is ready.

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~- [ ] This was checked for cross-browser compatibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-11-09T08:17:21Z

Pinging @elastic/kibana-app (Team:KibanaApp)

majagrubic · 2020-11-09T08:26:39Z

@elasticmachine merge upstream

pgayvallet · 2020-11-09T10:40:59Z

src/core/types/ui_settings.ts

+  /**
+   * Metric to track once this property changes
+   */
+  metric?: {
+    type: UiStatsMetricType;
+    name: string;
+  };


What bothers me with this approach is that the responsibilities around this new feature are, imho, wrongly dispatched:

We are defining the metric metadata for a given uiSetting on UiSettingsParams, and yet, core that is the 'owner' of this type / functionality will not uses at all. Instead, that's the responsibility of the advancedSettings plugin to use this new metric configuration. If this configuration is registered from core APIs, I would expect the uiSettingsClient to have the responsibility to perform the call usageCollection.reportUiStats when updating a setting.

The description can even be misleading, as when reading Metric to track once this property changes I would assume that this would work when using either the client or server-side ui settings client, not only from the advanced settings management page.

However, I know that this is not currently possible, as core has no way to access usage collection, which is currently a plugin, so I don't really have a better solution that wouldn't include creating an additional hook in core to allow registering usage collector in it, as we decided to exclude this solution in some previous discussions.

cc @elastic/kibana-telemetry as that reminds me of the issue about moving usage collection to a core service
cc @restrry what's your opinion on this?

+1 for moving the usageCollector to core. I also think now is a good time to do so.

We introduced CoreUsageDataService in #79101 so core can collect usage data now.

It can collect data, but I'm not sure we can perform the required calls to reportUiStats atm ?

Yes, it's only server-side. Though in this case I think doing this server-side is preferable so that we also pick up on uiSettings.overrides set in kibana.yml.

As an aside, we could use the same approach to implement a browser-side CoreUsageDataService. I'm neutral on whether telemetry should be inside Core, but I just don't think we have to block any use cases on that decision.

maybe it's simpler if the plugin that cares just reports this in it's own usage data?

++ here. There are so many ways to change uiSettings: UI, REST API call, kibana.yml. Plugin reading from UiSettings service and reporting set values seems to be the most reliable way to implement this.

maybe it's simpler if the plugin that cares just reports this in it's own usage data?

Which is what this PR does, isn't it? So we're good with the implementation?

Which is what this PR does, isn't it? So we're good with the implementation?

My initial suggestion was similar to https://github.com/elastic/kibana/pull/82860/files#r519857620
A plugin reads its UiSettings value and reports them. But it seems that in this case, we want to count how many times the value's been toggled.
Read UiSettings and compare it with the last stored value might not be an option since multiple Kibana users would report the "toggle" event.
The current solution works well until we want to track something more complex than boolean value. For example, for an array, one might want to track: the number of inserts, number of array items, number of permutations, etc.

What if we track what a plugin registers every "UiSetting" and inform a plugin-owner about write operation to implement a custom logic. As a bonus, we could restrict access to other plugin settings

@rudolf in theory, we could leverage CoreUsageDataService to report this count stats exposed by UiSettings service, but the core should be able to filter out keys then, which it doesn't know. We can merge the current solution considering it to-be-refactored-soon. WDYT?

yeah, as an alternative to #83084 we could implement UiSettings telemetry inside CoreUsageDataService. My concern is that we implement something which only solves one use case but doesn't work for other plugins. But it could very well be that if we just track UiSetting toggle events this is good enough for 90% of our telemetry needs. In such a case we just track a toggle event whether it's a boolean, array, string etc. If we go down this route we should just get some feedback from other plugins to validate that this solves their needs.

Though in general it feels like we might end up with a better design if we leave this outside of core and if it becomes adopted by many solutions and stabilises we pull it into core.

So I would say let's merge this and see refactor it later 👍

afharo · 2020-11-09T13:35:03Z

src/plugins/advanced_settings/public/management_app/components/form/form.tsx

+        case 'boolean':
+          if (metric && this.props.trackUiMetric) {
+            const metricName = valueToSave ? `${metric.name}_on` : `${metric.name}_off`;
+            this.props.trackUiMetric(metric.type, metricName);
+          }


We are counting the number of ON/OFF events, but we don't know the final value. Is this the intention?

For the final selected value, stack_stats.kibana.plugins.stack_management already reports it when it's different to the default. Would that work?

Not sure what you mean by "we don't know the final value"? valueToSave is the final value that is going to be saved.

For the final selected value, stack_stats.kibana.plugins.stack_management already reports it when it's different to the default. Would that work?

This is great insight, thanks. I'm not sure how it works, will have a look at it. Would it report if a value changes from non-default back to a default one?

Not sure what you mean by "we don't know the final value"? valueToSave is the final value that is going to be saved.

If we report:

{ "ui_metric": { "modifyColumns_on": 1, "modifyColumns_off": 1 } }

Do we assume it's ON because it's ON by default but it went OFF once and then ON again?

For the final selected value, stack_stats.kibana.plugins.stack_management already reports it when it's different to the default. Would that work?

This is great insight, thanks. I'm not sure how it works, will have a look at it. Would it report if a value changes from non-default back to a default one?

FYI: this the implementation:

kibana/src/plugins/kibana_usage_collection/server/collectors/management/telemetry_management_collector.ts

Line 49 in 18f7f04

export function registerManagementUsageCollector(

If the default value is ON, it will report as follows:

User's option Reported stack_stats.kibana.plugins.stack_management.modifyColumns

ON (same as default) the key won't be reported

OFF false

ON again the key won't be reported again because it matches the default

Do we assume it's ON because it's ON by default but it went OFF once and then ON again?

Yes, it means it's on. I think just a rough sense of the ratio of how many times users have switched on/off is a good metric. If it was switched off 50 times and switched back on 0 times - users must be having an issue with the default implementation. If it was switched off 50 times and switched back on 45 times - majority of users have tried the default behavior, compared it with the old behavior, and decided they like the new behavior better. I think the ratio here is a valuable metric and without reporting ON again scenario, we'd lose that insight.

Sounds good to me.
@alexfrancoeur

mshustov · 2020-11-10T18:06:55Z

src/core/types/ui_settings.ts

+  /**
+   * Metric to track once this property changes
+   */
+  metric?: {


@majagrubic could you mark this property as @deprecated and add a comment:
a temporary measurement until https://github.com/elastic/kibana/issues/83084

kibanamachine · 2020-11-11T17:18:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 532df09

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`advancedSettings`	918.8KB	919.3KB	+507.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`advancedSettings`	10.4KB	10.4KB	+48.0B

History

💔 Build #87326 failed d75dbdf
💚 Build #86650 succeeded 55ebb86
💚 Build #86616 succeeded 6332d5d
💚 Build #86614 succeeded 27710ce
💔 Build #86449 failed 7af7623

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

kertal

Code LGTM 👍 , tested locally in Chrome, Firefox, Safari. When changing discover:modifyColumnsOnSwitchTitle it is tracked. This is a big step / start for gaining more insights what users modify in our Advanced Settings, and will help us to improve Kibana to provide good defaults (e.g. when we notice that a feature is toggled often)

kertal · 2020-11-12T11:52:54Z

src/plugins/discover/server/ui_settings.ts

@@ -170,9 +171,13 @@ export const uiSettings: Record<string, UiSettingsParams> = {
    }),
    value: true,
    description: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchText', {
-      defaultMessage: 'Remove columns that not available in the new index pattern.',
+      defaultMessage: 'Remove columns that are not available in the new index pattern.',


Fixing this typo for thx!

kertal · 2020-11-12T11:54:11Z

src/plugins/discover/server/ui_settings.ts

    }),
    category: ['discover'],
    schema: schema.boolean(),
+    metric: {
+      type: METRIC_TYPE.CLICK,
+      name: 'discover:modifyColumnsOnSwitchTitle',


Looking forward to get some statistics here!

mattkime

app arch code owner changes are just a doc change, approved.

* master: [Advanced Settings] Introducing telemetry (elastic#82860) [alerts] add executionStatus to event log doc for action execute (elastic#82401) Add additional sources routes (elastic#83227) [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149) [Logs UI] Add pagination to the log stream shared component (elastic#81193) [Index Management] Add an index template link to data stream details (elastic#82592) Add maps_oss folder to code_owners (elastic#83204) fix truncation issue (elastic#83000) [Ingest Manger] Move asset getters out of registry (elastic#83214)

* [Advaned Settings] Introducing telemetry * Publishing doc changes * Move metric tracking to onSave method * Adding deprecated warning * Updating docs Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

… alerts/action-groups-as-conditions * origin/alerts/stack-alerts-public: (91 commits) removed import from plugin code as it causes FTR to fail [Advanced Settings] Introducing telemetry (elastic#82860) [alerts] add executionStatus to event log doc for action execute (elastic#82401) Add additional sources routes (elastic#83227) [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149) [Logs UI] Add pagination to the log stream shared component (elastic#81193) [Index Management] Add an index template link to data stream details (elastic#82592) Add maps_oss folder to code_owners (elastic#83204) fix truncation issue (elastic#83000) [Ingest Manger] Move asset getters out of registry (elastic#83214) make defaulted field non maybe Remove unused asciidoc file (elastic#83228) [Lens] Remove background from lens embeddable (elastic#83061) [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991) Removing unnecessary trailing slash in CODEOWNERS Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236) Trying to fix CODEOWERS, missing a starting slash (elastic#83233) skip flaky suite (elastic#83231) Add enzyme rerender test helper (elastic#83208) Move Elasticsearch type definitions out of APM (elastic#83081) ...

[Advaned Settings] Introducing telemetry

7af7623

majagrubic changed the title ~~[Advaned Settings] Introducing telemetry~~ [Advanced Settings] Introducing telemetry Nov 6, 2020

Maja Grubic added 2 commits November 8, 2020 13:01

Publishing doc changes

27710ce

Move metric tracking to onSave method

6332d5d

majagrubic added v7.11.0 release_note:roadmap Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 9, 2020

majagrubic marked this pull request as ready for review November 9, 2020 08:17

majagrubic requested a review from a team November 9, 2020 08:17

majagrubic requested review from a team as code owners November 9, 2020 08:17

Merge branch 'master' into app-management-telemetry

55ebb86

majagrubic requested a review from kertal November 9, 2020 09:27

pgayvallet reviewed Nov 9, 2020

View reviewed changes

afharo reviewed Nov 9, 2020

View reviewed changes

This was referenced Nov 10, 2020

[meta] uiSettings service improvements #48925

Open

Plugins should be aware of UiSettings values change #83084

Open

mshustov reviewed Nov 10, 2020

View reviewed changes

Maja Grubic added 2 commits November 11, 2020 12:46

Adding deprecated warning

b72959c

Merge branch 'master' into app-management-telemetry

d75dbdf

majagrubic requested a review from mshustov November 11, 2020 12:57

Updating docs

532df09

mshustov approved these changes Nov 12, 2020

View reviewed changes

kertal approved these changes Nov 12, 2020

View reviewed changes

mattkime approved these changes Nov 12, 2020

View reviewed changes

majagrubic merged commit 5551966 into elastic:master Nov 12, 2020

majagrubic deleted the app-management-telemetry branch November 12, 2020 14:39

majagrubic mentioned this pull request Nov 12, 2020

[7.x] [Advanced Settings] Introducing telemetry (#82860) #83290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Advanced Settings] Introducing telemetry #82860

[Advanced Settings] Introducing telemetry #82860

majagrubic commented Nov 6, 2020 •

edited

Loading

elasticmachine commented Nov 9, 2020

majagrubic commented Nov 9, 2020

pgayvallet Nov 9, 2020 •

edited

Loading

Bamieh Nov 9, 2020

rudolf Nov 9, 2020

pgayvallet Nov 9, 2020

rudolf Nov 9, 2020

mshustov Nov 9, 2020

pgayvallet Nov 9, 2020

mshustov Nov 9, 2020 •

edited

Loading

mshustov Nov 10, 2020

rudolf Nov 12, 2020

afharo Nov 9, 2020

majagrubic Nov 9, 2020 •

edited

Loading

afharo Nov 9, 2020

majagrubic Nov 9, 2020

afharo Nov 10, 2020

mshustov Nov 10, 2020

kibanamachine commented Nov 11, 2020

kertal left a comment

kertal Nov 12, 2020

kertal Nov 12, 2020

mattkime left a comment

User's option	Reported `stack_stats.kibana.plugins.stack_management.modifyColumns`
ON (same as default)	the key won't be reported
OFF	`false`
ON again	the key won't be reported again because it matches the default

[Advanced Settings] Introducing telemetry #82860

[Advanced Settings] Introducing telemetry #82860

Conversation

majagrubic commented Nov 6, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 9, 2020

majagrubic commented Nov 9, 2020

pgayvallet Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Nov 11, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

majagrubic commented Nov 6, 2020 •

edited

Loading

pgayvallet Nov 9, 2020 •

edited

Loading

mshustov Nov 9, 2020 •

edited

Loading

majagrubic Nov 9, 2020 •

edited

Loading