Enables running transitions synchronously #5476

dianabarsan · 2019-03-06T16:54:17Z

Description

Pulls sentinel transitions out into a shared library
Enables usage of new shared library @medic/transitions in Sentinel and API.
API sms and records endpoints now run transitions over docs before saving.

#4857

Review checklist

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on medic-docs
Tested: Unit and/or e2e where appropriate
Internationalised: All user facing text
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

shared-libs/transitions/src/schedule/due_tasks.js

shared-libs/transitions/src/transitions/index.js

dianabarsan · 2019-03-06T16:57:54Z

shared-libs/transitions/src/transitions/index.js

+    );
+    return callback();
+  }
+


I moved the call to canRun from the applyTransitions map function.
The reason was to test canRun over the document after it has been updated by previous transitions.

As an example, it enables the possibility of running registration over a fresh doc: currently registration requires either the form to be public or doc.contact to exist, while update_clinics is responsible for creating doc.contact.

This is a good idea.

We should also formalise transition run order to make this less fragile.

We already formalize that, since 2.15
https://github.com/medic/medic/blame/d7ec779eac1806a26cd6f546970b9319648a3cc7/shared-libs/transitions/src/transitions/index.js#L13

dianabarsan · 2019-03-06T17:26:45Z

shared-libs/transitions/src/transitions/index.js

          });
        });
+        async.series(operations, (err, results) => {
+          return err ? reject(err) : resolve(results);
+        });


Just thought about this.. do you guys think I should delete the just-created info-docs for docs that are just echo-ed back?

I think this is fine for now - deleting a doc is probably worse for performance than just leaving it there forever...

I think eventually we want the transitions function to return an array of docs to save, so all the docs are saved once all transitions have executed, but that would need careful consideration...

I would very much agree with the transaction-like approach.

I guess I don't understand this part of the codebase anymore: why would you want to delete them? Even if the transitions don't touch the document we still want an infodoc for replication dates and stuff right?

If the transition returns "not changed" or has an error, I return the untouched version of the document that I received, which will not even have an _id property.
The reason is that some transitions add errors on the docs, but still return as "not changed".
The doc will receive another _id upon save, so the info-doc we just created is useless.
I guess I could only copy the _id property over in this case, so it wouldn't be useless?

Might as well right? The upside I presume being that when these docs are written Sentinel won't re-run those transactions over them. And the main downside is that we don't generate "smart" UUIDs that CouchDB likes, but honestly we don't do that for 90% of our data right now anyway (cause of PouchDB), and if we really wanted to we could replace the uuid generation used here with a cache that sources UUIDs from CouchDB if we considered it an issue.

Sentinel would run all transitions over that doc either way.

That's a shame. Maybe not in the future though (like I feel like we should be able to take advantage of this even if we aren't now)!

Also, it's probably better for Impact to not have orphaned info docs. I may not matter depending on their impl, but it's a bug waiting to happen in any case.

This thread is outdated because docs are saved in series within the library immediately after running transitions, same as old sentinel.

dianabarsan · 2019-03-06T17:33:30Z

I guess this is it.. new and improved along with as much git history.
The branch itself now uses the shared lib published under another name.

Could you please have a look, @garethbowen & @SCdF . Thank you!

garethbowen

Wow. Awesome!

A few questions and comments about minor details but the general approach is great.

shared-libs/transitions/src/config.js

shared-libs/transitions/src/db.js

shared-libs/transitions/src/lib/logger.js

shared-libs/transitions/src/schedule/due_tasks.js

garethbowen · 2019-03-07T01:17:45Z

shared-libs/transitions/src/transitions/index.js

          });
        });
+        async.series(operations, (err, results) => {
+          return err ? reject(err) : resolve(results);
+        });


I think this is fine for now - deleting a doc is probably worse for performance than just leaving it there forever...

I think eventually we want the transitions function to return an array of docs to save, so all the docs are saved once all transitions have executed, but that would need careful consideration...

shared-libs/transitions/src/transitions/index.js

garethbowen · 2019-03-07T01:27:28Z

tests/e2e/registration-by-sms.js

