Manage telemetry opt-in via a dedicated document by legrego · Pull Request #22268 · elastic/kibana

legrego · 2018-08-22T19:17:49Z

This PR introduces a new telemetry document to the .kibana index. This document replaces the existing xpack:telemetryOptIn UI Setting as the authoritative source of the user's telemetry opt-in preference.

The existing UI setting is marked as readonly, so it will no longer appear in the Advanced UI Settings screen. ~~Instead, this PR adds a new Telemetry Management screen to allow users to adjust their preference, and see an example of the type of information we collect.~~
Instead, this PR adds a section to the bottom of the Advanced Settings screen to allow users to adjust their preference, and see an example of the type of information we collect.

Latest Screenshot

Outdated Screenshot 2

Outdated Screenshot 1

Migration Logic

This adds to the existing handle_old_settings logic, which previously migrated older UI settings to the current xpack:telemetryOptIn setting. I extended this to keep most of that existing logic, but instead of migrating to the UI setting, it will now create the new telemetry document to track their preference (if set):

If the xpack:telemetryOptIn preference has been set, then the new telemetry document is created with that preference.
Otherwise, if the legacy opt-in settings are present, then those are used to determine the value of the new telemetry document.
If none of those settings have been set, then the telemetry document is not created by this migration process.
If the old settings existed, then they are removed after successfully migrating to the new telemetry document. Otherwise, the telemetry document is not created by this migration process, and will instead be created the when the user first opts in.

Fixes #22144

legrego · 2018-08-23T13:20:25Z

@pickypg @chrisronline @kobelb @cchaos @gchaps
I took a stab at what the new telemetry management screen might look like. This is still a WIP, but I'm interested in getting your feedback on what I have so far here. Feel free to tag anyone else who might also have an interest in this.

kobelb · 2018-08-23T13:43:49Z

My opinion on the design matters very little, but I think it's looks awesome.

legrego · 2018-08-23T17:02:25Z

jenkins, test this

cchaos

Just had a couple UI suggestions. Other than those. my one concern would be naming it "Telemetry" as I don't think most users will understand what that term means. I'd definitely get with @gchaps to come up with a better name.

Oh and I noticed you didn't touch this file, but the flyout in opt_in_details_component.js is way too small. Can you just remove the size="s" so it defaults to the medium size? And add the maxWidth prop so it doesn't get too wide on large screens.

cchaos · 2018-08-23T17:40:04Z

x-pack/plugins/xpack_main/public/views/management/telemetry/manage_telemetry_page.js

+                  />
+                </EuiFormRow>
+                <EuiFormRow hasEmptyLabelSpace={true}>
+                  <EuiLink onClick={this.toggleExample}>See an example of what we collect</EuiLink>


Move this link to be part of the form group description.

<Fragment> <p>{CONFIG_TELEMETRY_DESC}</p> <p><EuiLink>...</EuiLink></p> </Fragment>

cchaos · 2018-08-23T17:42:18Z

x-pack/plugins/xpack_main/public/views/management/telemetry/manage_telemetry_page.js

