Add triggered cloud functions for submitting Glean pings #333
Conversation
functions/src/cors.ts
Outdated
|
||
export const useCors = cors({ | ||
origin: true, | ||
// Don't use CORS in testing mode | ||
origin: process.env.NODE_ENV !== "test" ? true : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need explicit true/false and process.env.NODE_ENV !== "test"
should get you that boolean.
functions/src/glean.ts
Outdated
} 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
functions/src/index.ts
Outdated
if ((!oldUser || !oldUser.enrolled) && newUser.enrolled === true) { | ||
// User just enrolled | ||
functions.logger.info(`Sending enrollment ping for user ID ${userID}`); | ||
// TODO send Glean ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you return true here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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/cors.ts
Outdated
|
||
export const useCors = cors({ | ||
origin: true, | ||
// Don't use CORS in testing mode | ||
origin: process.env.NODE_ENV !== "test" ? true : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need explicit true/false and process.env.NODE_ENV !== "test"
should get you that boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Nothing major, but I'd like to spend some time tomorrow testing it out and looking over it once more.
functions/src/glean.ts
Outdated
} 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/index.ts
Outdated
}) | ||
export const rallytoken = functions.https.onRequest( | ||
async (request, response) => | ||
new Promise((resolve) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why does this need the additional promise wrapper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…instead. Add basic Glean info to README
837b45a
to
f37a248
Compare
functions/src/index.ts
Outdated
}) | ||
export const rallytoken = functions.https.onRequest( | ||
async (request, response) => | ||
new Promise((resolve) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I noticed @thomik-corp left a few more comments, I am OK with those fixed. Thanks!
f16d13c
to
cc5489c
Compare
2e722da
to
cd9923c
Compare
…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
* 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
* 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
Closes #108
As part of this endeavor:
index.ts
that trigger when a user doc is updated or a user's study doc is updatedglean.ts
that construct and submit Glean pingsSome extra things that needed to happen along the way:
/functions
directory to use ESM instead of CommonJS (since Glean is ESM-only).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)