@@ -262,11 +262,11 @@ describe('registration transition', () => {
    const checkAutoResponse = () => {
      const taskElement = element(by.css('#reports-content .details > ul'));
      expect(taskElement.element(by.css('.task-list > li:nth-child(1) > ul > li')).getText()).toBe('Thank you '+ CAROL.name +' for registering Siobhan');
-      expect(taskElement.element(by.css('.task-list > li:nth-child(1) .task-state .state.pending')).isDisplayed()).toBeTruthy();
+      expect(taskElement.element(by.css('.task-list > li:nth-child(1) .task-state .state.forwarded-to-gateway')).isDisplayed()).toBeTruthy();


garethbowen · 2019-03-07T01:27:53Z

tests/e2e/sentinel/transitions/generate_patient_id_on_people.spec.js

@@ -2,7 +2,7 @@ const utils = require('../../../utils'),
      sentinelUtils = require('../utils'),
      uuid = require('uuid');

-describe('death_reporting', () => {
+describe('generate_patient_id_on_people', () => {


SCdF

Mammoth!

Various comments, but the general gist is looking awesome! I'm excited for this distinction to open up opportunities to improve Sentinel's performance as well as our SMS response times

SCdF · 2019-03-07T15:24:12Z

sentinel/src/transitions.js

+    }
+
+    if (!changed) {
+      return updateMetadata(change, callback);


Not your code I imagine, but it's weird that we call declare a call to updateMetadata twice even though we do it regardless (either we do it and return, or we do something else then do it).

SCdF · 2019-03-07T15:37:55Z

shared-libs/transitions/src/transitions/index.js

-            return;
-          }
+  let changes;
+  const clonedDocs = JSON.parse(JSON.stringify(docs)),


I haven't kept up and I'm cool if it is, but is this the state of the art for JSON cloning these days, perf-wise?

Yeah every time I check this is what it comes back to. Last time I checked was a couple of months ago.

I guess it depends on what you're cloning.
As a result of Stefan's comment, I tried a couple of reliable alternatives : http://jsben.ch/SJghM

lodash cloneDeep

angular copy

Do you guys think lodash.cloneDeep would be worth including? It can be required as a standalone.

In my Chromium, JSON is faster than lodash but angular is way faster. Do you get lodash as fastest?

slightly but consistently faster

Mine's consistently slower. Strange. Different browser versions maybe?

I wonder how angular copy is doing it?

I don't think it's worth switching to lodash but it may be worth ripping out the angular version and using that throughout in a separate PR.

I was planning on looking at the Angular version tomorrow, it got me curious.

The lodash seems to be the complete solution to deep copy and tries to treat all possible cases, while angular's solution is clearly stated to be riddled with caveats, albeit faster.

SCdF · 2019-03-07T15:42:38Z

shared-libs/transitions/src/transitions/index.js

-          }
+  let changes;
+  const clonedDocs = JSON.parse(JSON.stringify(docs)),
+        idsByIdx = [];


Might as well

Suggested change

idsByIdx = [];

idsByIdx = new Array(clonedDocs.length);

SCdF · 2019-03-07T15:54:43Z

shared-libs/transitions/src/transitions/index.js

+// - hydrate the docs and generate info docs
+// - run loaded transitions over all docs
+// - returns docs, either updated or their original version
+const processDocs = docs => {


Can processDocs wrap and so re-use processChange?

I guess we could, but then everything would be done one doc at a time:

fetching lineage

creating the infodoc

Perf wise, it would be much worse than bulking them.

SCdF · 2019-03-07T16:08:43Z

shared-libs/transitions/src/transitions/index.js

          });
        });
+        async.series(operations, (err, results) => {
+          return err ? reject(err) : resolve(results);
+        });


I guess I don't understand this part of the codebase anymore: why would you want to delete them? Even if the transitions don't touch the document we still want an infodoc for replication dates and stuff right?

shared-libs/transitions/src/transitions/index.js

SCdF · 2019-03-07T17:00:50Z

shared-libs/transitions/src/transitions/index.js

+    );
+    return callback();
+  }
+


This is a good idea.

We should also formalise transition run order to make this less fragile.

SCdF · 2019-03-07T17:04:37Z

shared-libs/transitions/src/schedule/due_tasks.js