+
+    return (
+      <EuiPage className="manageTelemetryPage">
+        <EuiPageBody>


Add restrictWidth prop to EuiPageBody

cchaos · 2018-08-23T17:43:53Z

x-pack/plugins/xpack_main/public/views/management/telemetry/manage_telemetry_page.less

@@ -0,0 +1,4 @@
+.manageTelemetryPage {
+  background-color: #f5f5f5;


Unnecessary background color because it's already applied via .euiPage

cchaos · 2018-08-23T17:45:15Z

x-pack/plugins/xpack_main/public/views/management/telemetry/manage_telemetry_page.less

@@ -0,0 +1,4 @@
+.manageTelemetryPage {
+  background-color: #f5f5f5;
+  min-height: 100vh;


Can you set the min-height to: min-height: calc(~"100vh - 70px"); This will incorporate the management tabs.

cchaos · 2018-08-23T17:58:33Z

x-pack/plugins/xpack_main/public/views/management/telemetry/manage_telemetry_page.js

+    return (
+      <EuiPage className="manageTelemetryPage">
+        <EuiPageBody>
+          <EuiTitle size="l"><h1>Manage Telemetry</h1></EuiTitle>


Lowercase 't' in 'telemetry'

legrego · 2018-08-23T18:14:50Z

Thanks for the review, @cchaos!

cchaos

Looks great, Thanks!

elasticmachine · 2018-08-23T20:23:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

gchaps · 2018-08-23T21:08:55Z

My suggestion is that we name the tab "Usage Data" And then use the text shown in this screenshot.

I removed the text "Opt-in"

Keep the "Help us improve" paragraph the same as the text on the opening screen. Adding a link to the privacy statement is a good idea.

The text for the toggle button is "Send usage data to Elastic"

legrego · 2018-08-28T18:36:08Z

Following a meeting with @AlonaNadler, @kobelb, @bmcconaghy, and @cchaos, we decided to stay with the current approach of shimming the Usage Data setting into the Advanced UI Settings screen.

We will also shim this setting to respond to the search bar, in a rather forceful way for now. As settings evolve, we may consider a more pluggable or holistic approach.

elasticmachine · 2018-08-29T14:46:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang

I'm overall ok with this approach of including telemetry data in Advanced Settings, though I agree it would be nice to make this page extendable in the future. My main areas of concern are the following:

1) Visual difference between a regular setting and telemetry setting:

The spacing is not quite the same around the field and description. Functionally, a regular setting has "Save" and "Cancel" buttons to confirm changes, whereas the telemetry setting auto-saves on change.

This could potentially be addressed by exporting the settings' field component (/public/management/sections/settings/components/field/field.js) so it can be used in TelemetryForm. The Field component expects a certain structure for the setting prop, so some data wrangling will be needed, but it accepts a custom save function as a prop, as well as clear.

2) Visibility of telemetry setting is low, since it is at bottom of the page.

Typing a term in the search bar does work for bringing up telemetry, but Usage Data does not appear in the category filter. This could be addressed by using the settings component registry to append additional categories, which advanced_settings.js can retrieve. (I realize this isn't the intended use of the registry however, since it isn't a component! 😄)

Alternatively, PageFooter could be changed to PageHeader instead so that telemetry is the top category. I think it is an important enough setting to warrant a prime location.

I'm sure I'm repeating some thoughts which may have already been discussed, but let me know what you think! 🙂

legrego · 2018-08-30T20:12:25Z

Thanks, @jen-huang! I'll see what I can do to make this more visually and functionally similar to the rest of the settings on the page

elasticmachine · 2018-08-30T20:13:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

legrego · 2018-08-30T20:56:49Z

src/core_plugins/kibana/public/management/sections/settings/components/field/field.js

@@ -462,30 +462,40 @@ export class Field extends PureComponent {
  }

  renderDescription(setting) {


Sorry for the whitespace deltas in this file. The important change is here. The description used to always render html (dangerously, I might add), but now it will render a React element if one is provided. If not, then it falls back to the original behavior. I need this so that I can render EuiLinks within the usage data description

elasticmachine · 2018-08-30T22:32:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2018-08-31T12:22:12Z

@jen-huang I updated the look-and-feel so that it's much closer to the rest of the settings screen. I'm hesitant to add this to the category dropdown right now, but I think it's certainly worthwhile to do if/when we make this page more extensible for other plugins to add entries to this page.

legrego · 2018-09-04T12:06:44Z

@jen-huang are you available for another review? I'd like to merge this sooner rather than later if possible, to unblock other work for Spaces.

bmcconaghy

Code LGTM

bmcconaghy · 2018-09-06T12:41:42Z

x-pack/plugins/xpack_main/common/constants.js

+/**
+ * Link to the Elastic Telemetry privacy statement.
+ */
+export const PRIVACY_STATEMENT_URL = `https://www.elastic.co/legal/telemetry-privacy-statement`;


Nit: these don't need to be backticks as it's just a string.

legrego · 2018-09-07T16:28:22Z

retest

elasticmachine · 2018-09-07T18:11:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2018-09-10T11:29:09Z

6.x/6.5 #22877

Backports the following commits to 6.x: - Manage telemetry opt-in via a dedicated document (#22268)

legrego added 2 commits August 22, 2018 10:39

add mappings for new telemetry document

837be31

use new telemetry document, and expose management page to set this

ca46990

legrego added WIP Work in progress :Management Feature:Telemetry labels Aug 22, 2018

legrego added 3 commits August 22, 2018 16:33

fixed and added tests for handle_old_settings

0aa1090

fix and add replace_injected_vars tests

13520da

fix screen registry

5db484a

elastic deleted a comment from elasticmachine Aug 23, 2018

legrego added v6.5.0 v7.0.0 labels Aug 23, 2018

legrego mentioned this pull request Aug 23, 2018

[Spaces] Enable Per-Space Advanced Settings #19869

Closed

elastic deleted a comment from elasticmachine Aug 23, 2018

legrego force-pushed the telemetry-document branch from 72d8e7f to 9037097 Compare August 23, 2018 17:00

update telemetry class tests

9bc393f

legrego force-pushed the telemetry-document branch from 9037097 to 9bc393f Compare August 23, 2018 17:01

elastic deleted a comment from elasticmachine Aug 23, 2018

cchaos suggested changes Aug 23, 2018

View reviewed changes

address PR design feedback

a0f9a4e

cchaos approved these changes Aug 23, 2018

View reviewed changes

elastic deleted a comment from elasticmachine Aug 23, 2018

rename telemetry => usage data, and add link to privacy statement

5a86f9d

shim usage data into advanced settings search results

448adc8

legrego added 3 commits August 28, 2018 17:01

fix tests

524c3b0

fix snapshot

8125923

update expected privilege/action mapping

36e0ac0

elastic deleted a comment from elasticmachine Aug 29, 2018

additional UI tests

b26d430

jen-huang reviewed Aug 30, 2018

View reviewed changes

render telemetry setting using common Field component

19625a0

legrego commented Aug 30, 2018

View reviewed changes

legrego removed the WIP Work in progress label Aug 31, 2018

bmcconaghy approved these changes Sep 7, 2018

View reviewed changes

legrego merged commit 0a870b2 into elastic:master Sep 10, 2018

legrego added a commit to legrego/kibana that referenced this pull request Sep 10, 2018

Manage telemetry opt-in via a dedicated document (elastic#22268)

93aa531

legrego mentioned this pull request Sep 10, 2018

[6.x] Manage telemetry opt-in via a dedicated document (#22268) #22877

Merged

legrego mentioned this pull request Sep 10, 2018

Fix stale snapshot from #22268 #22880

Merged

legrego added a commit that referenced this pull request Sep 10, 2018

[6.x] Manage telemetry opt-in via a dedicated document (#22268) (#22877)

aa1c265

Backports the following commits to 6.x: - Manage telemetry opt-in via a dedicated document (#22268)

legrego deleted the telemetry-document branch September 18, 2018 15:35

		@@ -0,0 +1,4 @@
		.manageTelemetryPage {
		background-color: #f5f5f5;

		@@ -462,30 +462,40 @@ export class Field extends PureComponent {
		}

		renderDescription(setting) {

Conversation

legrego commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Latest Screenshot

Outdated Screenshot 2

Outdated Screenshot 1

Migration Logic

Uh oh!

legrego commented Aug 23, 2018

Uh oh!

kobelb commented Aug 23, 2018

Uh oh!

legrego commented Aug 23, 2018

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

legrego commented Aug 23, 2018

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 23, 2018

💔 Build Failed

Uh oh!

gchaps commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legrego commented Aug 28, 2018

Uh oh!

elasticmachine commented Aug 29, 2018

💚 Build Succeeded

Uh oh!

jen-huang left a comment

Choose a reason for hiding this comment

Uh oh!

legrego commented Aug 30, 2018

Uh oh!

elasticmachine commented Aug 30, 2018

💔 Build Failed

Uh oh!

legrego Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 30, 2018

💚 Build Succeeded

Uh oh!

legrego commented Aug 31, 2018

Uh oh!

legrego commented Sep 4, 2018

Uh oh!

bmcconaghy left a comment

Choose a reason for hiding this comment

Uh oh!

bmcconaghy Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

legrego commented Sep 7, 2018

Uh oh!

elasticmachine commented Sep 7, 2018

💚 Build Succeeded

Uh oh!

legrego commented Sep 10, 2018

Uh oh!

Reviewers

legrego commented Aug 22, 2018 •

edited

Loading

gchaps commented Aug 23, 2018 •

edited

Loading