-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Advanced Settings] Introducing telemetry #82860
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
Changes from 4 commits
7af7623
27710ce
6332d5d
55ebb86
b72959c
d75dbdf
532df09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| <!-- Do not edit this file. It is automatically generated by API Documenter. --> | ||
|
|
||
| [Home](./index.md) > [kibana-plugin-core-public](./kibana-plugin-core-public.md) > [UiSettingsParams](./kibana-plugin-core-public.uisettingsparams.md) > [metric](./kibana-plugin-core-public.uisettingsparams.metric.md) | ||
|
|
||
| ## UiSettingsParams.metric property | ||
|
|
||
| Metric to track once this property changes | ||
|
|
||
| <b>Signature:</b> | ||
|
|
||
| ```typescript | ||
| metric?: { | ||
| type: UiStatsMetricType; | ||
| name: string; | ||
| }; | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| <!-- Do not edit this file. It is automatically generated by API Documenter. --> | ||
|
|
||
| [Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [UiSettingsParams](./kibana-plugin-core-server.uisettingsparams.md) > [metric](./kibana-plugin-core-server.uisettingsparams.metric.md) | ||
|
|
||
| ## UiSettingsParams.metric property | ||
|
|
||
| Metric to track once this property changes | ||
|
|
||
| <b>Signature:</b> | ||
|
|
||
| ```typescript | ||
| metric?: { | ||
| type: UiStatsMetricType; | ||
| name: string; | ||
| }; | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| * under the License. | ||
| */ | ||
| import { Type } from '@kbn/config-schema'; | ||
| import { UiStatsMetricType } from '@kbn/analytics'; | ||
|
|
||
| /** | ||
| * UI element type to represent the settings. | ||
|
|
@@ -80,6 +81,13 @@ export interface UiSettingsParams<T = unknown> { | |
| * Used to validate value on write and read. | ||
| */ | ||
| schema: Type<T>; | ||
| /** | ||
| * Metric to track once this property changes | ||
| */ | ||
| metric?: { | ||
| type: UiStatsMetricType; | ||
| name: string; | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The description can even be misleading, as when reading However, I know that this is not currently possible, as cc @elastic/kibana-telemetry as that reminds me of the issue about moving usage collection to a core service
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for moving the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We introduced
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can collect data, but I'm not sure we can perform the required calls to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 As an aside, we could use the same approach to implement a browser-side
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
++ 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Which is what this PR does, isn't it? So we're good with the implementation?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My initial suggestion was similar to https://github.com/elastic/kibana/pull/82860/files#r519857620 What if we track what a plugin registers every "UiSetting" and inform a plugin-owner about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rudolf in theory, we could leverage
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, as an alternative to #83084 we could implement UiSettings telemetry inside 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 👍 |
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,6 +36,7 @@ import { | |||||||||||
| import { FormattedMessage } from '@kbn/i18n/react'; | ||||||||||||
| import { isEmpty } from 'lodash'; | ||||||||||||
| import { i18n } from '@kbn/i18n'; | ||||||||||||
| import { UiStatsMetricType } from '@kbn/analytics'; | ||||||||||||
| import { toMountPoint } from '../../../../../kibana_react/public'; | ||||||||||||
| import { DocLinksStart, ToastsStart } from '../../../../../../core/public'; | ||||||||||||
|
|
||||||||||||
|
|
@@ -56,6 +57,7 @@ interface FormProps { | |||||||||||
| enableSaving: boolean; | ||||||||||||
| dockLinks: DocLinksStart['links']; | ||||||||||||
| toasts: ToastsStart; | ||||||||||||
| trackUiMetric?: (metricType: UiStatsMetricType, eventName: string | string[]) => void; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| interface FormState { | ||||||||||||
|
|
@@ -149,7 +151,7 @@ export class Form extends PureComponent<FormProps> { | |||||||||||
| if (!setting) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
| const { defVal, type, requiresPageReload } = setting; | ||||||||||||
| const { defVal, type, requiresPageReload, metric } = setting; | ||||||||||||
| let valueToSave = value; | ||||||||||||
| let equalsToDefault = false; | ||||||||||||
| switch (type) { | ||||||||||||
|
|
@@ -163,6 +165,11 @@ export class Form extends PureComponent<FormProps> { | |||||||||||
| const isArray = Array.isArray(JSON.parse((defVal as string) || '{}')); | ||||||||||||
| valueToSave = valueToSave.trim(); | ||||||||||||
| valueToSave = valueToSave || (isArray ? '[]' : '{}'); | ||||||||||||
| case 'boolean': | ||||||||||||
| if (metric && this.props.trackUiMetric) { | ||||||||||||
| const metricName = valueToSave ? `${metric.name}_on` : `${metric.name}_off`; | ||||||||||||
| this.props.trackUiMetric(metric.type, metricName); | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+168
to
+172
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what you mean by "we don't know the final value"?
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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we report: Do we assume it's ON because it's ON by default but it went OFF once and then ON again?
FYI: this the implementation: Line 49 in 18f7f04
If the default value is ON, it will report as follows:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. |
||||||||||||
| default: | ||||||||||||
| equalsToDefault = valueToSave === defVal; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import { i18n } from '@kbn/i18n'; | |
| import { schema } from '@kbn/config-schema'; | ||
|
|
||
| import { UiSettingsParams } from 'kibana/server'; | ||
| import { METRIC_TYPE } from '@kbn/analytics'; | ||
| import { | ||
| DEFAULT_COLUMNS_SETTING, | ||
| SAMPLE_SIZE_SETTING, | ||
|
|
@@ -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.', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixing this typo for thx! |
||
| }), | ||
| category: ['discover'], | ||
| schema: schema.boolean(), | ||
| metric: { | ||
| type: METRIC_TYPE.CLICK, | ||
| name: 'discover:modifyColumnsOnSwitchTitle', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking forward to get some statistics here! |
||
| }, | ||
| }, | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@majagrubic could you mark this property as
@deprecatedand add a comment:a temporary measurement until https://github.com/elastic/kibana/issues/83084