@@ -1,7 +1,6 @@
 // TODO: this doesn't need to exist. We can just work this out dynamically when
 //       gateway queries, it's not slow or complicated.
 const async = require('async'),
-  _ = require('underscore'),


What's the reasoning behind moving the schedule code into the transitions shared lib? AFAICT it's only used by sentinel.

It's funny you should ask!
It's because someone wrote this two years ago:
https://github.com/medic/medic/blame/8c25c15e6fec268b272d5fb9b361bd119864a4e6/sentinel/src/schedule/due_tasks.js#L1

@dianabarsan Could you please raise an issue so this actually gets fixed? I'm happy to do it but you're the sentinel expert now!

garethbowen

Awesome.

I haven't gone through each line again but reviewed the new commits and it looks good to me. If you think it's ready for AT go for it!

Rolls back due_tasks fix Changes how logger and db are inited Changes "sync"/"async" transition definitions to be hardcoded Three transitions are only async: muting, update_notifications and mutli_report_alerts

+ adds some more unit tests.

This reverts commit ffe2c19

+ adds some more unit tests.

dianabarsan · 2019-05-02T08:45:45Z

I've rebased to resolve a conflict. Also published the shared-lib under the correct name and updated the dependencies in the apps. Had to do some minor updates to get a couple of unit tests passing.
All changes included in this commit

garethbowen

LGTM.

Pulls sentinel transitions out into a shared library Enables usage of new shared library @medic/transitions in Sentinel and API. API sms and records endpoints now run transitions over docs before saving. #4857

dianabarsan commented Mar 6, 2019

View reviewed changes

shared-libs/transitions/src/schedule/due_tasks.js Outdated Show resolved Hide resolved

dianabarsan commented Mar 6, 2019

View reviewed changes

shared-libs/transitions/src/transitions/index.js Outdated Show resolved Hide resolved

dianabarsan commented Mar 6, 2019

View reviewed changes

dianabarsan requested review from garethbowen and SCdF March 6, 2019 17:33

garethbowen requested changes Mar 7, 2019

View reviewed changes

SCdF reviewed Mar 7, 2019

View reviewed changes

dianabarsan requested a review from garethbowen March 8, 2019 08:38

garethbowen approved these changes Mar 10, 2019

View reviewed changes

dianabarsan added 12 commits May 2, 2019 09:49

One commit to rule them all...

e8390b5

Many things..

69e3d30

Rolls back due_tasks fix Changes how logger and db are inited Changes "sync"/"async" transition definitions to be hardcoded Three transitions are only async: muting, update_notifications and mutli_report_alerts

Updates to package-locks because npm kinda forgot?

42f7e44

wrong form....

e58a3d2

Switch to saving docs immediately after running transitions.

074ce3e

Change comment.

cbca0a5

Switches generate_patient_id_on_people to be async only

188adc2

+ adds some more unit tests.

Revert "Switches generate_patient_id_on_people to be async only"

e46076f

This reverts commit ffe2c19

Switches generate_patient_id_on_people to be async only

18bfc7d

+ adds some more unit tests.

spacing.

71c6c19

Add processing delay back (oops).

611c6d5

Update dependencies and fix couple of tests.

95e9099

dianabarsan force-pushed the 4857-detached-sentinel-bis branch from 839e9ce to 95e9099 Compare May 2, 2019 08:11

dianabarsan requested a review from garethbowen May 2, 2019 09:33

garethbowen approved these changes May 2, 2019

View reviewed changes

dianabarsan merged commit df888af into master May 2, 2019

dianabarsan deleted the 4857-detached-sentinel-bis branch May 12, 2020 21:11

Enables running transitions synchronously #5476

Enables running transitions synchronously #5476

Conversation

dianabarsan commented Mar 6, 2019 • edited Loading

Description

Review checklist

License

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

dianabarsan commented Mar 6, 2019

garethbowen 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

SCdF 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

dianabarsan Mar 7, 2019 • 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

garethbowen left a comment

Choose a reason for hiding this comment

dianabarsan commented May 2, 2019

garethbowen left a comment

Choose a reason for hiding this comment

dianabarsan commented Mar 6, 2019 •

edited

Loading

dianabarsan Mar 7, 2019 •

edited

Loading