Skip to content
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

Merged
merged 7 commits into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface UiSettingsParams<T = unknown>
| [category](./kibana-plugin-core-public.uisettingsparams.category.md) | <code>string[]</code> | used to group the configured setting in the UI |
| [deprecation](./kibana-plugin-core-public.uisettingsparams.deprecation.md) | <code>DeprecationSettings</code> | optional deprecation information. Used to generate a deprecation warning. |
| [description](./kibana-plugin-core-public.uisettingsparams.description.md) | <code>string</code> | description provided to a user in UI |
| [metric](./kibana-plugin-core-public.uisettingsparams.metric.md) | <code>{</code><br/><code> type: UiStatsMetricType;</code><br/><code> name: string;</code><br/><code> }</code> | Metric to track once this property changes |
| [name](./kibana-plugin-core-public.uisettingsparams.name.md) | <code>string</code> | title in the UI |
| [optionLabels](./kibana-plugin-core-public.uisettingsparams.optionlabels.md) | <code>Record&lt;string, string&gt;</code> | text labels for 'select' type UI element |
| [options](./kibana-plugin-core-public.uisettingsparams.options.md) | <code>string[]</code> | array of permitted values for this setting |
Expand Down
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) &gt; [kibana-plugin-core-public](./kibana-plugin-core-public.md) &gt; [UiSettingsParams](./kibana-plugin-core-public.uisettingsparams.md) &gt; [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
Expand Up @@ -19,6 +19,7 @@ export interface UiSettingsParams<T = unknown>
| [category](./kibana-plugin-core-server.uisettingsparams.category.md) | <code>string[]</code> | used to group the configured setting in the UI |
| [deprecation](./kibana-plugin-core-server.uisettingsparams.deprecation.md) | <code>DeprecationSettings</code> | optional deprecation information. Used to generate a deprecation warning. |
| [description](./kibana-plugin-core-server.uisettingsparams.description.md) | <code>string</code> | description provided to a user in UI |
| [metric](./kibana-plugin-core-server.uisettingsparams.metric.md) | <code>{</code><br/><code> type: UiStatsMetricType;</code><br/><code> name: string;</code><br/><code> }</code> | Metric to track once this property changes |
| [name](./kibana-plugin-core-server.uisettingsparams.name.md) | <code>string</code> | title in the UI |
| [optionLabels](./kibana-plugin-core-server.uisettingsparams.optionlabels.md) | <code>Record&lt;string, string&gt;</code> | text labels for 'select' type UI element |
| [options](./kibana-plugin-core-server.uisettingsparams.options.md) | <code>string[]</code> | array of permitted values for this setting |
Expand Down
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) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [UiSettingsParams](./kibana-plugin-core-server.uisettingsparams.md) &gt; [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;
};
```
6 changes: 6 additions & 0 deletions src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { TransportRequestParams } from '@elastic/elasticsearch/lib/Transport';
import { TransportRequestPromise } from '@elastic/elasticsearch/lib/Transport';
import { Type } from '@kbn/config-schema';
import { TypeOf } from '@kbn/config-schema';
import { UiStatsMetricType } from '@kbn/analytics';
import { UnregisterCallback } from 'history';
import { UserProvidedValues as UserProvidedValues_2 } from 'src/core/server/types';

Expand Down Expand Up @@ -1362,6 +1363,11 @@ export interface UiSettingsParams<T = unknown> {
// Warning: (ae-forgotten-export) The symbol "DeprecationSettings" needs to be exported by the entry point index.d.ts
deprecation?: DeprecationSettings;
description?: string;
// @deprecated
metric?: {
type: UiStatsMetricType;
name: string;
};
name?: string;
optionLabels?: Record<string, string>;
options?: string[];
Expand Down
6 changes: 6 additions & 0 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ import { TransportRequestParams } from '@elastic/elasticsearch/lib/Transport';
import { TransportRequestPromise } from '@elastic/elasticsearch/lib/Transport';
import { Type } from '@kbn/config-schema';
import { TypeOf } from '@kbn/config-schema';
import { UiStatsMetricType } from '@kbn/analytics';
import { UpdateDocumentByQueryParams } from 'elasticsearch';
import { UpdateDocumentParams } from 'elasticsearch';
import { URL } from 'url';
Expand Down Expand Up @@ -2746,6 +2747,11 @@ export interface UiSettingsParams<T = unknown> {
category?: string[];
deprecation?: DeprecationSettings;
description?: string;
// @deprecated
metric?: {
type: UiStatsMetricType;
name: string;
};
name?: string;
optionLabels?: Record<string, string>;
options?: string[];
Expand Down
10 changes: 10 additions & 0 deletions src/core/types/ui_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -80,6 +81,15 @@ export interface UiSettingsParams<T = unknown> {
* Used to validate value on write and read.
*/
schema: Type<T>;
/**
* Metric to track once this property changes
* @deprecated
* Temporary measure until https://github.com/elastic/kibana/issues/83084 is in place
*/
metric?: {
Copy link
Contributor

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

type: UiStatsMetricType;
name: string;
};
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/advanced_settings/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
"server": true,
"ui": true,
"requiredPlugins": ["management"],
"optionalPlugins": ["home"],
"optionalPlugins": ["home", "usageCollection"],
"requiredBundles": ["kibanaReact", "home"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { Subscription } from 'rxjs';
import { Comparators, EuiFlexGroup, EuiFlexItem, EuiSpacer, Query } from '@elastic/eui';

import { useParams } from 'react-router-dom';
import { UiStatsMetricType } from '@kbn/analytics';
import { CallOuts } from './components/call_outs';
import { Search } from './components/search';
import { Form } from './components/form';
Expand All @@ -39,6 +40,7 @@ interface AdvancedSettingsProps {
dockLinks: DocLinksStart['links'];
toasts: ToastsStart;
componentRegistry: ComponentRegistry['start'];
trackUiMetric?: (metricType: UiStatsMetricType, eventName: string | string[]) => void;
}

interface AdvancedSettingsComponentProps extends AdvancedSettingsProps {
Expand Down Expand Up @@ -241,6 +243,7 @@ export class AdvancedSettingsComponent extends Component<
enableSaving={this.props.enableSaving}
dockLinks={this.props.dockLinks}
toasts={this.props.toasts}
trackUiMetric={this.props.trackUiMetric}
/>
<PageFooter
toasts={this.props.toasts}
Expand All @@ -263,6 +266,7 @@ export const AdvancedSettings = (props: AdvancedSettingsProps) => {
dockLinks={props.dockLinks}
toasts={props.toasts}
componentRegistry={props.componentRegistry}
trackUiMetric={props.trackUiMetric}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -56,6 +57,7 @@ interface FormProps {
enableSaving: boolean;
dockLinks: DocLinksStart['links'];
toasts: ToastsStart;
trackUiMetric?: (metricType: UiStatsMetricType, eventName: string | string[]) => void;
}

interface FormState {
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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, stack_stats.kibana.plugins.stack_management already reports it when it's different to the default. Would that work?

Copy link
Contributor Author

@majagrubic majagrubic Nov 9, 2020

Choose a reason for hiding this comment

The 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"? 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?

Copy link
Member

Choose a reason for hiding this comment

The 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"? 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:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.
@alexfrancoeur

default:
equalsToDefault = valueToSave === defVal;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export function toEditableConfig({
options: def.options,
optionLabels: def.optionLabels,
requiresPageReload: !!def.requiresPageReload,
metric: def.metric,
};

return conf;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { ManagementAppMountParams } from '../../../management/public';
import { ComponentRegistry } from '../types';

import './index.scss';
import { UsageCollectionSetup } from '../../../usage_collection/public';

const title = i18n.translate('advancedSettings.advancedSettingsLabel', {
defaultMessage: 'Advanced Settings',
Expand All @@ -49,12 +50,14 @@ const readOnlyBadge = {
export async function mountManagementSection(
getStartServices: StartServicesAccessor,
params: ManagementAppMountParams,
componentRegistry: ComponentRegistry['start']
componentRegistry: ComponentRegistry['start'],
usageCollection?: UsageCollectionSetup
) {
params.setBreadcrumbs(crumb);
const [{ uiSettings, notifications, docLinks, application, chrome }] = await getStartServices();

const canSave = application.capabilities.advancedSettings.save as boolean;
const trackUiMetric = usageCollection?.reportUiStats.bind(usageCollection, 'advanced_settings');

if (!canSave) {
chrome.setBadge(readOnlyBadge);
Expand All @@ -71,6 +74,7 @@ export async function mountManagementSection(
dockLinks={docLinks.links}
uiSettings={uiSettings}
componentRegistry={componentRegistry}
trackUiMetric={trackUiMetric}
/>
</Route>
</Switch>
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/advanced_settings/public/management_app/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import { UiStatsMetricType } from '@kbn/analytics';
import { UiSettingsType, StringValidation, ImageValidation } from '../../../../core/public';

export interface FieldSetting {
Expand All @@ -39,6 +40,10 @@ export interface FieldSetting {
message: string;
docLinksKey: string;
};
metric?: {
type: UiStatsMetricType;
name: string;
};
}

// until eui searchbar and query are typed
Expand Down
12 changes: 10 additions & 2 deletions src/plugins/advanced_settings/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ const title = i18n.translate('advancedSettings.advancedSettingsLabel', {

export class AdvancedSettingsPlugin
implements Plugin<AdvancedSettingsSetup, AdvancedSettingsStart, AdvancedSettingsPluginSetup> {
public setup(core: CoreSetup, { management, home }: AdvancedSettingsPluginSetup) {
public setup(
core: CoreSetup,
{ management, home, usageCollection }: AdvancedSettingsPluginSetup
) {
const kibanaSection = management.sections.section.kibana;

kibanaSection.registerApp({
Expand All @@ -41,7 +44,12 @@ export class AdvancedSettingsPlugin
const { mountManagementSection } = await import(
'./management_app/mount_management_section'
);
return mountManagementSection(core.getStartServices, params, component.start);
return mountManagementSection(
core.getStartServices,
params,
component.start,
usageCollection
);
},
});

Expand Down
2 changes: 2 additions & 0 deletions src/plugins/advanced_settings/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ComponentRegistry } from './component_registry';
import { HomePublicPluginSetup } from '../../home/public';

import { ManagementSetup } from '../../management/public';
import { UsageCollectionSetup } from '../../usage_collection/public';

export interface AdvancedSettingsSetup {
component: ComponentRegistry['setup'];
Expand All @@ -32,6 +33,7 @@ export interface AdvancedSettingsStart {
export interface AdvancedSettingsPluginSetup {
management: ManagementSetup;
home?: HomePublicPluginSetup;
usageCollection?: UsageCollectionSetup;
}

export { ComponentRegistry };
1 change: 1 addition & 0 deletions src/plugins/data/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import { ShardsResponse } from 'elasticsearch';
import { ToastInputFields } from 'src/core/public/notifications';
import { Type } from '@kbn/config-schema';
import { TypeOf } from '@kbn/config-schema';
import { UiStatsMetricType } from '@kbn/analytics';
import { Unit } from '@elastic/datemath';

// Warning: (ae-forgotten-export) The symbol "AggConfigSerialized" needs to be exported by the entry point index.d.ts
Expand Down
7 changes: 6 additions & 1 deletion src/plugins/discover/server/ui_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.',
Copy link
Member

Choose a reason for hiding this comment

The 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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to get some statistics here!

},
},
};