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

Add UI telemetry tracking to AS in Kibana #5

Merged
merged 4 commits into from
Apr 28, 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
1 change: 1 addition & 0 deletions x-pack/plugins/enterprise_search/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"kibanaVersion": "kibana",
"requiredPlugins": ["home"],
"configPath": ["enterpriseSearch"],
"optionalPlugins": ["usageCollection"],
"server": true,
"ui": true
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import { EuiPage, EuiPageBody, EuiPageContent, EuiEmptyPrompt, EuiCode } from '@

import { EuiButton } from '../../../shared/react_router_helpers';
import { SetAppSearchBreadcrumbs as SetBreadcrumbs } from '../../../shared/kibana_breadcrumbs';
import { SendAppSearchTelemetry as SendTelemetry } from '../../../shared/telemetry';
import { KibanaContext, IKibanaContext } from '../../../index';

import { EngineOverviewHeader } from '../engine_overview_header';

import './empty_states.scss';
Expand All @@ -21,6 +21,7 @@ export const ErrorState: ReactFC<> = () => {
return (
<EuiPage restrictWidth className="empty-state">
<SetBreadcrumbs isRoot />
<SendTelemetry action="error" metric="cannot_connect" />

<EuiPageBody>
<EngineOverviewHeader />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import React from 'react';
import { EuiPage, EuiPageBody, EuiPageContent, EuiEmptyPrompt, EuiCode } from '@elastic/eui';

import { SetAppSearchBreadcrumbs as SetBreadcrumbs } from '../../../shared/kibana_breadcrumbs';
import { SendAppSearchTelemetry as SendTelemetry } from '../../../shared/telemetry';
import { EngineOverviewHeader } from '../engine_overview_header';
import { getUserName } from '../../utils/get_username';

Expand All @@ -19,6 +20,7 @@ export const NoUserState: React.FC<> = () => {
return (
<EuiPage restrictWidth className="empty-state">
<SetBreadcrumbs isRoot />
<SendTelemetry action="error" metric="no_as_account" />

<EuiPageBody>
<EngineOverviewHeader />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from '@elastic/eui';

import { SetAppSearchBreadcrumbs as SetBreadcrumbs } from '../../../shared/kibana_breadcrumbs';
import { SendAppSearchTelemetry as SendTelemetry } from '../../../shared/telemetry';
import { KibanaContext, IKibanaContext } from '../../../index';

import EnginesIcon from '../../assets/engine.svg';
Expand Down Expand Up @@ -96,6 +97,7 @@ export const EngineOverview: ReactFC<> = () => {
return (
<EuiPage restrictWidth className="engine-overview">
<SetBreadcrumbs isRoot />
<SendTelemetry action="viewed" metric="engines_overview" />

<EuiPageBody>
<EngineOverviewHeader />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import React, { useContext } from 'react';
import { EuiBasicTable, EuiLink } from '@elastic/eui';

import { sendTelemetry } from '../../../shared/telemetry';
import { KibanaContext, IKibanaContext } from '../../../index';

interface IEngineTableProps {
Expand All @@ -27,17 +28,24 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
data,
pagination: { totalEngines, pageIndex = 0, onPaginate },
}) => {
const { enterpriseSearchUrl } = useContext(KibanaContext) as IKibanaContext;
const { enterpriseSearchUrl, http } = useContext(KibanaContext) as IKibanaContext;
const engineLinkProps = {
href: `${enterpriseSearchUrl}/as/engines/${name}`,
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
target: '_blank',
onClick: () =>
sendTelemetry({
http,
product: 'app_search',
action: 'clicked',
metric: 'engine_table_link',
}),
};

const columns = [
{
field: 'name',
name: 'Name',
render: name => (
<EuiLink href={`${enterpriseSearchUrl}/as/engines/${name}`} target="_blank">
{name}
</EuiLink>
),
render: name => <EuiLink {...engineLinkProps}>{name}</EuiLink>,
width: '30%',
truncateText: true,
mobileOptions: {
Expand Down Expand Up @@ -78,11 +86,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
field: 'name',
name: 'Actions',
dataType: 'string',
render: name => (
<EuiLink href={`${enterpriseSearchUrl}/as/engines/${name}`} target="_blank" color="primary">
Manage
</EuiLink>
),
render: name => <EuiLink {...engineLinkProps}>Manage</EuiLink>,
align: 'right',
width: '100px',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
import React, { useContext } from 'react';
import { EuiPageHeader, EuiPageHeaderSection, EuiTitle, EuiButton } from '@elastic/eui';

import { sendTelemetry } from '../../../shared/telemetry';
import { KibanaContext, IKibanaContext } from '../../../index';

export const EngineOverviewHeader: React.FC<> = () => {
const { enterpriseSearchUrl } = useContext(KibanaContext) as IKibanaContext;
const { enterpriseSearchUrl, http } = useContext(KibanaContext) as IKibanaContext;

const buttonProps = {
fill: true,
Expand All @@ -20,6 +21,13 @@ export const EngineOverviewHeader: React.FC<> = () => {
if (enterpriseSearchUrl) {
buttonProps.href = `${enterpriseSearchUrl}/as`;
buttonProps.target = '_blank';
buttonProps.onClick = () =>

Choose a reason for hiding this comment

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

Just wondering out loud if this will execute, or if the page will navigate away before this can execute?

Copy link
Owner Author

@cee-chen cee-chen Apr 27, 2020

Choose a reason for hiding this comment

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

These are set on all target="_blank" links only, so it'll always work because the page itself isn't navigating away to anything.

I'm aware there's nothing stopping another dev from using the sendTelemetry helper on a non target blank link, but I don't think it's worth trying to enforce that right now because eventually (when all of AS is on Kibana) we won't have any links opening to a new AS tab in any case, and will probably lose this telemetry action.

Choose a reason for hiding this comment

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

Makes send to me.

Choose a reason for hiding this comment

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

I can't even type

sendTelemetry({
http,
product: 'app_search',
action: 'clicked',
metric: 'header_launch_button',
});
} else {
buttonProps.isDisabled = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
} from '@elastic/eui';

import { SetAppSearchBreadcrumbs as SetBreadcrumbs } from '../../../shared/kibana_breadcrumbs';
import { SendAppSearchTelemetry as SendTelemetry } from '../../../shared/telemetry';

import GettingStarted from '../../assets/getting_started.png';
import './setup_guide.scss';
Expand All @@ -32,6 +33,8 @@ export const SetupGuide: React.FC<> = () => {
return (
<EuiPage className="setup-guide">
<SetBreadcrumbs text="Setup Guide" />
<SendTelemetry action="viewed" metric="setup_guide" />

<EuiPageSideBar>
<EuiText color="subdued" size="s">
<strong>Setup Guide</strong>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export { sendTelemetry } from './send_telemetry';
export { SendAppSearchTelemetry } from './send_telemetry';
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { mount } from 'enzyme';

import { httpServiceMock } from 'src/core/public/mocks';
import { mountWithKibanaContext } from '../../test_utils/helpers';
import { sendTelemetry, SendAppSearchTelemetry } from './';

describe('Shared Telemetry Helpers', () => {
const httpMock = httpServiceMock.createSetupContract();

beforeEach(() => {
jest.clearAllMocks();
});

describe('sendTelemetry', () => {
it('successfully calls the server-side telemetry endpoint', () => {
sendTelemetry({
http: httpMock,
product: 'enterprise_search',
action: 'viewed',
metric: 'setup_guide',
});

expect(httpMock.put).toHaveBeenCalledWith('/api/enterprise_search/telemetry', {
headers: { 'content-type': 'application/json' },
body: '{"action":"viewed","metric":"setup_guide"}',
});
});

it('throws an error if the telemetry endpoint fails', () => {
const httpRejectMock = { put: () => Promise.reject() };

expect(sendTelemetry({ http: httpRejectMock })).rejects.toThrow('Unable to send telemetry');
});
});

describe('React component helpers', () => {
it('SendAppSearchTelemetry component', () => {
const wrapper = mountWithKibanaContext(
<SendAppSearchTelemetry action="clicked" metric="button" />,
{ http: httpMock }
);

expect(httpMock.put).toHaveBeenCalledWith('/api/app_search/telemetry', {
headers: { 'content-type': 'application/json' },
body: '{"action":"clicked","metric":"button"}',
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React, { useContext, useEffect } from 'react';

import { HttpHandler } from 'src/core/public';
import { KibanaContext, IKibanaContext } from '../../index';

interface ISendTelemetryProps {
action: 'viewed' | 'error' | 'clicked';
metric: string; // e.g., 'setup_guide'
}

interface ISendTelemetry extends ISendTelemetryProps {
http(): HttpHandler;
product: 'app_search' | 'workplace_search' | 'enterprise_search';
}

/**
* Base function - useful for non-component actions, e.g. clicks
*/

export const sendTelemetry = async ({ http, product, action, metric }: ISendTelemetry) => {
try {
await http.put(`/api/${product}/telemetry`, {
headers: { 'content-type': 'application/json' },
body: JSON.stringify({ action, metric }),
});
} catch (error) {
throw new Error('Unable to send telemetry');
}
};

/**
* React component helpers - useful for on-page-load/views
* TODO: SendWorkplaceSearchTelemetry and SendEnterpriseSearchTelemetry
*/

export const SendAppSearchTelemetry: React.FC<ISendTelemetryProps> = ({ action, metric }) => {
const { http } = useContext(KibanaContext) as IKibanaContext;

useEffect(() => {
sendTelemetry({ http, action, metric, product: 'app_search' });
}, [action, metric, http]);

Choose a reason for hiding this comment

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

Is there any valid use case where action, metric, or http would actually change and we'd want to update the metrics? Just wondering if this should be [] instead.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So this is an eslint thing, personally I do think it should be [] but when I set it to that eslint's react-hooks/exhaustive-deps rule flags an error: facebook/react#14920

There's a ton of discourse on it on the internet and I'm not a huge fan of the rule, but since it's Kibana's codebase we either play by their rules or put it in a lot of one-liner eslint disables 🤷

There's actually another TODO comment in engine_overview.tsx where I did end up disabling the line - I think there's another link in that comment that has more context/info.

Choose a reason for hiding this comment

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

Gotcha. I mean i think it will be fine as is, I'm not concerned about it.

Copy link
Owner Author

@cee-chen cee-chen Apr 27, 2020

Choose a reason for hiding this comment

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

Haha yea same, but I also personally wish that lint rule was a little less strict 😁


return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { registerTelemetryUsageCollector, incrementUICounter } from './telemetry';

/**
* Since these route callbacks are so thin, these serve simply as integration tests
* to ensure they're wired up to the lib functions correctly. Business logic is tested
* more thoroughly in the lib/telemetry tests.
*/
describe('App Search Telemetry Usage Collector', () => {
const makeUsageCollectorStub = jest.fn();
const registerStub = jest.fn();

const savedObjectsRepoStub = {
get: () => ({
attributes: {
'ui_viewed.setup_guide': 10,
'ui_viewed.engines_overview': 20,
'ui_error.cannot_connect': 3,
'ui_error.no_as_account': 4,
'ui_clicked.header_launch_button': 50,
'ui_clicked.engine_table_link': 60,
},
}),
incrementCounter: jest.fn(),
};
Comment on lines +18 to +30
Copy link
Owner Author

@cee-chen cee-chen Apr 28, 2020

Choose a reason for hiding this comment

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

Note to self - if we weren't specifically returning a mocked response in get, we could also use Kibana's savedObjectsRepositoryMock test helper (via import { savedObjectsRepositoryMock } from 'src/core/server/mocks';), but it was as easy or faster to just stub out what we needed here

const dependencies = {
usageCollection: {
makeUsageCollector: makeUsageCollectorStub,
registerCollector: registerStub,
},
savedObjects: {
createInternalRepository: jest.fn(() => savedObjectsRepoStub),
},
};

beforeEach(() => {
jest.clearAllMocks();
});

describe('registerTelemetryUsageCollector', () => {
it('should make and register the usage collector', () => {
registerTelemetryUsageCollector(dependencies);

expect(registerStub).toHaveBeenCalledTimes(1);
expect(makeUsageCollectorStub).toHaveBeenCalledTimes(1);
expect(makeUsageCollectorStub.mock.calls[0][0].type).toBe('app_search_kibana_telemetry');
});
});

describe('fetchTelemetryMetrics', () => {
it('should return existing saved objects data', async () => {
registerTelemetryUsageCollector(dependencies);
const savedObjectsCounts = await makeUsageCollectorStub.mock.calls[0][0].fetch();

expect(savedObjectsCounts).toEqual({
ui_viewed: {
setup_guide: 10,
engines_overview: 20,
},
ui_error: {
cannot_connect: 3,
no_as_account: 4,
},
ui_clicked: {
header_launch_button: 50,
engine_table_link: 60,
},
});
});

it('should not error & should return a default telemetry object if no saved data exists', async () => {
registerTelemetryUsageCollector({
...dependencies,
savedObjects: { createInternalRepository: () => ({}) },
});
const savedObjectsCounts = await makeUsageCollectorStub.mock.calls[0][0].fetch();

expect(savedObjectsCounts).toEqual({
ui_viewed: {
setup_guide: 0,
engines_overview: 0,
},
ui_error: {
cannot_connect: 0,
no_as_account: 0,
},
ui_clicked: {
header_launch_button: 0,
engine_table_link: 0,
},
});
});
});

describe('incrementUICounter', () => {
it('should increment the saved objects internal repository', async () => {
const { savedObjects } = dependencies;

const response = await incrementUICounter({
savedObjects,
uiAction: 'ui_clicked',
metric: 'button',
});

expect(savedObjectsRepoStub.incrementCounter).toHaveBeenCalledWith(
'app_search_kibana_telemetry',
'app_search_kibana_telemetry',
'ui_clicked.button'
);
expect(response).toEqual({ success: true });
});
});
});
Loading