[Reporting/Telemetry] Do not send telemetry if we are in screenshot mode by jloleysens · Pull Request #100388 · elastic/kibana

jloleysens · 2021-05-20T10:46:17Z

Summary

Fix #41402

To reviewers

Is this the correct place to intercept the process of sending telemetry? Are some calls happening that can still be avoided?

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

Bamieh

code changes LGTM! a very welcomed changed :elasticheart:

Can we add a functional test do make sure this is working in the browser as intended?

afharo

Thank you for these changes! This will prove super useful!

I've added a couple of comments & NITs.

afharo · 2021-05-20T11:22:52Z

src/plugins/telemetry/public/plugin.ts

  ApplicationStart,
 } from 'src/core/public';

+import { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/public';


NIT:

Suggested change

import { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/public';

import type { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/public';

afharo · 2021-05-20T11:24:22Z

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

  });
+
+  describe('shouldSendTelemetry', () => {
+    it('does not send telemetry if screenshotMode is true', () => {


Can we test the other scenario as well?

afharo · 2021-05-20T11:25:30Z

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

    return this.isOptedIn;
  };

+  public canSendTelemetry = (): boolean => {


Would you mind adding the JSDocs to this public method?

afharo · 2021-05-20T11:28:45Z

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

  };

+  public canSendTelemetry = (): boolean => {
+    return Boolean(this.getIsOptedIn() && !this.isScreenshotMode);


NITs:

Why do we need to wrap it as Boolean?

Can we check isScreenshotMode first to avoid running the additional checks of getIsOptedIn()?

Why do we need to wrap it as Boolean?

I wanted to be explicit about the return type and getIsOptedIn returns a boolean | null. Happy to update that function return value with a Boolean() instead?

Can we check isScreenshotMode first to avoid running the additional checks of getIsOptedIn()?

Can do!

* added another Jest test * move Boolean() to make the opt-in value always boolean

- added plugin functional test - added jest test to check TelemetrySender behaviour - exported the localStorage/window value that flags screenshot mode

jloleysens · 2021-05-25T09:05:23Z

@elasticmachine merge upstream

…metry when the test finishes

jloleysens · 2021-05-25T09:53:32Z

@Bamieh @afharo I think I've addressed your feedback, would you mind taking another look?

jloleysens · 2021-05-25T15:10:15Z

@elasticmachine merge upstream

tsullivan

LGTM

elasticmachine · 2021-05-26T07:38:44Z

Pinging @elastic/kibana-app-services (Team:AppServices)

elasticmachine · 2021-05-26T07:38:45Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

jloleysens · 2021-05-28T16:29:15Z

@elasticmachine merge upstream

Bamieh

LGTM. @afharo's comments are addressed. He's on PTO so we can dismiss his review

Bamieh · 2021-05-31T07:42:36Z

@elasticmachine merge upstream

src/plugins/telemetry/public/plugin.ts

jloleysens · 2021-06-01T13:19:53Z

@elasticmachine merge upstream

kibanamachine · 2021-06-01T15:06:21Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`screenshotMode`	11	16	+5

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`screenshotMode`	0	1	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`screenshotMode`	3.0KB	3.2KB	+191.0B
`telemetry`	36.4KB	36.8KB	+397.0B
total			+588.0B

Unknown metric groups

API count

id	before	after	diff
`screenshotMode`	15	21	+6
`telemetry`	41	42	+1
total			+7

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`infra`	261	149	-112
`lens`	67	45	-22
`licensing`	18	15	-3
`lists`	239	236	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`securitySolution`	390	346	-44
`stackAlerts`	101	95	-6
total			-342

History

💚 Build #128129 succeeded f00a895
💚 Build #127978 succeeded dca59ac
💚 Build #127148 succeeded 3d5e140
💚 Build #127149 succeeded e0a0b94
💔 Build #127059 failed fec95b6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ode (elastic#100388) * do not send telemetry if isScreenshotMode * Implement PR feedback: * added another Jest test * move Boolean() to make the opt-in value always boolean * remove unused import and convert to import type * fix type issues * update jest snapshot * Expanded test coverage - added plugin functional test - added jest test to check TelemetrySender behaviour - exported the localStorage/window value that flags screenshot mode * fix test plugin name in package.json and make sure to opt out of telemetry when the test finishes * added missing type file to plugin_functional test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # src/plugins/telemetry/kibana.json

…ode (#100388) (#101076) * do not send telemetry if isScreenshotMode * Implement PR feedback: * added another Jest test * move Boolean() to make the opt-in value always boolean * remove unused import and convert to import type * fix type issues * update jest snapshot * Expanded test coverage - added plugin functional test - added jest test to check TelemetrySender behaviour - exported the localStorage/window value that flags screenshot mode * fix test plugin name in package.json and make sure to opt out of telemetry when the test finishes * added missing type file to plugin_functional test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # src/plugins/telemetry/kibana.json

…sens/kibana into reporting/new-png-pdf-report-type * 'reporting/new-png-pdf-report-type' of github.com:jloleysens/kibana: (46 commits) [Security Solution] Add Ransomware canary advanced policy option (elastic#101068) [Exploratory view] Core web vitals (elastic#100320) [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034) [Lens] Use a setter function for the dimension panel (elastic#101123) [Index Patterns] Fix return saved index pattern object (elastic#101051) [CI] For PRs, build TS refs before public api docs check (elastic#100791) [Maps] fix line and polygon label regression (elastic#101085) Migrate CCR to new ES JS client. (elastic#100131) [Canvas] Switch Canvas to use React Router (elastic#100579) [Expressions] Use table column ID instead of name when set (elastic#99724) [DOCS] Updates docs landing page (elastic#100749) [DOCS] Corrects typo in step 3 (elastic#101079) [DOCS] Updates runtime example in Discover (elastic#100926) Migrate kibana.autocomplete config to data plugin (elastic#100586) [Uptime] New width/delay definition for waterfall sidebar item tooltip (elastic#100147) [FTR] Use importExport for saved_object/basic archive (elastic#100244) [Fleet] Better input for multi text input in agent policy builder (elastic#101020) [CI] Buildkite support with Baseline pipeline (elastic#100492) [Reporting/Telemetry] Do not send telemetry if we are in screenshot mode (elastic#100388) Create API keys with metadata (elastic#100682) ...

do not send telemetry if isScreenshotMode

f9cc783

jloleysens added zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels May 20, 2021

jloleysens requested review from Bamieh and tsullivan May 20, 2021 10:46

Bamieh reviewed May 20, 2021

View reviewed changes

afharo reviewed May 20, 2021

View reviewed changes

jloleysens added 5 commits May 20, 2021 14:10

Implement PR feedback:

0f86d84

* added another Jest test * move Boolean() to make the opt-in value always boolean

remove unused import and convert to import type

7e7f33b

fix type issues

2c31aa4

update jest snapshot

4f91bab

Expanded test coverage

afe7dbd

- added plugin functional test - added jest test to check TelemetrySender behaviour - exported the localStorage/window value that flags screenshot mode

kibanamachine and others added 2 commits May 25, 2021 05:05

Merge branch 'master' into reporting/prevent-sending-telemetry

1990c1f

fix test plugin name in package.json and make sure to opt out of tele…

fec95b6

…metry when the test finishes

added missing type file to plugin_functional test

3d5e140

Merge branch 'master' into reporting/prevent-sending-telemetry

e0a0b94

tsullivan approved these changes May 25, 2021

View reviewed changes

jloleysens marked this pull request as ready for review May 26, 2021 07:38

jloleysens requested a review from a team as a code owner May 26, 2021 07:38

Merge branch 'master' into reporting/prevent-sending-telemetry

dca59ac

Bamieh approved these changes May 31, 2021

View reviewed changes

Bamieh requested a review from afharo May 31, 2021 07:42

Merge branch 'master' into reporting/prevent-sending-telemetry

f00a895

ppisljar reviewed May 31, 2021

View reviewed changes

src/plugins/telemetry/public/plugin.ts Show resolved Hide resolved

Merge branch 'master' into reporting/prevent-sending-telemetry

ce7e4d2

jloleysens merged commit 0ecca23 into elastic:master Jun 1, 2021

jloleysens deleted the reporting/prevent-sending-telemetry branch June 1, 2021 17:03

jloleysens mentioned this pull request Jun 1, 2021

[7.x] [Reporting/Telemetry] Do not send telemetry if we are in screenshot mode (#100388) #101076

Merged

tsullivan mentioned this pull request Oct 20, 2021

[Telemetry] Stop sending telemetry and collecting metrics when reporting service is running kibana #93582

Closed

	import { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/public';
	import type { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/public';

Conversation

jloleysens commented May 20, 2021

Summary

To reviewers

Checklist

Uh oh!

Bamieh left a comment

Choose a reason for hiding this comment

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

afharo May 20, 2021

Choose a reason for hiding this comment

Uh oh!

afharo May 20, 2021

Choose a reason for hiding this comment

Uh oh!

afharo May 20, 2021

Choose a reason for hiding this comment

Uh oh!

afharo May 20, 2021

Choose a reason for hiding this comment

Uh oh!

jloleysens May 20, 2021

Choose a reason for hiding this comment

Uh oh!

jloleysens commented May 25, 2021

Uh oh!

jloleysens commented May 25, 2021

Uh oh!

jloleysens commented May 25, 2021

Uh oh!

tsullivan left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 26, 2021

Uh oh!

elasticmachine commented May 26, 2021

Uh oh!

jloleysens commented May 28, 2021

Uh oh!

Bamieh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bamieh commented May 31, 2021

Uh oh!

Uh oh!

jloleysens commented Jun 1, 2021

Uh oh!

kibanamachine commented Jun 1, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

Page load bundle

API count

References to deprecated APIs

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Bamieh left a comment •

edited

Loading