Skip to content

Commit 51256b8

Browse files
afharoafharo
authored
[Telemetry] Fix optIn telemetry report bug (#64214) (#64294)
1 parent 0c86753 commit 51256b8

File tree

4 files changed

+137
-33
lines changed

4 files changed

+137
-33
lines changed

‎src/plugins/telemetry/public/mocks.ts‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,20 @@ import { httpServiceMock } from '../../../core/public/http/http_service.mock';
2525
import { notificationServiceMock } from '../../../core/public/notifications/notifications_service.mock';
2626
import { TelemetryService } from './services/telemetry_service';
2727
import { TelemetryNotifications } from './services/telemetry_notifications/telemetry_notifications';
28-
import { TelemetryPluginStart } from './plugin';
28+
import { TelemetryPluginStart, TelemetryPluginConfig } from './plugin';
29+
30+
// The following is to be able to access private methods
31+
/* eslint-disable dot-notation */
32+
33+
export interface TelemetryServiceMockOptions {
34+
reportOptInStatusChange?: boolean;
35+
config?: Partial<TelemetryPluginConfig>;
36+
}
2937

3038
export function mockTelemetryService({
3139
reportOptInStatusChange,
32-
}: { reportOptInStatusChange?: boolean } = {}) {
40+
config: configOverride = {},
41+
}: TelemetryServiceMockOptions = {}) {
3342
const config = {
3443
enabled: true,
3544
url: 'http://localhost',
@@ -39,14 +48,22 @@ export function mockTelemetryService({
3948
banner: true,
4049
allowChangingOptInStatus: true,
4150
telemetryNotifyUserAboutOptInDefault: true,
51+
...configOverride,
4252
};
4353

44-
return new TelemetryService({
54+
const telemetryService = new TelemetryService({
4555
config,
4656
http: httpServiceMock.createStartContract(),
4757
notifications: notificationServiceMock.createStartContract(),
4858
reportOptInStatusChange,
4959
});
60+
61+
const originalReportOptInStatus = telemetryService['reportOptInStatus'];
62+
telemetryService['reportOptInStatus'] = jest.fn().mockImplementation(optInPayload => {
63+
return originalReportOptInStatus(optInPayload); // Actually calling the original method
64+
});
65+
66+
return telemetryService;
5067
}
5168

5269
export function mockTelemetryNotifications({

‎src/plugins/telemetry/public/services/telemetry_service.test.ts‎

Lines changed: 86 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,31 @@ describe('TelemetryService', () => {
6767
});
6868

6969
describe('setOptIn', () => {
70+
it('does not call the api if canChangeOptInStatus==false', async () => {
71+
const telemetryService = mockTelemetryService({
72+
reportOptInStatusChange: false,
73+
config: { allowChangingOptInStatus: false },
74+
});
75+
expect(await telemetryService.setOptIn(true)).toBe(false);
76+
77+
expect(telemetryService['http'].post).toBeCalledTimes(0);
78+
});
79+
7080
it('calls api if canChangeOptInStatus', async () => {
71-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: false });
72-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
81+
const telemetryService = mockTelemetryService({
82+
reportOptInStatusChange: false,
83+
config: { allowChangingOptInStatus: true },
84+
});
7385
await telemetryService.setOptIn(true);
7486

7587
expect(telemetryService['http'].post).toBeCalledTimes(1);
7688
});
7789

7890
it('sends enabled true if optedIn: true', async () => {
79-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: false });
80-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
91+
const telemetryService = mockTelemetryService({
92+
reportOptInStatusChange: false,
93+
config: { allowChangingOptInStatus: true },
94+
});
8195
const optedIn = true;
8296
await telemetryService.setOptIn(optedIn);
8397

@@ -87,8 +101,10 @@ describe('TelemetryService', () => {
87101
});
88102

89103
it('sends enabled false if optedIn: false', async () => {
90-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: false });
91-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
104+
const telemetryService = mockTelemetryService({
105+
reportOptInStatusChange: false,
106+
config: { allowChangingOptInStatus: true },
107+
});
92108
const optedIn = false;
93109
await telemetryService.setOptIn(optedIn);
94110

@@ -98,29 +114,32 @@ describe('TelemetryService', () => {
98114
});
99115

100116
it('does not call reportOptInStatus if reportOptInStatusChange is false', async () => {
101-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: false });
102-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
103-
telemetryService['reportOptInStatus'] = jest.fn();
117+
const telemetryService = mockTelemetryService({
118+
reportOptInStatusChange: false,
119+
config: { allowChangingOptInStatus: true },
120+
});
104121
await telemetryService.setOptIn(true);
105122

106123
expect(telemetryService['reportOptInStatus']).toBeCalledTimes(0);
107124
expect(telemetryService['http'].post).toBeCalledTimes(1);
108125
});
109126

110127
it('calls reportOptInStatus if reportOptInStatusChange is true', async () => {
111-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: true });
112-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
113-
telemetryService['reportOptInStatus'] = jest.fn();
128+
const telemetryService = mockTelemetryService({
129+
reportOptInStatusChange: true,
130+
config: { allowChangingOptInStatus: true },
131+
});
114132
await telemetryService.setOptIn(true);
115133

116134
expect(telemetryService['reportOptInStatus']).toBeCalledTimes(1);
117135
expect(telemetryService['http'].post).toBeCalledTimes(1);
118136
});
119137

120138
it('adds an error toast on api error', async () => {
121-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: false });
122-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
123-
telemetryService['reportOptInStatus'] = jest.fn();
139+
const telemetryService = mockTelemetryService({
140+
reportOptInStatusChange: false,
141+
config: { allowChangingOptInStatus: true },
142+
});
124143
telemetryService['http'].post = jest.fn().mockImplementation((url: string) => {
125144
if (url === '/api/telemetry/v2/optIn') {
126145
throw Error('failed to update opt in.');
@@ -133,9 +152,13 @@ describe('TelemetryService', () => {
133152
expect(telemetryService['notifications'].toasts.addError).toBeCalledTimes(1);
134153
});
135154

155+
// This one should not happen because the entire method is fully caught but hey! :)
136156
it('adds an error toast on reportOptInStatus error', async () => {
137-
const telemetryService = mockTelemetryService({ reportOptInStatusChange: true });
138-
telemetryService.getCanChangeOptInStatus = jest.fn().mockReturnValue(true);
157+
const telemetryService = mockTelemetryService({
158+
reportOptInStatusChange: true,
159+
config: { allowChangingOptInStatus: true },
160+
});
161+
139162
telemetryService['reportOptInStatus'] = jest.fn().mockImplementation(() => {
140163
throw Error('failed to report OptIn Status.');
141164
});
@@ -146,4 +169,50 @@ describe('TelemetryService', () => {
146169
expect(telemetryService['notifications'].toasts.addError).toBeCalledTimes(1);
147170
});
148171
});
172+
173+
describe('getTelemetryUrl', () => {
174+
it('should return the config.url parameter', async () => {
175+
const url = 'http://test.com';
176+
const telemetryService = mockTelemetryService({
177+
config: { url },
178+
});
179+
180+
expect(telemetryService.getTelemetryUrl()).toBe(url);
181+
});
182+
});
183+
184+
describe('setUserHasSeenNotice', () => {
185+
it('should hit the API and change the config', async () => {
186+
const telemetryService = mockTelemetryService({
187+
config: { telemetryNotifyUserAboutOptInDefault: undefined },
188+
});
189+
190+
expect(telemetryService.userHasSeenOptedInNotice).toBe(undefined);
191+
expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false);
192+
await telemetryService.setUserHasSeenNotice();
193+
expect(telemetryService['http'].put).toBeCalledTimes(1);
194+
expect(telemetryService.userHasSeenOptedInNotice).toBe(true);
195+
expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(true);
196+
});
197+
198+
it('should show a toast notification if the request fail', async () => {
199+
const telemetryService = mockTelemetryService({
200+
config: { telemetryNotifyUserAboutOptInDefault: undefined },
201+
});
202+
203+
telemetryService['http'].put = jest.fn().mockImplementation((url: string) => {
204+
if (url === '/api/telemetry/v2/userHasSeenNotice') {
205+
throw Error('failed to update opt in.');
206+
}
207+
});
208+
209+
expect(telemetryService.userHasSeenOptedInNotice).toBe(undefined);
210+
expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false);
211+
await telemetryService.setUserHasSeenNotice();
212+
expect(telemetryService['http'].put).toBeCalledTimes(1);
213+
expect(telemetryService['notifications'].toasts.addError).toBeCalledTimes(1);
214+
expect(telemetryService.userHasSeenOptedInNotice).toBe(false);
215+
expect(telemetryService.getUserHasSeenOptedInNotice()).toBe(false);
216+
});
217+
});
149218
});

‎src/plugins/telemetry/public/services/telemetry_service.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,15 @@ export class TelemetryService {
122122
}
123123

124124
try {
125-
await this.http.post('/api/telemetry/v2/optIn', {
125+
// Report the option to the Kibana server to store the settings.
126+
// It returns the encrypted update to send to the telemetry cluster [{cluster_uuid, opt_in_status}]
127+
const optInPayload = await this.http.post<string[]>('/api/telemetry/v2/optIn', {
126128
body: JSON.stringify({ enabled: optedIn }),
127129
});
128130
if (this.reportOptInStatusChange) {
129-
await this.reportOptInStatus(optedIn);
131+
// Use the response to report about the change to the remote telemetry cluster.
132+
// If it's opt-out, this will be the last communication to the remote service.
133+
await this.reportOptInStatus(optInPayload);
130134
}
131135
this.isOptedIn = optedIn;
132136
} catch (err) {
@@ -162,7 +166,11 @@ export class TelemetryService {
162166
}
163167
};
164168

165-
private reportOptInStatus = async (OptInStatus: boolean): Promise<void> => {
169+
/**
170+
* Pushes the encrypted payload [{cluster_uuid, opt_in_status}] to the remote telemetry service
171+
* @param optInPayload [{cluster_uuid, opt_in_status}] encrypted by the server into an array of strings
172+
*/
173+
private reportOptInStatus = async (optInPayload: string[]): Promise<void> => {
166174
const telemetryOptInStatusUrl = this.getOptInStatusUrl();
167175

168176
try {
@@ -171,7 +179,7 @@ export class TelemetryService {
171179
headers: {
172180
'Content-Type': 'application/json',
173181
},
174-
body: JSON.stringify({ enabled: OptInStatus }),
182+
body: JSON.stringify(optInPayload),
175183
});
176184
} catch (err) {
177185
// Sending the ping is best-effort. Telemetry tries to send the ping once and discards it immediately if sending fails.

‎src/plugins/telemetry/server/routes/telemetry_opt_in.ts‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ import { Observable } from 'rxjs';
2222
import { take } from 'rxjs/operators';
2323
import { schema } from '@kbn/config-schema';
2424
import { IRouter } from 'kibana/server';
25-
import { TelemetryCollectionManagerPluginSetup } from 'src/plugins/telemetry_collection_manager/server';
25+
import {
26+
StatsGetterConfig,
27+
TelemetryCollectionManagerPluginSetup,
28+
} from 'src/plugins/telemetry_collection_manager/server';
2629
import { getTelemetryAllowChangingOptInStatus } from '../../common/telemetry_config';
2730
import { sendTelemetryOptInStatus } from './telemetry_opt_in_stats';
2831

@@ -79,23 +82,30 @@ export function registerTelemetryOptInRoutes({
7982
});
8083
}
8184

85+
const statsGetterConfig: StatsGetterConfig = {
86+
start: moment()
87+
.subtract(20, 'minutes')
88+
.toISOString(),
89+
end: moment().toISOString(),
90+
unencrypted: false,
91+
};
92+
93+
const optInStatus = await telemetryCollectionManager.getOptInStats(
94+
newOptInStatus,
95+
statsGetterConfig
96+
);
97+
8298
if (config.sendUsageFrom === 'server') {
8399
const optInStatusUrl = config.optInStatusUrl;
84100
await sendTelemetryOptInStatus(
85101
telemetryCollectionManager,
86102
{ optInStatusUrl, newOptInStatus },
87-
{
88-
start: moment()
89-
.subtract(20, 'minutes')
90-
.toISOString(),
91-
end: moment().toISOString(),
92-
unencrypted: false,
93-
}
103+
statsGetterConfig
94104
);
95105
}
96106

97107
await updateTelemetrySavedObject(context.core.savedObjects.client, attributes);
98-
return res.ok({});
108+
return res.ok({ body: optInStatus });
99109
}
100110
);
101111
}

0 commit comments

Comments
 (0)