New analytics #4163

kneth · 2021-12-20T11:58:11Z

What, How & Why?

☑️ ToDos

📝 Changelog entry
~~[ ] 📝 Compatibility label is updated or copied from previous entry~~
🚦 Tests
~~[ ] 📱 Check the React Native/other sample apps work if necessary~~
~~[ ] 📝 Public documentation PR created or is not necessary~~
~~[ ] 💥 Breaking label has been applied or is not necessary~~

If this PR adds or changes public API's:

~~[ ] typescript definitions file is updated~~
~~[ ] jsdoc files updated~~
~~[ ] Chrome debug API is updated if API is available on React Native~~

kneth · 2022-01-13T16:41:01Z

Creating test package

To verify that the script is working as expected, I have modified package.json to:

"postinstall": "node scripts/submit-analytics.js --dryRun --log"

After creating a NPM package (as tgz), I have installed it in a plain node.js app and a React Native using the following command:

npm install --loglevel verbose -ddd --foreground-scripts ~/Projects/realm-js/realm-10.11.0.tgz

React Native

payload: {"webHook":{"event":"install","properties":{"token":"aab85907a13e1ff44a95be539d9942a9","JS Analytics Version":2,"distinct_id":"e6c2d8cd214bcb1925b36a120c4cfb7b069832fd78ba6b61dba48bde361d3e45","Anonymized Machine Identifier":"e6c2d8cd214bcb1925b36a120c4cfb7b069832fd78ba6b61dba48bde361d3e45","Anonymized Application ID":"e20852f3c853f54e4a8ce229ed53a93dfd12b0f7099cab55983db172de41c150","Binding":"javascript","Version":"0.0.1","Language":"javascript","Framework":"react-native","Framework Version":"0.66.4","JavaScript Engine":"hermes","Host OS Type":"darwin","Host OS Version":"20.6.0","Node.js version":"v16.13.0"}}}
Dry run; will not submit analytics

node.js

payload: {"webHook":{"event":"install","properties":{"token":"aab85907a13e1ff44a95be539d9942a9","JS Analytics Version":2,"distinct_id":"e6c2d8cd214bcb1925b36a120c4cfb7b069832fd78ba6b61dba48bde361d3e45","Anonymized Machine Identifier":"e6c2d8cd214bcb1925b36a120c4cfb7b069832fd78ba6b61dba48bde361d3e45","Anonymized Application ID":"c7f9a7ee0a2d96427a4e7d05d41d147b9eafa9d9a7f342de0976d605f00ccbc7","Binding":"javascript","Version":"1.0.0","Language":"javascript","Framework":"node.js","Framework Version":"v16.13.0","JavaScript Engine":"v8","Host OS Type":"darwin","Host OS Version":"20.6.0","Node.js version":"v16.13.0"}}}
Dry run; will not submit analytics

Electron

payload: {"webHook":{"event":"install","properties":{"token":"aab85907a13e1ff44a95be539d9942a9","JS Analytics Version":2,"distinct_id":"e6c2d8cd214bcb1925b36a120c4cfb7b069832fd78ba6b61dba48bde361d3e45","Anonymized Machine Identifier":"e6c2d8cd214bcb1925b36a120c4cfb7b069832fd78ba6b61dba48bde361d3e45","Anonymized Application ID":"c7f9a7ee0a2d96427a4e7d05d41d147b9eafa9d9a7f342de0976d605f00ccbc7","Binding":"javascript","Version":"1.0.0","Language":"javascript","Framework":"electron","Framework Version":"^16.0.7","JavaScript Engine":"v8","Host OS Type":"darwin","Host OS Version":"20.6.0","Node.js version":"v16.13.0"}}}
Dry run; will not submit analytics

tomduncalf

Looks good, just some minor comments

tomduncalf · 2022-01-24T10:04:43Z

scripts/submit-analytics.js

