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

pmuellr · 2019-10-29T15:26:37Z

Summary

If a user has previously opted out of telemetry, we are making a
change to re-prompt them when the version of Kibana they are
running changes. Previously, once opted out, they would never
get prompted again.

resolves #49519

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~This was checked for breaking API changes and was labeled appropriately~~
This includes a feature addition or change that requires a release note and was labeled appropriately

resolves elastic#49519 If a user has previously opted out of telemetry, we are making a change to re-prompt them when the version of Kibana they are running changes. Previously, once opted out, they would never get prompted again.

pmuellr · 2019-10-29T15:32:02Z

First pass at fixing #49519 - "Re-prompt for telemetry opt-in at each minor version upgrade, if not opted in"

This seems like it works, but not sure if there's a better approach.

We will start storing the kibana version used, when persisting the opt-in setting. When determining the opt-in value, we make a change if the value is false, in that we check if the version of Kiban used when it was set to false is the same as the current version of Kibana. If they're different, return a null value which indicates they should be prompted.

Not sure if we need a migration, since we're updating the SO mapping, but having the new field lastVersionChecked return null is perfectly fine.

Also not sure the best way to check the current Kibana version - server.config.get('pkg.version') seems to work, but perhaps there's something better (or we could cache it somewhere in the plugin at startup). Certainly this code won't work under NP.

elasticmachine · 2019-10-29T15:34:43Z

Pinging @elastic/kibana-stack-services (Team:Stack Services)

pmuellr · 2019-10-29T15:39:35Z

I've added label release_note:skip, but guessing we will probably want SOME KIND OF NOTE SOMEWHERE, since this is new behavior, and is likely unexpected by both customers and elasticians.

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts

elasticmachine · 2019-10-29T16:42:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9586b4f

gmmorris · 2019-10-29T17:17:05Z

Code LGTM and seems to be behaving as expected.

The only issue is that Patch version, once that's sorted 👍

pmuellr · 2019-10-29T17:40:28Z

@restrry pointed me at some existing code to get the Kibana version in NP, as well as some code that is checking Kibana versions

pmuellr · 2019-10-29T22:39:48Z

The branch today seems pretty good. Now testing major/minor, not just does the previous version != the current version.

Going to see about adding tests tomorrow. Should certainly be able to add some unit tests for the optIn calculator, not sure how functional tests might work, will need to investigate ...

elasticmachine · 2019-10-29T22:56:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1823bbd

gmmorris

Made some small notes about typing and what I think might be some unnecessary repetition, but none of it is blocking, so happy to approve and leave it to you what to do.

It would be nice to add an FTS test that runs across the process, but I'm not familiar enough to know how feasible that is, so up to you.

src/legacy/core_plugins/telemetry/index.ts

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts

mikecote

Code LGTM

pmuellr · 2019-10-30T13:00:06Z

Some notes on manual testing.

The changes involve a new lastVersionChecked field in the telemetry:telemetry SavedObject. When the currently existing field enabled is changed (boolean value, true: opt'd in, false: opt'd out, null: hasn't opt'd either way yet), we now write out the Kibana version being used. The check to display the opt-in banner used to be (is the enabled field == null), but now the banner will be displayed if you previously opt'd out AND the Kibana version you're running is a minor release or more later than when you opt'd out.

So, to test, we'll be dealing with the saved object, updating it with bogus values, deleting it, etc, and then reloading a Kibana page in the browser. And then checking it after clicking the banner, to ensure the values are as expected.

getting it:

curl -k https://elastic:changeme@localhost:5601/api/saved_objects/telemetry/telemetry

returns

{
  ...
  "attributes": {
    "enabled": false,
    "lastVersionChecked": "8.0.0"
  },
  ...
}

updating it:

curl -k -X PUT -H "kbn-xsrf: foo" -H "content-type: application/json" \
  https://elastic:changeme@localhost:5601/api/saved_objects/telemetry/telemetry \
  -d '{"attributes": {"enabled": false, "lastVersionChecked": "7.9.0"}}'

