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

[Telemetry] Remind users about telemetry on each minor version #49644

Merged
merged 10 commits into from
Nov 5, 2019
18 changes: 16 additions & 2 deletions src/legacy/core_plugins/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ const telemetry = (kibana: any) => {
// `config` is used internally and not intended to be set
config: Joi.string().default(Joi.ref('$defaultConfigPath')),
banner: Joi.boolean().default(true),
lastVersionChecked: Joi.string()
.allow('')
.default(''),
url: Joi.when('$dev', {
is: true,
then: Joi.string().default(
Expand Down Expand Up @@ -77,7 +80,8 @@ const telemetry = (kibana: any) => {
},
},
async replaceInjectedVars(originalInjectedVars: any, request: any) {
const telemetryOptedIn = await getTelemetryOptIn(request);
const currentKibanaVersion = getCurrentKibanaVersion(request.server);
const telemetryOptedIn = await getTelemetryOptIn({ request, currentKibanaVersion });

return {
...originalInjectedVars,
Expand All @@ -97,7 +101,13 @@ const telemetry = (kibana: any) => {
mappings,
},
init(server: Server) {
const initializerContext = {} as PluginInitializerContext;
const initializerContext = {
env: {
packageInfo: {
version: getCurrentKibanaVersion(server),
},
},
} as PluginInitializerContext;
pmuellr marked this conversation as resolved.
Show resolved Hide resolved

const coreSetup = ({
http: { server },
Expand All @@ -116,3 +126,7 @@ const telemetry = (kibana: any) => {

// eslint-disable-next-line import/no-default-export
export default telemetry;

function getCurrentKibanaVersion(server: Server): string {
return server.config().get('pkg.version');
}
3 changes: 3 additions & 0 deletions src/legacy/core_plugins/telemetry/mappings.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
"properties": {
"enabled": {
"type": "boolean"
},
"lastVersionChecked": {
"type": "keyword"
}
}
}
Expand Down
214 changes: 214 additions & 0 deletions src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { getTelemetryOptIn } from './get_telemetry_opt_in';

describe('get_telemetry_opt_in', () => {
it('returns false when request path is not /app*', async () => {
const params = getCallGetTelemetryOptInParams({
requestPath: '/foo/bar',
});

const result = await callGetTelemetryOptIn(params);

expect(result).toBe(false);
});

it('returns null when saved object not found', async () => {
const params = getCallGetTelemetryOptInParams({
savedObjectNotFound: true,
});

const result = await callGetTelemetryOptIn(params);

expect(result).toBe(null);
});

it('returns false when saved object forbidden', async () => {
const params = getCallGetTelemetryOptInParams({
savedObjectForbidden: true,
});

const result = await callGetTelemetryOptIn(params);

expect(result).toBe(false);
});

it('throws an error on unexpected saved object error', async () => {
const params = getCallGetTelemetryOptInParams({
savedObjectOtherError: true,
});

let threw = false;
try {
await callGetTelemetryOptIn(params);
} catch (err) {
threw = true;
expect(err.message).toBe(SavedObjectOtherErrorMessage);
}

expect(threw).toBe(true);
Comment on lines +58 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an FYI, Jest has helpers to make asserting on exceptions easier https://jestjs.io/docs/en/expect#tothrowerror

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, I've used them before, but I couldn't get it to work this time. For some reason, it wouldn't catch the exception in the jest test, but then also claimed the exception that got thrown wasn't handled, and I was .... waaa??? ... spent 20 minutes trying to figure it out in the debugger, gave up, did it by hand :-)

});

it('returns null if enabled is null or undefined', async () => {
for (const enabled of [null, undefined]) {
const params = getCallGetTelemetryOptInParams({
enabled,
});

const result = await callGetTelemetryOptIn(params);

expect(result).toBe(null);
}
});

it('returns true when enabled is true', async () => {
const params = getCallGetTelemetryOptInParams({
enabled: true,
});

const result = await callGetTelemetryOptIn(params);

expect(result).toBe(true);
});

// build a table of tests with version checks, with results for enabled false
type VersionCheckTable = Array<Partial<CallGetTelemetryOptInParams>>;

const EnabledFalseVersionChecks: VersionCheckTable = [
{ lastVersionChecked: '8.0.0', currentKibanaVersion: '8.0.0', result: false },
{ lastVersionChecked: '8.0.0', currentKibanaVersion: '8.0.1', result: false },
{ lastVersionChecked: '8.0.1', currentKibanaVersion: '8.0.0', result: false },
{ lastVersionChecked: '8.0.0', currentKibanaVersion: '8.1.0', result: null },
{ lastVersionChecked: '8.0.0', currentKibanaVersion: '9.0.0', result: null },
{ lastVersionChecked: '8.0.0', currentKibanaVersion: '7.0.0', result: false },
{ lastVersionChecked: '8.1.0', currentKibanaVersion: '8.0.0', result: false },
{ lastVersionChecked: '8.0.0-X', currentKibanaVersion: '8.0.0', result: false },
{ lastVersionChecked: '8.0.0', currentKibanaVersion: '8.0.0-X', result: false },
{ lastVersionChecked: null, currentKibanaVersion: '8.0.0', result: null },
{ lastVersionChecked: undefined, currentKibanaVersion: '8.0.0', result: null },
{ lastVersionChecked: 5, currentKibanaVersion: '8.0.0', result: null },
{ lastVersionChecked: '8.0.0', currentKibanaVersion: 'beta', result: null },
{ lastVersionChecked: 'beta', currentKibanaVersion: '8.0.0', result: null },
{ lastVersionChecked: 'beta', currentKibanaVersion: 'beta', result: false },
{ lastVersionChecked: 'BETA', currentKibanaVersion: 'beta', result: null },
].map(el => ({ ...el, enabled: false }));

// build a table of tests with version checks, with results for enabled true/null/undefined
const EnabledTrueVersionChecks: VersionCheckTable = EnabledFalseVersionChecks.map(el => ({
...el,
enabled: true,
result: true,
}));

const EnabledNullVersionChecks: VersionCheckTable = EnabledFalseVersionChecks.map(el => ({
...el,
enabled: null,
result: null,
}));

const EnabledUndefinedVersionChecks: VersionCheckTable = EnabledFalseVersionChecks.map(el => ({
...el,
enabled: undefined,
result: null,
}));

const AllVersionChecks = [
...EnabledFalseVersionChecks,
...EnabledTrueVersionChecks,
...EnabledNullVersionChecks,
...EnabledUndefinedVersionChecks,
];

test.each(AllVersionChecks)(
'returns expected result for version check with %j',
async (params: Partial<CallGetTelemetryOptInParams>) => {
const result = await callGetTelemetryOptIn({ ...DefaultParams, ...params });
expect(result).toBe(params.result);
}
);
});

interface CallGetTelemetryOptInParams {
requestPath: string;
savedObjectNotFound: boolean;
savedObjectForbidden: boolean;
savedObjectOtherError: boolean;
enabled: boolean | null | undefined;
lastVersionChecked?: any; // should be a string, but test with non-strings
currentKibanaVersion: string;
result?: boolean | null;
}

const DefaultParams = {
requestPath: '/app/something',
savedObjectNotFound: false,
savedObjectForbidden: false,
savedObjectOtherError: false,
enabled: true,
lastVersionChecked: '8.0.0',
currentKibanaVersion: '8.0.0',
};

function getCallGetTelemetryOptInParams(
overrides: Partial<CallGetTelemetryOptInParams>
): CallGetTelemetryOptInParams {
return { ...DefaultParams, ...overrides };
}

async function callGetTelemetryOptIn(params: CallGetTelemetryOptInParams): Promise<boolean | null> {
const { currentKibanaVersion } = params;
const request = getMockRequest(params);
return await getTelemetryOptIn({ request, currentKibanaVersion });
}

function getMockRequest(params: CallGetTelemetryOptInParams): any {
return {
path: params.requestPath,
getSavedObjectsClient() {
return getMockSavedObjectsClient(params);
},
};
}

const SavedObjectNotFoundMessage = 'savedObjectNotFound';
const SavedObjectForbiddenMessage = 'savedObjectForbidden';
const SavedObjectOtherErrorMessage = 'savedObjectOtherError';

function getMockSavedObjectsClient(params: CallGetTelemetryOptInParams) {
return {
async get(type: string, id: string) {
if (params.savedObjectNotFound) throw new Error(SavedObjectNotFoundMessage);
if (params.savedObjectForbidden) throw new Error(SavedObjectForbiddenMessage);
if (params.savedObjectOtherError) throw new Error(SavedObjectOtherErrorMessage);

const enabled = params.enabled;
const lastVersionChecked = params.lastVersionChecked;
return { attributes: { enabled, lastVersionChecked } };
},
errors: {
isNotFoundError(error: any) {
return error.message === SavedObjectNotFoundMessage;
},
isForbiddenError(error: any) {
return error.message === SavedObjectForbiddenMessage;
},
},
};
}
66 changes: 63 additions & 3 deletions src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,21 @@
* under the License.
*/

export async function getTelemetryOptIn(request: any) {
import semver from 'semver';

import { SavedObjectAttributes } from './routes/opt_in';

interface GetTelemetryOptIn {
request: any;
currentKibanaVersion: string;
}

// Returns whether telemetry has been opt'ed into or not.
// Returns null not set, meaning Kibana should prompt in the UI.
export async function getTelemetryOptIn({
request,
currentKibanaVersion,
}: GetTelemetryOptIn): Promise<boolean | null> {
const isRequestingApplication = request.path.startsWith('/app');

// Prevent interstitial screens (such as the space selector) from prompting for telemetry
Expand All @@ -27,9 +41,9 @@ export async function getTelemetryOptIn(request: any) {

const savedObjectsClient = request.getSavedObjectsClient();

let savedObject;
try {
const { attributes } = await savedObjectsClient.get('telemetry', 'telemetry');
return attributes.enabled;
savedObject = await savedObjectsClient.get('telemetry', 'telemetry');
} catch (error) {
if (savedObjectsClient.errors.isNotFoundError(error)) {
return null;
Expand All @@ -43,4 +57,50 @@ export async function getTelemetryOptIn(request: any) {

throw error;
}

const { attributes }: { attributes: SavedObjectAttributes } = savedObject;

// if enabled is already null, return null
if (attributes.enabled == null) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (attributes.enabled == null) return null;
if (attributes.enabled === null) return null;

Can we change this to triple equals?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's against my nature, since == catches undefined as well as null. I could be convinced otherwise, but don't remember doing a === null in a long time, unless I was specifically wanting to check null vs undefined for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

In what valid situation would attributes.enabled be undefined, and why should we return null in that situation?

Copy link
Member Author

Choose a reason for hiding this comment

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

If someone (maybe us, accidently??) reset the saved object attributes to {}, the enabled field would be undefined, not null. In questionable cases like this (as well as if the semver wasn't parseable), I've made some arbitrary decisions, and in most cases, you'd want this to be considered "re-prompt the user".

It's easy for me to imagine folks wanting to defeat banner-ism by hacking saved objects, so ... I've tried to be pretty conservative here in our checks of things users might be "providing" to use :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

If someone (maybe us, accidently??) reset the saved object attributes to {}, the enabled field would be undefined, not null.

In this scenario the fact that enabled is undefined is a bug and IMO we shouldn't act like it means something it doesn't. Looseness like this leads to subtle bugs that are hard to fix.

That said, the stakes are low in this context, so if you don't want to make the change we can simply agree to disagree.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we'll have to agree to disagree at this point, but would welcome some kind of project-wide guidance on this. As noted elsewhere, I think I lived for a while with a linter than MADE you use == null, so I'll admit my brain may be warped.

Still, seems like unless you are specifically expecting a null OR undefined, as semantically different things, from an API, you should always just use == null for such checks. Also note, there are plenty of folks who shorten this check to !x (from x == null), since it's often the case that this works as well, but those ones do kinda bug me :-)


const enabled = !!attributes.enabled;

// if enabled is true, return it
if (enabled === true) return enabled;

// Additional check if they've already opted out (enabled: false):
// - if the Kibana version has changed by at least a minor version,
// return null to re-prompt.

const lastKibanaVersion = attributes.lastVersionChecked;

// if the last kibana version isn't set, or is somehow not a string, return null
if (typeof lastKibanaVersion !== 'string') return null;

// if version hasn't changed, just return enabled value
if (lastKibanaVersion === currentKibanaVersion) return enabled;

const lastSemver = parseSemver(lastKibanaVersion);
const currentSemver = parseSemver(currentKibanaVersion);

// if either version is invalid, return null
if (lastSemver == null || currentSemver == null) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (lastSemver == null || currentSemver == null) return null;
if (lastSemver === null || currentSemver === null) return null;

Triple equals would be nice here as well if semver specifically returns null when there is a parse error.

Copy link
Member Author

Choose a reason for hiding this comment

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

for cases like this, where there's a local function I can validate won't return undefined, I'm happier to to use a === check, but still kind of consider it to be bad form, for future-proofing in case some day the called function DOES end up returning undefined.

I'm curious why you're looking for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs for semver's parse specifically state that null is returned if the value can't be parsed. You're guessing that in the future, if parse starts to return undefined, that undefined will mean the same as null. But that is not necessarily the case. Some future version of parse might return undefined for some other reason, and then we might have a bug on our hands because this condition is overly loose. I don't consider null and undefined interchangeable, they can mean two very different things depending on context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some future version of parse() might return undefined for some other reason, and then we might have a bug on our hands because this condition is overly loose.

I guess I'd say, based on my years-long, unhappy history with semver (as much as I don't like it, it gets a lot of use, so it's the best out there, as long as you protect yourself), that we'd more likely see some future of version of parse() return undefined where it returns null today, and then we might have a bug on our hands because === null is overly strict. :-)

I don't trust 3rd party libraries much ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's also the case that I've used linters (standard?) that used to complain if you ever DID do an === null or === undefined. Happened to fit with my world view, so was happy with it. Or was I brainwashed?!?! heh

Copy link
Contributor

Choose a reason for hiding this comment

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

that we'd more likely see some future of version of parse() return undefined where it returns null today, and then we might have a bug on our hands because === null is overly strict. :-)

Nope, because our unit tests would catch this. The === null condition would fail and we would get an unexpected result. This is the benefit of being strict. On the other hand, our unit tests would likely not catch the bug in my example because the looseness of the check may cover up an issue. Semver might respond to some issue by returning undefined but this == check will make it look like everything is fine.

my years-long, unhappy history with semver

My decades long, unhappy history with Javascript has taught me to never use double equals :) It's not just me:

https://2ality.com/2011/12/strict-equality-exemptions.html (look especially at Case 2)

https://stackoverflow.com/a/359509 (note in particular the quote from Javascript: The Good Parts)

Preferring triples equals is a best practice most of the community agrees upon.

But like I said above, the stakes in this particular situation are probably low, so if you are not convinced, we can agree to disagree.

Copy link
Member Author

Choose a reason for hiding this comment

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

pro-tip: you will never convince me of anything based on references to "JavaScript: The Good Parts". hehe

I'm slightly curious, because I thought triple quotes were best practice for number/boolean/string, but double quotes was actually best practice for null/undefined.

It would certainly be interesting to add a lint rule disallowing == null and see what the vsCode Problems page looks like.

quick search:

search results files
== null 600 236
=== null 339 177
=== undefined 573 284
== undefined 8 1 hahahaha

Copy link
Contributor

Choose a reason for hiding this comment

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

I was attempting an appeal to authority out of frustration since the concrete explanations I gave you are for some reason unconvincing. You didn't actually respond to the concrete explanation I gave about this specific piece of code we're looking at. You also ignored the information from the first link I provided, which has a section (which I called out) that specifically deals with this particular use of ==. Instead you made a flippant comment about Javascript: The Good Parts and listed a statistic from the existing code base which is irrelevant to the discussion of whether using == is actually a good idea, or if it is appropriate to use in this specific context. You also keep noting that the default ruleset of some linter you used encouraged use of ==. This is also an appeal to authority, except the authority is anonymous and their reasoning is unknown, so I would say this is also irrelevant to this specific conversation.

I won't reply anymore, because I'm obviously extremely frustrated and this is not a productive conversation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: sent a note to Bargs offline, as I didn't realize his frustration level on this - I thought the issue was resolved but we were just having some banter on the topic of == null.


// actual major/minor version comparison, for cases when to return null
if (currentSemver.major > lastSemver.major) return null;
if (currentSemver.major === lastSemver.major) {
if (currentSemver.minor > lastSemver.minor) return null;
}

// current version X.Y is not greater than last version X.Y, return enabled
return enabled;
}

function parseSemver(version: string): semver.SemVer | null {
// semver functions both return nulls AND throw exceptions: "it depends!"
try {
return semver.parse(version);
pmuellr marked this conversation as resolved.
Show resolved Hide resolved
} catch (err) {
return null;
}
}
2 changes: 1 addition & 1 deletion src/legacy/core_plugins/telemetry/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ export { getTelemetryOptIn } from './get_telemetry_opt_in';
export { telemetryCollectionManager } from './collection_manager';

export const telemetryPlugin = (initializerContext: PluginInitializerContext) =>
new TelemetryPlugin();
new TelemetryPlugin(initializerContext);
export { constants };
11 changes: 9 additions & 2 deletions src/legacy/core_plugins/telemetry/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,21 @@
* under the License.
*/

import { CoreSetup } from 'src/core/server';
import { CoreSetup, PluginInitializerContext } from 'src/core/server';
import { registerRoutes } from './routes';
import { telemetryCollectionManager } from './collection_manager';
import { getStats } from './telemetry_collection';

export class TelemetryPlugin {
private readonly currentKibanaVersion: string;

constructor(initializerContext: PluginInitializerContext) {
this.currentKibanaVersion = initializerContext.env.packageInfo.version;
}

public setup(core: CoreSetup) {
const currentKibanaVersion = this.currentKibanaVersion;
telemetryCollectionManager.setStatsGetter(getStats, 'local');
registerRoutes(core);
registerRoutes({ core, currentKibanaVersion });
}
}
9 changes: 7 additions & 2 deletions src/legacy/core_plugins/telemetry/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@ import { CoreSetup } from 'src/core/server';
import { registerOptInRoutes } from './opt_in';
import { registerTelemetryDataRoutes } from './telemetry_stats';

export function registerRoutes(core: CoreSetup) {
registerOptInRoutes(core);
interface RegisterRoutesParams {
core: CoreSetup;
currentKibanaVersion: string;
}

export function registerRoutes({ core, currentKibanaVersion }: RegisterRoutesParams) {
registerOptInRoutes({ core, currentKibanaVersion });
registerTelemetryDataRoutes(core);
}
Loading