-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Advanced Settings] Introducing telemetry #82860
Changes from all 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,21 @@ | ||
<!-- 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 | ||
|
||
> Warning: This API is now obsolete. | ||
> | ||
> Temporary measure until https://github.com/elastic/kibana/issues/83084 is in place | ||
> | ||
|
||
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,21 @@ | ||
<!-- 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 | ||
|
||
> Warning: This API is now obsolete. | ||
> | ||
> Temporary measure until https://github.com/elastic/kibana/issues/83084 is in place | ||
> | ||
|
||
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
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, 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? 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:
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 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.', | ||
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', | ||
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
@deprecated
and add a comment:a temporary measurement until https://github.com/elastic/kibana/issues/83084