+class Webhook {
+  /**
+   * Path and credentials required to submit analytics through the webhook (production mode).
+   */
+  constructor() {
+    this.urlPrefix =
+      "https://webhooks.mongodb-realm.com/api/client/v2.0/app/realmsdkmetrics-zmhtm/service/metric_webhook/incoming_webhook/metric?ip=1&data=";
+  }
+
+  /**
+   * Constructs the full URL that will submit analytics to the webhook.
+   * @param  {Object} payload Information that will be submitted through the webhook.
+   * @returns {string} Complete analytics submission URL
+   */
+  buildRequest(payload) {
+    const request = this.urlPrefix + Buffer.from(JSON.stringify(payload.webHook), "utf8").toString("base64");
+    return request;
+  }
+}


I wonder if it might make sense to simplify this to just a constant + function rather than a class, as it doesn't do anything stateful. I don't have very strong feelings either way though so I'm happy for it to stay as it is equally!

Suggested change

class Webhook {

/**

* Path and credentials required to submit analytics through the webhook (production mode).

*/

constructor() {

this.urlPrefix =

"https://webhooks.mongodb-realm.com/api/client/v2.0/app/realmsdkmetrics-zmhtm/service/metric_webhook/incoming_webhook/metric?ip=1&data=";

}

/**

* Constructs the full URL that will submit analytics to the webhook.

* @param {Object} payload Information that will be submitted through the webhook.

* @returns {string} Complete analytics submission URL

*/

buildRequest(payload) {

const request = this.urlPrefix + Buffer.from(JSON.stringify(payload.webHook), "utf8").toString("base64");

return request;

}

}

/**

* Path and credentials required to submit analytics through the webhook (production mode).

*/

const ANALYTICS_BASE_URL = "https://webhooks.mongodb-realm.com/api/client/v2.0/app/realmsdkmetrics-zmhtm/service/metric_webhook/incoming_webhook/metric?ip=1&data=";

/**

* Constructs the full URL that will submit analytics to the webhook.

* @param {Object} payload Information that will be submitted through the webhook.

* @returns {string} Complete analytics submission URL

*/

const getAnalyticsRequestUrl = (payload) => ANALYTICS_BASE_URL + Buffer.from(JSON.stringify(payload.webHook), "utf8").toString("base64");

I was going to suggest something to the art of:

const ANALYTICS_BASE_URL = "https://webhooks.mongodb-realm.com/api/client/v2.0/app/realmsdkmetrics-zmhtm/service/metric_webhook/incoming_webhook/metric"; const requestUrl = `${ANALYTICS_BASE_URL}?ip=1&data=${getAnalyticsData()}`; This makes the request more maintainable in the future.

I like the idea of splitting it.

tomduncalf · 2022-01-24T10:12:24Z

scripts/submit-analytics.js

+    return;
+  }
+
+  const context = require("../../../package.json");


I wonder whether there are any strange case where this require could not work as expected (e.g. a user has changed their NODE_PATH to somewhere else, or some new npm replacement decides to store packages somewhere else...) and what the impact would be if it does?

Perhaps it is worth wrapping this block in try/catch just in case.

Maybe there's an alternative strategy (e.g. walk up the directory tree looking for package.json), but perhaps that is overkill and it's OK to fail silently

If we added this to @realm/react then this would be one directory deeper.

This could use the npm_config_local_prefix environment to get the path of the package we're getting installed into. We also have fs-extra as a dependency already, we might as well use readJsonSync here.

Also isn't it a biiiit excessive to send off the contents of the entire package.json? Isn't the dependencies object enough for what we want here?

I dob't think we are sending off the entire package.json unless I misread the payload?

realm-js/scripts/submit-analytics.js

Line 93 in fe84d68

