WIP: Refreshcasestats using Functions/Firestore #1988

matthewblain · 2021-05-05T16:49:11Z

This rewrites the refreshCaseStats functionality in Typescript on the Google Cloud Functions platform.

Current status is that it is functional: It can load the casestats from the WHO/ArcGIS endpoint and store them in the Datastore/Firestore. This functionality should be exactly the same as the old system. It has a getCaseStats endpoint which should return the data in the equivalent format as the old system.

What is missing?

Unit tests.
Automated 'cron' type functionality. This currently only runs on an (unsecured) http request endpoint.
Possibly changing how the data is stored in Firestore from a single entity (doc) per country to a collection per country. This would be to make syncing easier on the client, in case firestore sync works better that way.
Also, the code is totally non-idomatic Typescript, as this is the first time I've used TS.

#1972

Screenshots

N/A

How did you test the change?

Well, curl to the google cloud functions/firestore emulator.

Checklist:

Followed the Contributor Guidelines and verified that all contributions are properly licensed pursuant to the LICENSE.

…s way better.

rjhuijsman · 2021-05-11T04:53:56Z

server/functions/package.json

-    "test": "firebase --project=$GCLOUD_PROJECT emulators:exec \"mocha -r ts-node/register src/**/*.spec.ts\"",
+    "test_rules": "firebase --project=mblain-13nov2020 emulators:exec \"mocha -r ts-node/register src/**/*.spec.ts\"",
+    "test": "mocha -r ts-node/register --reporter spec",
    "check-location": "./check_location.sh",
-    "serve": "npm run build && firebase emulators:start",
-    "shell": "npm run build && firebase functions:shell",
+    "preserve": "tsc",
+    "serve": "firebase emulators:start --project=mblain-13nov2020",
+    "shell": "npm run build np&& firebase functions:shell",


Was there a usability issue with the tests that you're working around here? I'm concerned that in this config just running npm test will not work unless you knew to previously start the emulators, but if the tests were hard for you to use prior to this change we should address that!

Also, we'll want to get rid of hardcoded project ID's before submitting this, obviously. :)

The current system assumes my shell is bash and some other things. My version is worse in that it hardcodes some stuff. I think there's some package.json way to do the dependecies (build -> serve). Interestingly now if I do npm run serve it does get it right. Test, not sure.

rjhuijsman · 2021-05-11T04:57:06Z

server/functions/src/index.ts

  isoCountryCode: string;
  subscribedTopics: string[];
 }

+refreshcasestats.setDb(db); /// This seems wrong???????????????????????????????????????????????????????????


It's fine to run...

admin.initializeApp(); const db = admin.firestore();

... in every file where we need database access. Alternatively, we can create a db.ts which has those two lines and an export db that we can then import in all the other files.

This does not seem to be correct. If I call it in both files, I get an error like this:

! Error: The default Firebase app already exists. This means you called initializeApp() more than once without providing an app name as the second argument. In most cases you only need to call initializeApp() once. But if you do want to initialize multiple apps, pass a second argument to initializeApp() to give each app a unique name.

Oh shoot, that unfortunately makes a lot of sense.

In that case, I'd go for a separate db.ts module that we import from everywhere.

Creating a new module seems to work. Not sure if I did it correctly.

rjhuijsman · 2021-05-11T05:00:58Z

server/functions/src/index.ts

 import { SERVING_REGION } from "./config";
+import * as refreshcasestats from "./refreshcasestats"; // Is this right?


This is fine! You might also consider importing only specific symbols (like we do from ./config above), notably refreshCaseStats, so that the export (further down) can just be export refreshCaseStats; - but only once we can remove the setDb() call, since that would look very strange without the context of refreshcasestats.

rjhuijsman · 2021-05-11T05:04:18Z

server/functions/src/index.ts