deleting it:

curl -k -X DELETE -H "kbn-xsrf: foo" https://elastic:changeme@localhost:5601/api/saved_objects/telemetry/telemetry

Bamieh

LGTM, a few nits

src/legacy/core_plugins/telemetry/index.ts

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts

elasticmachine · 2019-10-31T06:19:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 07be553

elasticmachine · 2019-10-31T18:24:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d5e3950

pmuellr · 2019-10-31T19:59:57Z

@elasticmachine merge upstream

pmuellr · 2019-10-31T20:02:14Z

Can't quite tell, but might have passed the last ci except for the failed prbot:outdated, so it's definitely time to Always Be Merging. Let's see ...

elasticmachine · 2019-10-31T20:53:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d9e5690

Bargs

Left a couple of minor suggestions but nothing that's an absolute must in my mind.

I agree with the other reviewers who said an end to end functional test would be nice to confirm the opt in shows up in the browser as expected, especially since we're relying on null as a convention to mean "show the opt in".

Bargs · 2019-10-31T20:21:07Z

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts

+  const { attributes }: { attributes: SavedObjectAttributes } = savedObject;
+
+  // if enabled is already null, return null
+  if (attributes.enabled == null) return null;


Suggested change

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

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

Can we change this to triple equals?

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.

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

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 :-)

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.

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 :-)

Bargs · 2019-10-31T20:25:39Z

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts

+  const currentSemver = parseSemver(currentKibanaVersion);
+
+  // if either version is invalid, return null
+  if (lastSemver == null || currentSemver == null) return null;


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.

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.

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.

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 ...

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

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.

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

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.

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.

Bargs · 2019-10-31T20:43:42Z

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.test.ts

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


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

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 :-)

Bargs · 2019-10-31T21:07:41Z

x-pack/test/api_integration/apis/telemetry/index.js