const payloads = {

Ahh - my bad. Read it too fast.

I wonder if it is save to assume that we are interested in package.json in the same directory as node_modules. If so, we can use process.cwd() and chop off from node_modules and down.

The working directory is set by NPM to our package root, so that won't work.
I really think we use something like the npm_config_local_prefix environment variable.

scripts/submit-analytics.js

tomduncalf · 2022-01-24T10:52:51Z

scripts/submit-analytics.js

+    frameworkVersion = context.dependencies["react-native"];
+    try {
+      const podfile = fs.readFileSync("../../ios/Podfile", "utf8");
+      if (podfile.includes("hermes_enabled => true")) {


This feels a little brittle – maybe a regex match would be better to allow for formatting/syntax differences?

Suggested change

if (podfile.includes("hermes_enabled => true")) {

if (/hermes_enabled.*true/.test(podfile)) {

tomduncalf · 2022-01-24T10:57:23Z

scripts/submit-analytics.js

+    framework = "react-native";
+    frameworkVersion = context.dependencies["react-native"];
+    try {
+      const podfile = fs.readFileSync("../../ios/Podfile", "utf8");


I don't think there's much we can do about this, but just pointing out that this path is not necessarily fixed, e.g. in my old project we kept the Podfile somewhere else for reasons

Do we want to try to detect if they have Hermes enabled on Android too or is iOS enough?

I'd like to see the Hermes vs. Android/JSC statistics

I picked iOS since a Podfile is easier to parse 😄

tomduncalf · 2022-01-24T11:02:06Z

scripts/submit-analytics.js

+    frameworkVersion = context.dependencies["electron"];
+  }
+
+  const payloads = {


Suggested change

const payloads = {

const payload = {

tomduncalf · 2022-01-24T11:03:19Z

scripts/submit-analytics.js

+  const https = require("https");
+
+  return new Promise((resolve, reject) => {
+    const webhookRequest = new Webhook().buildRequest(payload);


If my suggestion at the top is useful, then

Suggested change

const webhookRequest = new Webhook().buildRequest(payload);

const webhookRequest = getAnalyticsRequestUrl(payload);

tomduncalf · 2022-01-24T11:04:28Z

scripts/submit-analytics.js

+  }
+
+  await Promise.all([
+    // send in analytics in the newer S3 format


Is this comment still useful as there's no mention of the older format?

Suggested change

// send in analytics in the newer S3 format

Indeed, it is mostly a comment for old-timers.

tomduncalf · 2022-01-24T11:05:40Z

scripts/submit-analytics.js

+  // eslint-disable-next-line no-unused-vars
+  doLog = (_) => {


Or could this just be

Suggested change

// eslint-disable-next-line no-unused-vars

doLog = (_) => {

doLog = () => {

tomduncalf · 2022-01-24T11:06:24Z

tests/js/index.js

@@ -66,7 +66,6 @@ if (global.enableSyncTests) {
    TESTS.SetSyncTests = node_require("./set-sync-tests");
    TESTS.DictionarySyncTests = node_require("./dictionary-sync-tests");
    TESTS.MixedSyncTests = node_require("./mixed-sync-tests");
-    TESTS.AnalyticsTests = require("./analytics-tests");


Do you think it's worth having some kind of integration test for the new way of doing analytics? Maybe one for a follow up in any case

See my comment above about the staging. If we don't have access to such a bucket, I don't think we can meaningfully do integration tests on this.

I think it could still be interesting to have, for example, a set of skeleton "test projects" with the various different Podfile/package.json permutations that we know of (RN Hermes iOS, RN Hermes Android, RN JSC iOS, RN JSC Android, Electron etc...), then run the script on them with a mocked https library and assert that (parts of) the payload match what we expect for each project.

I agree - having tests for this would be great.

At the moment, we will skip it all if CI is set - which Github Actions does.

takameyer

Looks good. Just a few suggestions and comments. I'm wondering if we can repurpose this to be used in any of our packages? Probably as a part of the re-packaging effort.

README.md

CHANGELOG.md

react-native/android/build.gradle

takameyer · 2022-01-24T12:54:44Z

scripts/submit-analytics.js

+class Webhook {
+  /**
+   * Path and credentials required to submit analytics through the webhook (production mode).
+   */
+  constructor() {
+    this.urlPrefix =
+      "https://webhooks.mongodb-realm.com/api/client/v2.0/app/realmsdkmetrics-zmhtm/service/metric_webhook/incoming_webhook/metric?ip=1&data=";
+  }
+
+  /**
+   * Constructs the full URL that will submit analytics to the webhook.
+   * @param  {Object} payload Information that will be submitted through the webhook.
+   * @returns {string} Complete analytics submission URL
+   */
+  buildRequest(payload) {
+    const request = this.urlPrefix + Buffer.from(JSON.stringify(payload.webHook), "utf8").toString("base64");
+    return request;
+  }
+}


I was going to suggest something to the art of:

const ANALYTICS_BASE_URL = "https://webhooks.mongodb-realm.com/api/client/v2.0/app/realmsdkmetrics-zmhtm/service/metric_webhook/incoming_webhook/metric"; const requestUrl = `${ANALYTICS_BASE_URL}?ip=1&data=${getAnalyticsData()}`; This makes the request more maintainable in the future.

takameyer · 2022-01-24T13:01:17Z

scripts/submit-analytics.js

+    return;
+  }
+
+  const context = require("../../../package.json");


If we added this to @realm/react then this would be one directory deeper.

fronck · 2022-01-26T10:15:05Z

scripts/submit-analytics.js

@@ -0,0 +1,201 @@
+////////////////////////////////////////////////////////////////////////////
+//
+// Copyright 2021 Realm Inc.


Suggested change

// Copyright 2021 Realm Inc.

// Copyright 2022 Realm Inc.

fronck · 2022-01-26T10:19:01Z

scripts/submit-analytics.js

+
+class Webhook {
+  /**
+   * Path and credentials required to submit analytics through the webhook (production mode).


Do we no longer have the option to submit to a staging bucket for testing?

I don't think so but I can check.

fronck

Please review comments.

If my concerns are wrong: LGTM :)

kraenhansen · 2022-01-31T18:37:11Z

scripts/submit-analytics.js

+}
+
+async function submitAnalytics(dryRun) {
+  const wd = process.cwd();


I still feel it's less brittle if we relied on process.env.npm_config_local_prefix instead.

kraenhansen · 2022-01-31T18:38:48Z

scripts/submit-analytics.js

+}
+
+const optionDefinitions = [
+  { name: "dryRun", type: Boolean, multiple: false, description: "If true, don't submit analytics" },


I'm pretty sure these objects take a default value, making your next couple of lines obsolete.

kraenhansen · 2022-01-31T18:43:52Z

scripts/submit-analytics.js

+(async function () {
+  await submitAnalytics(dryRun);
+})();


This might result on an unhandled promise rejection.

Suggested change

(async function () {

await submitAnalytics(dryRun);

})();

submitAnalytics(dryRun).catch(console.error);

kraenhansen · 2022-01-31T18:47:41Z

.github/workflows/pr-analytics.yml

+          cd integration-tests/tests
+          tmpfile=$(mktemp)
+          node ../../scripts/submit-analytics.js --test > $tmpfile
+          cat $tmpfile | grep ^payload | cut -f2- -d' ' | jq '.webHook.properties' | jq 'keys' | jq 'join(",")' | grep -c 'Anonymized Application ID,Anonymized Machine Identifier,Binding,Framework,Framework Version,Host OS Type,Host OS Version,JS Analytics Version,JavaScript Engine,Language,Node.js version,Version,distinct_id,token'


I'd like the analytics code to be imported from the JS (integration) tests and the fetchPlatformData called with different contexts and asserted accordingly. I feel this bash assertion is a bit hard to maintain and it doesn't answer anything about the actual values in the object.

I see your point and we can expand the testing.

The approach here is valuable as it is testing the script as such (including the package.json discovery parts).

including the package.json discovery parts.

But it doesn't test the values of the payload properties?

* Suggested changes to the submit-analytics script * Scaffolding Node.js specific analytics tests

takameyer · 2022-02-07T08:42:23Z

@kneth With this change, can we remove the analytics from react-native/ios/RealmReact ? Specifically RealmAnalytics.h and RealmAnalytics.mm

cla-bot bot added the cla: yes label Dec 20, 2021

kneth changed the title ~~Kneth/new analytics~~ New analytics Dec 20, 2021

kneth self-assigned this Dec 20, 2021

kneth force-pushed the kneth/new-analytics branch from de67270 to aeaab9b Compare December 20, 2021 15:12

kneth force-pushed the kneth/new-analytics branch 2 times, most recently from 295c009 to 5082a9d Compare January 13, 2022 16:02

kneth force-pushed the kneth/new-analytics branch from 5082a9d to efa4a2f Compare January 21, 2022 15:08

kneth marked this pull request as ready for review January 21, 2022 16:04

kneth requested review from kraenhansen, tomduncalf, fronck and takameyer January 21, 2022 16:06

tomduncalf approved these changes Jan 24, 2022

View reviewed changes

takameyer approved these changes Jan 24, 2022

View reviewed changes

fronck reviewed Jan 26, 2022

View reviewed changes

fronck approved these changes Jan 26, 2022

View reviewed changes

kneth force-pushed the kneth/new-analytics branch 4 times, most recently from d41cd23 to 90464f3 Compare January 27, 2022 17:30

kraenhansen reviewed Jan 31, 2022

View reviewed changes

kneth added 4 commits February 2, 2022 15:00

changes to android build files

8c2ca3a

Initial version of postinstall script for node.js/electron

3353c04

wip

dcb30d7

No more analytics code in Android

6d49410

kneth added 6 commits February 2, 2022 15:01

Try to test analytics code

6d16e09

wip

787062a

GHA debugging

84a56cf

Move CI check a bit

6d8beac

Clean up

48676c4

PR feedback

4f696dc

kneth force-pushed the kneth/new-analytics branch from 951da7b to 4f696dc Compare February 2, 2022 15:38

kneth added 4 commits February 3, 2022 13:46

refactoring

c98e4ed

wip

d9c8516

fix typo

9a0f9f8

Fix lint error

fafe4fa

kraenhansen force-pushed the kneth/new-analytics branch from 3cd5145 to fafe4fa Compare February 4, 2022 13:13

kraenhansen and others added 3 commits February 4, 2022 14:52

New analytics extended (#4312)

929c758

* Suggested changes to the submit-analytics script * Scaffolding Node.js specific analytics tests

Add testing to integration-tests

fba05a2

PR fixes

d084a6e

kneth added 10 commits February 7, 2022 13:39

Fix tests

90ee8b5

Fix reading files

28ee2dd

Remove iOS analytics code

d398d75

wip

541a266

wip

32a6f2f

debug test

02253e7

debug test

d20ff59

Resolve path of test files

4b55684

Resolve path of root package.json

34d2799

Fix test

0ba677f

kneth merged commit e6295a9 into master Feb 8, 2022

kneth deleted the kneth/new-analytics branch February 8, 2022 15:57

kraenhansen mentioned this pull request Feb 17, 2022

Fix bug in submit analytics script when running "npm install" in the realm-js repository #4355

Closed

9 tasks

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

	if (podfile.includes("hermes_enabled => true")) {
	if (/hermes_enabled.*true/.test(podfile)) {

	const webhookRequest = new Webhook().buildRequest(payload);
	const webhookRequest = getAnalyticsRequestUrl(payload);

	// eslint-disable-next-line no-unused-vars
	doLog = (_) => {
	doLog = () => {

New analytics #4163

New analytics #4163

Conversation

kneth commented Dec 20, 2021 • edited Loading

What, How & Why?

☑️ ToDos

kneth commented Jan 13, 2022 • edited Loading

Creating test package

React Native

node.js

Electron

tomduncalf left a 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

kraenhansen Jan 26, 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

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

takameyer left a 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

fronck left a comment

Choose a reason for hiding this comment

kraenhansen Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

kneth Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takameyer commented Feb 7, 2022 • edited Loading

kneth commented Dec 20, 2021 •

edited

Loading

kneth commented Jan 13, 2022 •

edited

Loading

kraenhansen Jan 26, 2022 •

edited

Loading

kraenhansen Jan 31, 2022 •

edited

Loading

kraenhansen Jan 31, 2022 •

edited

Loading

kneth Feb 2, 2022 •

edited

Loading

takameyer commented Feb 7, 2022 •

edited

Loading