Add triggered cloud functions for submitting Glean pings by aaga · Pull Request #333 · mozilla-rally/rally-web-platform

aaga · 2022-04-20T18:33:44Z

Closes #108

As part of this endeavor:

Add Glean metrics and pings yamls
Add Glean build process
Implement functions in index.ts that trigger when a user doc is updated or a user's study doc is updated
- these functions have logic to determine what info has actually changed, and calls the appropriate Glean ping function
Implement functions in glean.ts that construct and submit Glean pings
- uses a CustomPingUploader - eventually pings will be queued as POST requests in a Google Cloud Task queue instead of being submitted directly to Glean.
- there is mutex lock on the global Glean instance since Firebase Functions could be running multiple functions at the same time on the same instance, and that could lead to metrics getting overwritten before sending etc.
- there is also a semaphore signaling mechanism to ensure that the cloud function does not prematurely exit before the Glean ping has been sent. The Glean ping dispatcher is a background process, so we need to hook into the CustomPingUploader to know when the ping has actually been dispatched.

Some extra things that needed to happen along the way:

Port /functions directory to use ESM instead of CommonJS (since Glean is ESM-only)
- Add a patch to TS with a custom resolver, which prevents us having to add .js extensions to all our imports. (Node has experimental support for doing this with its own resolver, but adding that flag creates other issues with bin executables)
Port existing jest setup to use ESM
- Jest with ESM doesn't support mocking ESM modules so instead of mocking CORS I just disable it for testing.
- Jest doesn't yet support the package exports feature that Glean uses, so those imports have to be mapped manually for now. Support will land in Jest v28 so we can remove the manual mapping at that time.

.circleci/config.yml

functions/jest.config.js

aaga · 2022-04-20T18:39:33Z

functions/src/cors.ts

 export const useCors = cors({
-  origin: true,
+  // Don't use CORS in testing mode
+  origin: process.env.NODE_ENV !== "test" ? true : false,


(from my initial ticket comment)

Jest with ESM doesn't support mocking ESM modules so instead of mocking CORS I just disable it for testing.

You don't need explicit true/false and process.env.NODE_ENV !== "test" should get you that boolean.

functions/tsconfig.json

aaga · 2022-04-20T19:04:45Z

functions/src/glean.ts

+} from "@mozilla/glean/uploader";
+
+const GLEAN_DEBUG_VIEW_TAG = "MozillaRally";
+const GLEAN_RALLY_APP_ID = process.env.NODE_ENV !== "test" ? "rally-core" : "test-app-id";


@rhelmer this code prevents Glean pings from getting sent to the "real" rally-core app during npm run test, but I'm wondering if we should do something about the npm run dev case, i.e. when we're all doing general local testing? Is it ok for those pings to get sent to the main environment, or should we disable/redirect them? I'm not clear on how that all currently works in the Core Addon...

The core add-on just doesn't initialize Glean at all in dev mode:
https://github.com/mozilla-rally/rally-core-addon/blob/30ccc1ea49046fd188c06a8e884d846de0a0037c/core-addon/DataCollection.js#L39

Decided to disable Glean by default whenever the Firebase Emulator is running, with an option to explicitly enable it via an ENABLE_GLEAN environment variable. When the variable is set the pings are logged and sent to a test app id. When running in the "real" non-emulator environment, pings aren't logged and are sent to the "rally-core" app id. Added some info to the README. Lmk your thoughts.

.circleci/config.yml

functions/src/glean.ts

functions/src/index.ts

thomik-corp · 2022-04-20T22:46:29Z

functions/src/index.ts

+  if ((!oldUser || !oldUser.enrolled) && newUser.enrolled === true) {
+    // User just enrolled
+    functions.logger.info(`Sending enrollment ping for user ID ${userID}`);
+    // TODO send Glean ping


Should you return true here ?

@thomik-corp I wait on returning true in case the Firebase document change for enrollment and demographics update gets lumped together in the same change. This way we don't return prematurely and miss out on sending the Glean ping for the demographic change.

It's an unlikely scenario but I didn't see any harm in leaving it this way. Let me know if you feel otherwise.

functions/src/index.ts

functions/src/__tests__/authentication.test.ts

functions/tsconfig.json

functions/.eslintrc.cjs

functions/src/__tests__/authentication.test.ts

thomik-corp · 2022-04-20T23:06:43Z

functions/src/cors.ts

 export const useCors = cors({
-  origin: true,
+  // Don't use CORS in testing mode
+  origin: process.env.NODE_ENV !== "test" ? true : false,


You don't need explicit true/false and process.env.NODE_ENV !== "test" should get you that boolean.

rhelmer

This looks great! Nothing major, but I'd like to spend some time tomorrow testing it out and looking over it once more.

.circleci/config.yml

rhelmer · 2022-04-20T23:25:35Z

functions/src/glean.ts

+} from "@mozilla/glean/uploader";
+
+const GLEAN_DEBUG_VIEW_TAG = "MozillaRally";
+const GLEAN_RALLY_APP_ID = process.env.NODE_ENV !== "test" ? "rally-core" : "test-app-id";


The core add-on just doesn't initialize Glean at all in dev mode:
https://github.com/mozilla-rally/rally-core-addon/blob/30ccc1ea49046fd188c06a8e884d846de0a0037c/core-addon/DataCollection.js#L39

functions/src/glean.ts

rhelmer · 2022-04-20T23:35:18Z

functions/src/index.ts

-  })
+export const rallytoken = functions.https.onRequest(
+  async (request, response) =>
+    new Promise((resolve) =>


Hm, why does this need the additional promise wrapper?

@rhelmer This one is a little bit funny. The cors object is an old JavaScript function from Express that only uses a callback, not a promise. We need to be able to await properly on the rallytoken function for our tests. So I wrapped it in a promise and resolve it at the end of the callback.

You should also take in a reject parameter and invoke it from catch clause. We can also use async await from useCors and change its implementation around that but that's for later.

functions/src/glean.ts

package.json

functions/src/index.ts

…instead. Add basic Glean info to README

thomik-corp · 2022-04-26T19:55:52Z

functions/src/index.ts

-  })
+export const rallytoken = functions.https.onRequest(
+  async (request, response) =>
+    new Promise((resolve) =>


You should also take in a reject parameter and invoke it from catch clause. We can also use async await from useCors and change its implementation around that but that's for later.

functions/src/index.ts

rhelmer

lgtm, I noticed @thomik-corp left a few more comments, I am OK with those fixed. Thanks!

…ise from rallytoken implementation

* Add Glean metrics, pings, docs, and npm scripts * Add triggered function stubs * Add getRallyID function, fix some function naming * Port /functions from CommonJS to ESM * Fix existing Jest tests to use ESM * Implement Glean pings * Disable function triggers and Glean in integration test * Add APP_ID and VERSION to Glean * Add basic Glean info to README * Change order of extensionUser deletion * Change rallytoken tests to use callbacks instead of await