@@ -8,5 +8,6 @@ export default function ({ loadTestFile }) {
  describe('Telemetry', () => {
    loadTestFile(require.resolve('./telemetry'));
    loadTestFile(require.resolve('./telemetry_local'));
+    loadTestFile(require.resolve('./opt_in'));


This may be a historical question for someone else, but do you know why these API integration tests are under xpack when the code is in OSS?

Hahaha, great question, don't know exactly. I believe telemetry moved from x-pack to OSS (or at least not x-pack), the code was migrated, and of course it turns out the functional tests didn't actually HAVE to move, and so ... didn't.

Seemed out-of-scope for this PR, but I'll open a new issue to track it.

opened issue #49933 to track this

elasticmachine · 2019-10-31T23:13:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4b7902e

Bamieh

LGTM. Thanks!

Bamieh · 2019-11-04T09:33:11Z

@elasticmachine merge upstream

elasticmachine · 2019-11-04T10:50:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dc11b3e

…ic#49644) resolves elastic#49519 If a user has previously opted out of telemetry, this PR will cause them to be prompted again, when the major or minor version of Kibana changes. Previously, once opted out, they would never get prompted again.

… (#50026) resolves #49519 If a user has previously opted out of telemetry, this PR will cause them to be prompted again, when the major or minor version of Kibana changes. Previously, once opted out, they would never get prompted again.

… (#50025) resolves #49519 If a user has previously opted out of telemetry, this PR will cause them to be prompted again, when the major or minor version of Kibana changes. Previously, once opted out, they would never get prompted again.

pmuellr requested review from Bamieh and bmcconaghy October 29, 2019 15:32

pmuellr added v7.5.0 Feature:Telemetry Team:Stack Services labels Oct 29, 2019

pmuellr added the release_note:skip Skip the PR/issue when compiling release notes label Oct 29, 2019

gmmorris reviewed Oct 29, 2019

View reviewed changes

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts Outdated Show resolved Hide resolved

pmuellr changed the title ~~forces a re-prompt of telemetry opt-in when Kibana version changes~~ forces a re-prompt of telemetry opt-in when Kibana major/minor version changes Oct 29, 2019

change to do major/minor comparison

1823bbd

gmmorris self-requested a review October 30, 2019 08:36

gmmorris approved these changes Oct 30, 2019

View reviewed changes

src/legacy/core_plugins/telemetry/index.ts Show resolved Hide resolved

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts Outdated Show resolved Hide resolved

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts Outdated Show resolved Hide resolved

mikecote approved these changes Oct 30, 2019

View reviewed changes

Bamieh reviewed Oct 30, 2019

View reviewed changes

src/legacy/core_plugins/telemetry/index.ts Show resolved Hide resolved

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts Outdated Show resolved Hide resolved

src/legacy/core_plugins/telemetry/server/get_telemetry_opt_in.ts Show resolved Hide resolved

pmuellr changed the title ~~forces a re-prompt of telemetry opt-in when Kibana major/minor version changes~~ remind users about telemetry on each minor version Oct 30, 2019

pmuellr changed the title ~~remind users about telemetry on each minor version~~ [Telemetry] remind users about telemetry on each minor version Oct 30, 2019

pmuellr added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Oct 30, 2019

pmuellr changed the title ~~[Telemetry] remind users about telemetry on each minor version~~ [Telemetry] Remind users about telemetry on each minor version Oct 30, 2019

pmuellr added the v8.0.0 label Oct 30, 2019

add some functional tests

07be553

change FTR to get kibana version differently

d5e3950

Merge branch 'master' into telemetry/opt-in-version

d9e5690

Bargs approved these changes Oct 31, 2019

View reviewed changes

rearrange some of the test setup to avoid async issues

4b7902e

Bamieh approved these changes Nov 4, 2019

View reviewed changes

Merge branch 'master' into telemetry/opt-in-version

dc11b3e

pmuellr merged commit 5ba237a into elastic:master Nov 5, 2019

This was referenced Nov 5, 2019

[7.x] [Telemetry] Remind users about telemetry on each minor version (#49644) #50025

Merged

[7.5] [Telemetry] Remind users about telemetry on each minor version (#49644) #50026

Merged

pmuellr added the backported label Nov 5, 2019

alexfrancoeur mentioned this pull request Nov 12, 2019

Add updated notice text to banner #50245

Closed

alexfrancoeur mentioned this pull request Nov 15, 2019

[DOCS] 7.5.0 Release Notes #50425

Merged

1 task

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

	if (lastSemver == null \|\| currentSemver == null) return null;
	if (lastSemver === null \|\| currentSemver === null) return null;

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

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

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

Conversation

pmuellr commented Oct 29, 2019 • edited Loading

Summary

Checklist

For maintainers

pmuellr commented Oct 29, 2019

elasticmachine commented Oct 29, 2019

pmuellr commented Oct 29, 2019

elasticmachine commented Oct 29, 2019

💚 Build Succeeded

gmmorris commented Oct 29, 2019

pmuellr commented Oct 29, 2019

pmuellr commented Oct 29, 2019

elasticmachine commented Oct 29, 2019

💚 Build Succeeded

gmmorris left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

pmuellr commented Oct 30, 2019

Bamieh left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 31, 2019

💔 Build Failed

elasticmachine commented Oct 31, 2019

💔 Build Failed

pmuellr commented Oct 31, 2019

pmuellr commented Oct 31, 2019

elasticmachine commented Oct 31, 2019

💔 Build Failed

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 31, 2019

💚 Build Succeeded

Bamieh left a comment

Choose a reason for hiding this comment

Bamieh commented Nov 4, 2019

elasticmachine commented Nov 4, 2019

💚 Build Succeeded

pmuellr commented Oct 29, 2019 •

edited

Loading