[Telemetry] Set `telemetry` SO as `hidden` #147631

afharo · 2022-12-15T16:43:33Z

Summary

Sets the telemetry SO as hidden and makes the necessary changes to accommodate that:

The telemetry plugin now instantiates the internal SO client and repositories to include the hidden type telemetry.
The browser now fetches the configuration via the new endpoint GET /api/telemetry/v2/config.
All the common/telemetry_config helpers have been moved to the server (except for one that is still relevant for the UI).
FTR tests can no-longer use the SO Global HTTP APIs to set the scenarios => They now write the document to .kibana.

Checklist

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

For maintainers

Adding @elastic/enterprise-search-frontend as reviewers so they can fill in if they are happy with using GET /api/telemetry/v2/config to fetch the opt-in status 😇

This was checked for breaking API changes and was labeled appropriately

afharo · 2022-12-15T16:48:45Z

src/plugins/telemetry/server/fetcher.ts

+    this.internalRepository = new SavedObjectsClient(
+      savedObjects.createInternalRepository([TELEMETRY_SAVED_OBJECT_TYPE])
+    );


nit: Potential improvement: Probably using the repository is good enough (no need to instantiate the client).

ATM, hidden types aren't exposed to the SO client unless we instantiate a new client exposing the hidden types with includedHiddenTypes (ref: #147150). So yes, we don't need to create a new client (unless I'm missing something)

afharo · 2022-12-15T16:50:23Z

src/plugins/telemetry/server/plugin.ts

@@ -111,9 +115,9 @@ export class TelemetryPlugin implements Plugin<TelemetryPluginSetup, TelemetryPl
   *
   * Using the internal client in all cases ensures the permissions to interact the document.
   */
-  private savedObjectsInternalClient$ = new ReplaySubject<SavedObjectsClient>(1);
+  private readonly savedObjectsInternalClient$ = new ReplaySubject<SavedObjectsClient>(1);


Similar to my previous comment: probably the repository is enough for our usage here.

Yea, we can follow up in a new PR to make all the changes needed for methods to accept SavedObjectsClientContract rather than SavedObjectsClient.

afharo · 2022-12-15T16:53:29Z

src/plugins/telemetry/server/routes/telemetry_config.ts

+      const body: FetchTelemetryConfigResponse = {
+        allowChangingOptInStatus,
+        optIn,
+        sendUsageFrom,
+        telemetryNotifyUserAboutOptInDefault,
+      };


Mostly porting the dynamic values of the deleted function updateConfigsBasedOnSavedObjects on the public side.

We may require follow-up PRs to validate the output of flags like telemetryNotifyUserAboutOptInDefault (refer to my comments in the FTRs)

afharo · 2022-12-15T16:55:12Z

src/plugins/telemetry/server/routes/telemetry_opt_in.ts

+      let telemetrySavedObject: TelemetrySavedObject | undefined;
+      try {
+        telemetrySavedObject = await getTelemetrySavedObject(soClient);
+      } catch (err) {
+        if (SavedObjectsErrorHelpers.isForbiddenError(err)) {
+          // If we couldn't get the saved object due to lack of permissions,
+          // we can assume the user won't be able to update it either
+          return res.forbidden();
+        }


I changed the getTelemetrySavedObject method to return the value or throw. The forbidden only applies to this API and was causing unexpected behavior in the other usages of the method.

afharo · 2022-12-15T17:04:03Z

src/plugins/telemetry/server/saved_objects/get_telemetry_saved_object.ts

    return attributes;
  } catch (error) {
    if (SavedObjectsErrorHelpers.isNotFoundError(error)) {
-      return null;


Removing this because we relied on null when there was not a SO (for Opt-In handling). But we store so many things now in the SO, that we may return something and still not have a clear opt-in property stored in the SO, so it's better to check the property itself.

I plan to take #135107 to fix this existing bug.

afharo · 2022-12-15T17:07:09Z

src/plugins/telemetry/server/telemetry_config/get_telemetry_opt_in.ts

@@ -42,6 +38,8 @@ export const getTelemetryOptIn: GetTelemetryOptIn = ({
  // if enabled is true, return it
  if (savedOptIn === true) return savedOptIn;

+  // TODO: Should we split the logic below into another OptIn getter?


I thought I already did enough refactoring to also tackle this... IMO, it could be handled as part of #135107

afharo · 2022-12-15T17:14:23Z

src/plugins/telemetry/server/saved_objects/get_telemetry_saved_object.test.ts

-    }
-
-    expect(threw).toBe(true);
+    await expect(callGetTelemetrySavedObject(params)).rejects.toThrow(SavedObjectOtherErrorMessage);


Same test but using the expect().rejects.toThrow APIs.

afharo · 2022-12-15T17:15:50Z

test/api_integration/apis/telemetry/telemetry_config.ts

+        // it's not opted-in (in the YAML config) despite being opted-in via API/UI, and we still say false??
+        telemetryNotifyUserAboutOptInDefault: false,


This should be fixed on #135107

afharo · 2022-12-15T17:15:55Z

test/api_integration/apis/telemetry/telemetry_config.ts

+        allowChangingOptInStatus: true,
+        optIn: false, // the config.js for this FTR sets it to `false`
+        sendUsageFrom: 'server',
+        telemetryNotifyUserAboutOptInDefault: false, // it's not opted-in, so we don't notify about opt-in??


This should be fixed on #135107

afharo · 2022-12-15T17:17:03Z

test/api_integration/apis/telemetry/telemetry_last_reported.ts

-      await supertest
-        .get('/api/telemetry/v2/last_reported')
-        .set('kbn-xsrf', 'xxx')
-        .expect(200, { lastReported: undefined });


For some reason, now, when reporting lastReported: undefined the serializer removed the field entirely... Functionally, it's the same for JS, so no biggie.

afharo · 2022-12-15T17:19:48Z

src/plugins/telemetry/server/routes/telemetry_opt_in_stats.ts

@@ -35,7 +35,7 @@ export async function sendTelemetryOptInStatus(
    channelName: 'optInStatus',
  });

-  const optInStatusPayload: UnencryptedTelemetryPayload =
+  const optInStatusPayload: UnencryptedTelemetryPayload | EncryptedTelemetryPayload =


It could also return EncryptedTelemetryPayload (actually, attending to the usage, it's the most likely output) 😇

elasticmachine · 2022-12-16T06:19:34Z

Pinging @elastic/kibana-core (Team:Core)

afharo · 2022-12-16T10:02:09Z

buildkite test this

TinaHeiligers

Seems like there's a bit of follow-up work needed to polish off the change over to an API layer
This PR does what is required for Enterprise Search. I'm still considering edge cases and haven't come across anything immediate yet.

TinaHeiligers · 2022-12-18T23:47:10Z

src/plugins/telemetry/server/fetcher.ts

+    this.internalRepository = new SavedObjectsClient(
+      savedObjects.createInternalRepository([TELEMETRY_SAVED_OBJECT_TYPE])
+    );


ATM, hidden types aren't exposed to the SO client unless we instantiate a new client exposing the hidden types with includedHiddenTypes (ref: #147150). So yes, we don't need to create a new client (unless I'm missing something)

TinaHeiligers

We'll need to give detailed instructions for testing. Telemetry's always been a little hard to test fully 😉

afharo · 2022-12-19T10:22:31Z

@elasticmachine merge upstream

carlosdelest

LGTM from Enterprise Search side. I have not validated the technical implementation though 👍

TinaHeiligers

LGTM, based on the follow up PR's needed to wrap the conversion up.

TinaHeiligers · 2022-12-20T15:04:41Z

src/plugins/telemetry/server/plugin.ts

@@ -111,9 +115,9 @@ export class TelemetryPlugin implements Plugin<TelemetryPluginSetup, TelemetryPl
   *
   * Using the internal client in all cases ensures the permissions to interact the document.
   */
-  private savedObjectsInternalClient$ = new ReplaySubject<SavedObjectsClient>(1);
+  private readonly savedObjectsInternalClient$ = new ReplaySubject<SavedObjectsClient>(1);


Yea, we can follow up in a new PR to make all the changes needed for methods to accept SavedObjectsClientContract rather than SavedObjectsClient.

afharo · 2022-12-20T15:13:48Z

@elasticmachine merge upstream

kibana-ci · 2022-12-20T15:39:12Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`telemetry`	44	32	-12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`telemetry`	13.8KB	4.9KB	-8.9KB

Page load bundle

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

id	before	after	diff
`telemetry`	22.8KB	21.7KB	-1.2KB

Unknown metric groups

async chunk count

id	before	after	diff
`telemetry`	4	3	-1

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	61	67	+6
`osquery`	109	115	+6
`securitySolution`	439	445	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	70	76	+6
`osquery`	110	117	+7
`securitySolution`	515	521	+6
total			+21

History

💚 Build #95626 succeeded b2e2b66
💚 Build #95315 succeeded 1acff4f
💚 Build #95248 succeeded 1acff4f
💔 Build #95217 failed 9e494521d47bb7f7e3468be302d7c8c7f14145ec
💔 Build #95198 failed 25509adf5f7655d370f4e251ae586a2b3b97b432

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

afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Dec 15, 2022

afharo force-pushed the telemetry/hide-telemetry-saved-objects branch 2 times, most recently from bbc8ce7 to a9ca182 Compare December 15, 2022 17:13

afharo commented Dec 15, 2022

View reviewed changes

afharo force-pushed the telemetry/hide-telemetry-saved-objects branch from a9ca182 to 4e07e7c Compare December 15, 2022 17:19

afharo commented Dec 15, 2022

View reviewed changes

afharo force-pushed the telemetry/hide-telemetry-saved-objects branch 2 times, most recently from 25509ad to 9e49452 Compare December 15, 2022 21:19

[Telemetry] Set telemetry SO as hidden

1acff4f

afharo force-pushed the telemetry/hide-telemetry-saved-objects branch from 9e49452 to 1acff4f Compare December 15, 2022 23:55

afharo marked this pull request as ready for review December 16, 2022 06:19

afharo requested a review from a team as a code owner December 16, 2022 06:19

afharo requested a review from a team December 16, 2022 09:51

afharo added the ci:cloud-deploy Create or update a Cloud deployment label Dec 16, 2022

TinaHeiligers reviewed Dec 18, 2022

View reviewed changes

TinaHeiligers requested review from a team and TinaHeiligers December 19, 2022 00:00

TinaHeiligers reviewed Dec 19, 2022

View reviewed changes

carlosdelest added ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment and removed ci:cloud-deploy Create or update a Cloud deployment labels Dec 19, 2022

afharo removed the ci:cloud-redeploy Always create a new Cloud deployment label Dec 19, 2022

Merge branch 'main' into telemetry/hide-telemetry-saved-objects

b2e2b66

afharo requested review from carlosdelest and removed request for a team December 19, 2022 12:18

carlosdelest approved these changes Dec 19, 2022

View reviewed changes

TinaHeiligers approved these changes Dec 20, 2022

View reviewed changes

Merge branch 'main' into telemetry/hide-telemetry-saved-objects

e88c8aa

afharo enabled auto-merge (squash) December 20, 2022 15:14

afharo mentioned this pull request Dec 20, 2022

[Telemetry] Use internal repository instead of SO Client #147864

Closed

afharo merged commit 30469a4 into elastic:main Dec 20, 2022

afharo deleted the telemetry/hide-telemetry-saved-objects branch December 20, 2022 16:13

kibanamachine added the v8.7.0 label Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Telemetry] Set `telemetry` SO as `hidden` #147631

[Telemetry] Set `telemetry` SO as `hidden` #147631

afharo commented Dec 15, 2022 •

edited

Loading

afharo Dec 15, 2022

TinaHeiligers Dec 18, 2022 •

edited

Loading

afharo Dec 15, 2022

TinaHeiligers Dec 20, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

afharo Dec 15, 2022

elasticmachine commented Dec 16, 2022

afharo commented Dec 16, 2022

TinaHeiligers left a comment

TinaHeiligers Dec 18, 2022 •

edited

Loading

TinaHeiligers left a comment

afharo commented Dec 19, 2022

carlosdelest left a comment

TinaHeiligers left a comment

TinaHeiligers Dec 20, 2022

afharo commented Dec 20, 2022

kibana-ci commented Dec 20, 2022 •

edited

Loading

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

		// it's not opted-in (in the YAML config) despite being opted-in via API/UI, and we still say false??
		telemetryNotifyUserAboutOptInDefault: false,

[Telemetry] Set telemetry SO as hidden #147631

[Telemetry] Set telemetry SO as hidden #147631

Conversation

afharo commented Dec 15, 2022 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

TinaHeiligers Dec 18, 2022 • edited Loading

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 Dec 16, 2022

afharo commented Dec 16, 2022

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

afharo commented Dec 19, 2022

carlosdelest left a comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo commented Dec 20, 2022

kibana-ci commented Dec 20, 2022 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

[Telemetry] Set `telemetry` SO as `hidden` #147631

[Telemetry] Set `telemetry` SO as `hidden` #147631

afharo commented Dec 15, 2022 •

edited

Loading

TinaHeiligers Dec 18, 2022 •

edited

Loading

TinaHeiligers Dec 18, 2022 •

edited

Loading

kibana-ci commented Dec 20, 2022 •

edited

Loading