+      jurisdiction.code
+    }`;
+    console.log(`Loading data for ${key}`);
+    let loadedData = await db.collection("StoredCaseStats").doc(key).get();


We'll probably want to make "StoredCaseStats" a constant loaded from refreshcasestats.ts?

That's the database schema question! What are the entity kind names, and where is that documented in the code? Is it shared 'client' and 'server'? For now, I added a const.

rjhuijsman · 2021-05-11T05:07:27Z

server/functions/src/index.ts

-    const data = JSON.parse(fs.readFileSync("casestats.json", "utf8"));
-    response.status(200).json(data);
+    getCaseStatsAsync(request.body)
+      .then(function (resp) {


It's generally preferred to use the now-more-common shorthand for an inline function definition:

(params) => { body }

There's a few examples throughout this file.

rjhuijsman · 2021-05-11T05:28:16Z

server/functions/src/refreshcasestats.ts

+// Description of the JSON data returned by the ArcGIS Server
+interface Attributes {
+  ISO_2_CODE: string;
+  date_epicrv: number;
+  NewCase: number;
+  CumCase: number;
+  NewDeath: number;
+  CumDeath: number;
+  ADM0_NAME: string;
+}
+
+interface Feature {
+  attributes: Attributes;
+}
+
+// The ARCGISResponse contains more than this, but all we need are the features.
+interface ArcGISResponse {
+  features: Feature[];
+}


I like that we have this here, very nice code-as-documentation.

rjhuijsman · 2021-05-11T05:31:18Z

server/functions/src/refreshcasestats.ts

+  .https.onRequest((request, response) => {
+    refreshCaseStatsAsync()
+      .then(function (ok) {
+        if (!ok) {
+          response.status(500).send("Error");
+          return;
+        }
+        response.status(200).send("OK");
+      })
+      .catch(function (error) {
+        console.log(error);
+        response.status(500).send("Error"); // How to get the system to backoff/retry? What text to use?
+        return;
+      });


You can use async/await here too (async (request, response) => { ... }) if you'd like.

rjhuijsman · 2021-05-11T05:33:54Z

server/functions/src/refreshcasestats.ts

+// TODO: Scheduled functions require billing (lol). It's only $0.10/month but
+// but that's absurd for a tiny project...


It's not even the functions that require billing... It's the scheduler that fires off the Pub/Sub message that triggers the function that needs it... Le sigh. 😕

FWIW, even on billing-enabled projects the free tier is in effect, so we'll likely still be charged $0.

rjhuijsman · 2021-05-11T05:36:02Z

server/functions/src/who.ts

+// This was generated from who.proto with https://github.com/stephenh/ts-proto
+// Then all of the encoding methods were removed as they interfered with
+// Firestore and are not necessary.
+// This leaves only the interface definitions.


I like this a lot! I in particular like how the use of these interfaces produces very nice self-documenting code in the other .ts files.

However it needs better tooling to keep in sync w/proto changes. Probably just some flags to ts-proto but I have not investigated.

rjhuijsman · 2021-05-11T05:42:21Z

server/functions/src/index.ts

+  let resp = {
+    globalStats: undefined,
+    jurisdictionStats: [],
+    ttl: 0,
+  } as who.GetCaseStatsResponse;


Consider this slight change:

let resp: who.GetCaseStatsResponse = { globalStats: undefined, jurisdictionStats: [], ttl: 0, }

... Since the as type assertion will not throw a type error if you're missing required fields (I'm basing that on e.g. this), but when constructing an object of a given type you will be warned about that.

thanks! i didn't know how to do 'constructors' but copied another example in this file.

stale · 2021-05-18T09:58:18Z

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

matthewblain · 2021-05-26T17:14:37Z

See comments.
Note: I have not tried to actually deploy!
Note2: If the request to getcasestats is not valid json, the function returns immediately... but (at least the emulator) keeps the function running in the background for 60s then kills it. Strange, IIRC it's the opposite of app engine. (IIRC, on return, appengine kills the request? Or vice versa, the request doesn't return until it's done?). Anyway I have no idea why this is, or if it matters. I saw the same behavior when I had some bugs in refreshcasestats (I fired off but did not wait for some async requests), but in that case at least it sort of makes sense.
Note3: Still not unit tests.

rjhuijsman · 2021-06-02T10:47:35Z

See comments.

Note: I have not tried to actually deploy!

If the emulators work the deploy will most likely work too, but we'll of course have to verify before we submit.

Note2: If the request to getcasestats is not valid json, the function returns immediately... but (at least the emulator) keeps the function running in the background for 60s then kills it. Strange, IIRC it's the opposite of app engine. (IIRC, on return, appengine kills the request? Or vice versa, the request doesn't return until it's done?). Anyway I have no idea why this is, or if it matters. I saw the same behavior when I had some bugs in refreshcasestats (I fired off but did not wait for some async requests), but in that case at least it sort of makes sense.

Interesting... Do you get the error 400 from getCaseStats, as one might expect?

The behavior you observe would be consistent with my expectations if the code failed to send an HTTP response before returning - but if there is a response being sent (as I believe the catch in getCaseStats should ensure) then I believe you're likely seeing emulator behavior that's not consistent with production.

Note3: Still not unit tests.

rjhuijsman

No major code issues to my eyes!

To me, the remaining work looks like:

Factor out the firebase.initializeApp(); export db = firebase.firestore(); into a db.ts that can get imported by the other files.
Fix up the package.json so that it's easily usable (ideally no hardcoded project IDs, if that's not feasible then make the hardcoded values something like YOUR-PROJECT-ID-HERE and update README.md with instructions).
Unit tests where useful.
Remove the HTTP function and replace with the scheduled async function (note that scheduled functions can be triggered manually too by clicking on the Cloud Console's Cloud Scheduler page).
Deploy to a real project and verify there.

How does that sound?

server/functions/src/refreshcasestats.ts

rjhuijsman · 2021-06-02T10:58:42Z

server/functions/src/refreshcasestats.ts

+    if (lastSnapshot.dailyCases == 0 && lastSnapshot.dailyDeaths == 0) {
+      if (priorSnapshot.dailyCases > 0 || priorSnapshot.dailyDeaths > 0) {
+        // Likely "No Data Reported"
+        timeseries.pop();
+        countryStat.lastUpdated = priorSnapshot.epochMsec;
+        countriesLastDayRemoved.push(countryName);
+      }
+    }


Given that these ifs don't have elses, consider using if (opposite) continue here too, to improve readability by reducing indentation.

I think having the exceptional case in an 'if' is more understandable.

stale · 2021-06-09T11:19:44Z

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2021-06-16T19:43:51Z

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

matthewblain added 7 commits April 14, 2021 09:31

Refreshcasestats early draft

8a76240

Move refreshcasestats to its own file

489e83d

Casestats can now iterate over paginated requests. Also handles error…

3950d41

…s way better.

Refreshcasestats now saves case stats

c6310d7

GetCaseStats working. Also moved to .proto (sort of).

65ca4cc

add totalCasesDeltaCheck

76f513c

Rename casestats.ts to refreshcasestats.ts

0ee47a9

matthewblain requested a review from rjhuijsman May 5, 2021 16:49

matthewblain requested a review from brunobowden as a code owner May 5, 2021 16:49

rjhuijsman reviewed May 11, 2021

View reviewed changes

stale bot added the resolved:stale No recent activity on the issue or PR label May 18, 2021

stale bot removed the resolved:stale No recent activity on the issue or PR label May 26, 2021

matthewblain added 2 commits May 26, 2021 08:41

Review updates

e621633

Asyncify

1e6ce7f

rjhuijsman suggested changes Jun 2, 2021

View reviewed changes

stale bot added resolved:stale No recent activity on the issue or PR and removed resolved:stale No recent activity on the issue or PR labels Jun 9, 2021

stale bot added the resolved:stale No recent activity on the issue or PR label Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Refreshcasestats using Functions/Firestore #1988

WIP: Refreshcasestats using Functions/Firestore #1988

matthewblain commented May 5, 2021 •

edited

Loading

rjhuijsman May 11, 2021

matthewblain May 26, 2021

rjhuijsman May 11, 2021

matthewblain May 26, 2021

rjhuijsman Jun 2, 2021

matthewblain Jun 9, 2021

rjhuijsman May 11, 2021

rjhuijsman May 11, 2021

matthewblain May 26, 2021

rjhuijsman May 11, 2021

matthewblain May 26, 2021

rjhuijsman May 11, 2021

rjhuijsman May 11, 2021

matthewblain May 26, 2021

rjhuijsman May 11, 2021

rjhuijsman May 11, 2021

matthewblain May 26, 2021

rjhuijsman May 11, 2021

matthewblain May 26, 2021

stale bot commented May 18, 2021

matthewblain commented May 26, 2021

rjhuijsman commented Jun 2, 2021

rjhuijsman left a comment

rjhuijsman Jun 2, 2021

matthewblain Jun 9, 2021

stale bot commented Jun 9, 2021

stale bot commented Jun 16, 2021

		import { SERVING_REGION } from "./config";
		import * as refreshcasestats from "./refreshcasestats"; // Is this right?

		// TODO: Scheduled functions require billing (lol). It's only $0.10/month but
		// but that's absurd for a tiny project...

WIP: Refreshcasestats using Functions/Firestore #1988

Are you sure you want to change the base?

WIP: Refreshcasestats using Functions/Firestore #1988

Conversation

matthewblain commented May 5, 2021 • edited Loading

Screenshots

How did you test the change?

Checklist:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented May 18, 2021

matthewblain commented May 26, 2021

rjhuijsman commented Jun 2, 2021

rjhuijsman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 9, 2021

stale bot commented Jun 16, 2021

matthewblain commented May 5, 2021 •

edited

Loading