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/opt in welcome screen #42110

Merged
merged 43 commits into from
Aug 27, 2019
Merged

Telemetry/opt in welcome screen #42110

merged 43 commits into from
Aug 27, 2019

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Jul 28, 2019

Summary

Add Telemetry Opt in card to welcome screen. Closes https://github.com/elastic/dev/issues/1075

The telemetry in opt in screen will be hidden in the following scenarios:

  1. OSS Kibana, since telemetry currently lives in x-pack.
  2. User already chose to opt-in or opt-out.
  3. Kibana config xpack.telemetry.banner: false.
  4. Kibana config: xpack.telemetry.enabled: false.

Scenarios:

  1. The user clicks "No":
    Opt in banner and this card will never be shown again. Telemetry is opted out.

  2. The user clicks "Yes":
    Opt in banner and this card will never be shown again. Telemetry is opted in.

  3. The user does not get the welcome screen
    Users who already have data on elasticsearch will not see the welcome screen, hence they will only see the opt in banner.

Step 1:

image

Step 2:

image

With example opened:

image

Dev Docs

Added Telemetry Opt in card to welcome screen. The aim is to increate increase opt in rate. Added tracking to measure the success of this change.

image

@Bamieh Bamieh added Feature:Telemetry release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Jul 28, 2019
@Bamieh Bamieh requested a review from a team July 28, 2019 15:13
@Bamieh Bamieh requested a review from a team as a code owner July 28, 2019 15:13
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

I tried to get this running, but can't get the welcome screen to show. I still get the old banner. Tried clearing my local storage, sessions and cookies but I just get this.

image

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ryankeairns
Copy link
Contributor

Quick look at the screenshots in the description, it reads that Step 1 has the first dot (under the box) as gray when it should be blue. In other words, the active slide should be associated with a blue dot.

This may just be a mixup in the description, as I've not run this locally to confirm, but just pointing it out in the even it needs to change.

Looks good!

@Bamieh
Copy link
Member Author

Bamieh commented Jul 29, 2019

@ryankeairns Thanks for the review! I just set the disabled attribute to true to the current card, so the grey one is the current one as it is not clickable. I would take any recommendation to improving the experience! Happy to chat over zoom/slack as well

@ryankeairns
Copy link
Contributor

@Bamieh for the active card, perhaps just use a regular EuiIcon (instead of EuiButtonIcon) and set the color to euiColorPrimary. For the other card (not currently visible), keep the EuiButtonIcon but set the color to disabled (don't actually set the button to disabled).

Unrelated, if the user clicks 'Try sample data' will they navigate away and potentially never reach the second card for telemetry?

*/

// @ts-ignore
export { TelemetryOptInProvider } from '../../../../../../src/legacy/core_plugins/kibana/public/home/telemetry_opt_in'; // eslint-disable-line
Copy link
Member

@tsullivan tsullivan Aug 21, 2019

Choose a reason for hiding this comment

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

// eslint-disable-line is not necessary here

This include needs to be tested out with a packaged version of the Kibana code. When Kibana is built, the location of x-pack moves. Relative imports out of x-pack this way, do not work afaik

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'm disabling eslint as it spreads the export statement into 3 lines as the import string is long. Then the @ts-ignore does not ignore the right line. Moving the @ts-ignore line between those 3 lines of the export statement also makes the linter complain.

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 did test this in a production build and it works fine; there are places in x-pack where its requiring code from src as well in a 100 different files (searched x-pack folder for ../src excluding test files)

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

I'm going to go ahead and request more changes and ask you to investigate the import made outside of x-pack to OSS code. While that obviously works when running from source, I want to make extra sure it won't break in a build

@tsullivan
Copy link
Member

@Bamieh

The user does not get the welcome screen

How do we test that in this PR branch?

I can add a quick fix by setting a timeout so that it gets populated after the loading screen is covering the screen.

I am ++ for a quick fix of the flash of the upper banner. Would it be possible to tweak some timing around there?

@Bamieh
Copy link
Member Author

Bamieh commented Aug 26, 2019

@tsullivan thanks for the review. To test this scenario:

  • The user does not get the welcome screen
    Users who already have data on elasticsearch will not see the welcome screen, hence they will only see the opt in banner.

You can press esc to skip the welcome screen dialog without specifying opt in or opt out. You should see the opt in banner then.

Another way to test this is by having some data in ES, then the welcome screen will not show in the first place; leaving the users with the opt in banner to enable telemetry.

@Bamieh
Copy link
Member Author

Bamieh commented Aug 26, 2019

@tsullivan @alexfrancoeur For the banner timeout enhancement I will create a separate PR for it as it seems out of scope for this PR.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM!

Reviewed the code changes and tested in a local build

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bamieh Bamieh merged commit 5917cd3 into master Aug 27, 2019
@Bamieh Bamieh deleted the telemetry/opt_in_welcome_screen branch August 27, 2019 10:35
Bamieh added a commit to Bamieh/kibana that referenced this pull request Aug 27, 2019
* refactor opt_in_message

* welcome optin card

* finish optin in welcome screen

* add  steps

* disable  current step

* remove checkbox option

* add metrics

* fix  typescript checks

* hide in oss

* update snapshots

* only require TelemetryOptInProvider if telemetry is enabled

* pass telemetry description from service

* remove x-pack import

* remove x-pack import

* update image

* update per design feedback

* update props

* fix in oss

* await before moving to next screen

* utlize bannerId in telemtry provider

* keep export in 1 line

* ts-ignore banner import

* add test

* update tests

* update translations

* update home tests

* remove extra license header

* showTelemetryOptIn -> shouldShowTelemetryOptIn

* remote extra EuiTexts

* update jest snapshot

* unencrypted telemetry example
Bamieh added a commit that referenced this pull request Aug 27, 2019
* refactor opt_in_message

* welcome optin card

* finish optin in welcome screen

* add  steps

* disable  current step

* remove checkbox option

* add metrics

* fix  typescript checks

* hide in oss

* update snapshots

* only require TelemetryOptInProvider if telemetry is enabled

* pass telemetry description from service

* remove x-pack import

* remove x-pack import

* update image

* update per design feedback

* update props

* fix in oss

* await before moving to next screen

* utlize bannerId in telemtry provider

* keep export in 1 line

* ts-ignore banner import

* add test

* update tests

* update translations

* update home tests

* remove extra license header

* showTelemetryOptIn -> shouldShowTelemetryOptIn

* remote extra EuiTexts

* update jest snapshot

* unencrypted telemetry example
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.4.0 v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants