[EBT] Add Shipper "FullStory" #129927

afharo · 2022-04-11T17:57:25Z

Summary

It moves the FullStory implementation from the cloud plugin to the @elastic/analytics package.

It also changes the way the cloud plugin sets the context of the events. We'll move some of them to where they actually belong in #125690.

It also adds the logic in the telemetry/public to call the optIn API with the isOptedIn value.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

afharo · 2022-04-11T17:58:28Z

packages/elastic-analytics/src/events/types.ts

+  /**
+   * The current page.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  page?: string;
+  /**
+   * The current application ID.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  app_id?: string;
+  /**
+   * The current entity ID.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  ent_id?: string;


Should we define a good set of names for these fields and add a backwards-compatible layer in the FullStory shipper?

I'd say so, yes. shipper-specific mappings aren't an issue, but I'd put that in the shipper implementation.

Updated types to full names and shipper to maintain bwc

afharo · 2022-04-11T18:01:43Z

test/analytics/__fixtures__/plugins/analytics_plugin_a/server/plugin.ts

-        return res.ok({ body: stats.slice(-takeNumberOfCounters) });
+        return res.ok({
+          body: stats
+            .filter((counter) => counter.event_type === eventType)


I had to add this filter to this FTR plugin because new events may affect the tests otherwise.

afharo · 2022-04-11T18:48:40Z

x-pack/plugins/cloud/public/plugin.tsx

+        context$: from(loadUserId({ getCurrentUser: security.authc.getCurrentUser })).pipe(
+          filter((userId): userId is string => Boolean(userId)),
+          exhaustMap(async (userId) => {
+            const { sha256 } = await import('js-sha256');


Import the hashing library only if we got a userId.

We can use the browser native digest function here:
https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest

Though it only works on https which might be an OK tradeoff?

Alternatively, wouldn't t be better to send a request to a custom route on the kibana server to get this in a GET/POST request instead of loading an external library?

Good point! I created #130147 to look for alternatives. I'll keep it for now because we already had this dependency before.

pgayvallet

Overall LGTM. A few remarks and questions

packages/elastic-analytics/src/analytics_client/analytics_client.test.ts

packages/elastic-analytics/src/analytics_client/context_service.ts

pgayvallet · 2022-04-12T08:29:00Z

packages/elastic-analytics/src/events/types.ts

+  /**
+   * The current page.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  page?: string;
+  /**
+   * The current application ID.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  app_id?: string;
+  /**
+   * The current entity ID.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  ent_id?: string;


I'd say so, yes. shipper-specific mappings aren't an issue, but I'd put that in the shipper implementation.

pgayvallet · 2022-04-12T08:30:42Z

packages/elastic-analytics/src/events/types.ts

+   * The current page.
+   * @remarks We need to keep this for backwards compatibility because it was provided by previous implementations of FullStory.
+   */
+  page?: string;


Outside of the scope of this PR, but this makes me wonder: for the v3 telemetry shipper, will we have a specific field to dissociate events sent from the client and from the server?

That's a good question! I haven't thought about it. If we want to, we can define different channel names when shipping the data to V3. I'll ask the Platform Analytics team in our next meeting today.

packages/elastic-analytics/src/shippers/fullstory/format_payload.ts

pgayvallet · 2022-04-12T08:38:29Z

packages/elastic-analytics/src/shippers/fullstory/format_payload.ts

+}
+
+function isDate(value: unknown): value is Date {
+  return value instanceof Date || moment.isMoment(value);


This is a bit scary to know that we may have moment instances used for dates instead of plain Dates or timestamp. Do we really want to support that?

I can remove this. Technically, anything is possible. We are accepting unknown. And nothing is really stopping folks from setting a field to a moment instance. But let's remove it for now 👍

pgayvallet · 2022-04-12T08:39:18Z

packages/elastic-analytics/src/shippers/fullstory/fullstory_shipper.mocks.ts

+ * Side Public License, v 1.
+ */
+
+import type { FullStoryApi } from './types';


Naming convention: fullstory_shipper.test.mocks.ts

pgayvallet · 2022-04-12T08:58:11Z

packages/elastic-analytics/src/shippers/fullstory/get_parsed_version.ts

+  version_minor_int: number;
+  version_patch_int: number;
+} {
+  const [major, minor, patch] = version.split('.');


Optional: Could use Semver.parse

yes! I thought so: but I wanted to avoid the additional dependency. Bear in mind that this shipper runs on the UI.

pgayvallet · 2022-04-12T13:29:37Z

x-pack/plugins/cloud/public/plugin.tsx

+    }
+
+    // Keep this import async so that we do not load any FullStory code into the browser when it is disabled.
+    const { FullStoryShipper } = await import('@elastic/analytics');


Given we know that the @elastic/analytics will at least always be imported by core, should we move it to the shared libs?

I can create a follow-up PR to test what's the gain in bundle size 👍

#130027

pgayvallet · 2022-04-12T13:32:33Z

x-pack/plugins/cloud/public/plugin.tsx

+    const executionContext = await executionContextPromise;
+    analytics.registerContextProvider({
+      name: 'execution_context',
+      context$: executionContext.context$.pipe(
+        // Update the current context every time it changes
+        map(({ name, page, id }) => ({
+          pageName: `${compact([name, page]).join(':')}`,
+          app_id: name ?? 'unknown',


If it make sense to have the cloud plugin register the orgId/userId providers, I wonder if that's the best owner for the EC and version providers?

I totally agree! I just used this PR to "convert" the FS calls to the new analytics client's API. The plan is to use #125690 to move them where they actually belong :)

…ry-shipper

Bamieh · 2022-04-13T08:43:00Z

packages/elastic-analytics/BUILD.bazel

@@ -36,6 +36,7 @@ NPM_MODULE_EXTRA_FILES = [
 #    "@npm//name-of-package"
 #    eg. "@npm//lodash"
 RUNTIME_DEPS = [
+  "@npm//moment",


+1 for removing moment from the package and using plain old unix timestamps or Date objects

we need moment to validate the ISO string. We could replace it with a regex, but I worry we might leave out some formats depending on the TZ.

kibana-ci · 2022-04-13T09:40:25Z

💚 Build Succeeded

Test Failures

[job] [logs] Jest Tests #4 / useActiveCursor should trigger cursor pointer update (chart type: datatable - time based, event type: time)

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cloud`	19	32	+13
`core`	346	352	+6
total			+19

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
`@elastic/analytics`	11	23	+12

Async chunks

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

id	before	after	diff
`cloud`	15.1KB	26.5KB	+11.5KB
`core`	132.3KB	132.3KB	+29.0B
total			+11.5KB

Page load bundle

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

id	before	after	diff
`cloud`	8.8KB	10.0KB	+1.2KB
`core`	320.0KB	324.7KB	+4.7KB
`telemetry`	27.6KB	27.7KB	+150.0B
total			+6.1KB

Unknown metric groups

API count

id	before	after	diff
`@elastic/analytics`	82	106	+24
`core`	2499	2511	+12
total			+36

async chunk count

id	before	after	diff
`cloud`	2	3	+1

ESLint disabled in files

id	before	after	diff
`@elastic/analytics`	0	1	+1
`cloud`	2	1	-1
total			-0

ESLint disabled line counts

id	before	after	diff
`cloud`	7	6	-1

Total ESLint disabled count

id	before	after	diff
`@elastic/analytics`	16	17	+1
`cloud`	9	7	-2
total			-1

History

💛 Build #37723 was flaky 81f9fc8
💔 Build #37672 failed aeedc22
💚 Build #37649 succeeded 92d4f92
💚 Build #37415 succeeded 8b22fb7

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

pgayvallet

second pass - LGTM

afharo added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.3.0 labels Apr 11, 2022

afharo requested a review from a team as a code owner April 11, 2022 17:57

afharo force-pushed the ebt/add-fullstory-shipper branch from 62a51d3 to 41877f9 Compare April 11, 2022 18:00

[EBT] Add Shipper "FullStory"

9202cbe

afharo force-pushed the ebt/add-fullstory-shipper branch 2 times, most recently from 6637a0a to c411fe8 Compare April 11, 2022 18:44

Lazy import of js-sha256 to reduce page bundle size

76786bb

afharo force-pushed the ebt/add-fullstory-shipper branch from c411fe8 to 76786bb Compare April 11, 2022 18:45

afharo commented Apr 11, 2022

View reviewed changes

Disable the telemetry plugin for the FTR tests

8b22fb7

pgayvallet approved these changes Apr 12, 2022

View reviewed changes

afharo mentioned this pull request Apr 12, 2022

Add @kbn/analytics-client to the shared libs to test any bundle size improvements #130027

Closed

afharo added 5 commits April 12, 2022 18:01

Address PR feedback

92d4f92

Move backwards compatible fields to the shipper's implementation

aeedc22

Update tests

34288a7

Merge branch 'main' of github.com:elastic/kibana into ebt/add-fullsto…

22999ec

…ry-shipper

Replace .toPromise() with firstValueFrom

81f9fc8

Bamieh reviewed Apr 13, 2022

View reviewed changes

Add test use case for null

4d6229e

pgayvallet approved these changes Apr 13, 2022

View reviewed changes

afharo mentioned this pull request Apr 13, 2022

[EBT] userId hash: Find alternatives to js-sha256 #130147

Closed

afharo merged commit bed793a into elastic:main Apr 13, 2022

afharo deleted the ebt/add-fullstory-shipper branch April 13, 2022 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EBT] Add Shipper "FullStory" #129927

[EBT] Add Shipper "FullStory" #129927

afharo commented Apr 11, 2022

afharo Apr 11, 2022

pgayvallet Apr 12, 2022

afharo Apr 12, 2022

afharo Apr 11, 2022

afharo Apr 11, 2022

Bamieh Apr 13, 2022

afharo Apr 13, 2022

pgayvallet left a comment

pgayvallet Apr 12, 2022

pgayvallet Apr 12, 2022

afharo Apr 12, 2022 •

edited

Loading

pgayvallet Apr 12, 2022

afharo Apr 12, 2022

pgayvallet Apr 12, 2022

pgayvallet Apr 12, 2022

afharo Apr 12, 2022

pgayvallet Apr 12, 2022

afharo Apr 12, 2022 •

edited

Loading

pgayvallet Apr 12, 2022

afharo Apr 12, 2022

Bamieh Apr 13, 2022

afharo Apr 13, 2022

kibana-ci commented Apr 13, 2022 •

edited

Loading

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

pgayvallet left a comment

[EBT] Add Shipper "FullStory" #129927

[EBT] Add Shipper "FullStory" #129927

Conversation

afharo commented Apr 11, 2022

Summary

Checklist

For maintainers

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

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo Apr 12, 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

afharo Apr 12, 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

kibana-ci commented Apr 13, 2022 • edited Loading

💚 Build Succeeded

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet left a comment

Choose a reason for hiding this comment

afharo Apr 12, 2022 •

edited

Loading

afharo Apr 12, 2022 •

edited

Loading

kibana-ci commented Apr 13, 2022 •

edited

Loading