Entries and treatments clean db admin tools

[jpcunningh]

No description provided.

[PieterGit]

@jpcunningh good work. LGTM. is it ready to test?

[jpcunningh]

This is ready for review and comment.

[PieterGit]

@DrVoles did you manage to test this PR? If you confirm that it works, I can merge it to dev.

[DrVoles]

I'm afraid I haven't yet had time (not managed to work out how to actually test it either...., I'm relatively new to all this)..

…

On Wed, 28 Nov 2018, 22:23 PieterGit, ***@***.***> wrote: @DrVoles <https://github.com/DrVoles> did you manage to test this PR? If you confirm that it works, I can merge it to dev. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4090 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aou2Y8_mjUjB-YsAOeb_h_GV2fOXeoG8ks5uzwzDgaJpZM4Ytiuh> .

[PieterGit]

@DrVoles ask for help at https://gitter.im/nightscout/public and we'll help you to test this PR

[sulkaharo]

API calls to delete data: please check the permissioning system is invoked correctly and that data loaded to the runtime state is also cleaned correctly if the deletion affects data that's currently loaded

[jpcunningh]

@sulkaharo, the permission system was invoked correctly and tested as it was. Please let me know if you see something incorrect.

I added a call to refresh the data, calling ctx.bus.emit('data-received'). I'm not 100% sure that was the correct method. Does that sound reasonable to you to ensure the local data is refreshed?

Thanks!

[sulkaharo]

@jpcunningh Looks like that might be enough for the server side but I think the client won't delete the treatment from the memory due to the way the data delta calculation system works, so we need to either create a new event that causes the client to do a full reload or implement semantics on how to update treartments to indicate they're deleted and not actually delete the entry as opposed to do something like set the insulin and carbs to 0 and add a deleted flag & send these entries to the system (maybe with the original entry contained inside the object).

Another use case that comes to mind that actually requires data to be updated rather than deleted is all artificial pancreas systems that sync pump data into Nightscout. Depending on how the receiving end of the treatment system works with data syncs, deleting data from Nightscout will either result in OpenAPS / AAPS etc to just sync the treatments back into Nightscout (if the entry didn't originate from Nightscout) OR Nightscout not showing the treatment entry originating from the pump, when the loop on the other end is still using the data. Both of these scenarios would be pretty problematic (either deeply confusing or possibly contribute to wrong insulin dosing).

Oh also - what's the correct semantics for xDrip, Spike to handle data having disappeared from Nightscout? Delete locally or re-upload? Another case where not deleting data but marking data as deleted would provide clear API behavior.

(So on short term, if people are having issues with running out of space in the free mLab tier, the correct way to fix that is to pay $8 / month to MongoDB Atlas and have way more capacity.)

[jpcunningh]

Thanks, @sulkaharo!

If we put a validation check in to prevent the user from deleting entries within 2 days of the current day, that would prevent the issues you noted from occurring without having to do anything elaborate.

Does that sound like a reasonable path to you?

Thanks!

[tynbendad]

i used the jpcunningh:entries-and-treatments-clean-db branch to test DELETE requests using curl of entries, treatments, and devicestatus and they all worked.

i did need to change package.json node version down to 8.11.x to use the branch on azure.

[PieterGit]

I agree with @sulkaharo that we should prevent possible display errors and possible even safety problems. On the other end I think many people don't want to spend a monthly fee for hosting their data. I store my data in an own mongo, but I see a lot of users that want to have a free hosting options.

So I think @jpcunningh solutions should work that we don't delete treatments younger than2 days (48 hours?). I think that will prevent possible errors with APS'es.

[PieterGit]

some of the new strings are missing in https://github.com/nightscout/cgm-remote-monitor/blob/dev/lib/language.js

[jpcunningh]

I put the validation check in to prevent deleting records newer than 2 days and reviewed all of the strings.

I also added a unit test for cleanstatusdb's old record delete function and will add unit tests for cleanentriesdb and cleantreatmentsdb tomorrow.

[jpcunningh]

@PieterGit and @sulkaharo, the request changes have been implemented.

Let me know if you see anything else.

Thanks!

[PieterGit]

Just did a quick review of the changes and look good to me. Next up is @sulkaharo approval and we need to find a user willing to test this PR and confirm that the new features work like expected.

@tynbendad can you confirm that the latest PR works, can you also test the Web UI?

I noticed we have the 2 days limitation only implemented in the Web UI. I wonder if this is enough. I think we should document it in the treatments API swagger, or even prevent the API from not deleting recent treatment data. Thoughts?

[tynbendad]

i think if you're using the REST API directly you are aware you can break things, doesn't need extra protection. i'll try to update git on this one, test, and then switch over to dev and test the other one...

[tynbendad]

i managed to update my github, resync azure, and test each of the new delete admin tools - watched each db on mlab and the deletes worked fine (i actually only had a few days of devicestatus since i had an issue a few days ago and wiped it out. keeping just 1 day of devicestatus worked fine from the web UI).

thinking about this though, the reason i was trying to use the REST API is because i wanted to make sure my rig kept the db's size reasonable (i was aiming for 1.5 years of treatments/entries and just a few weeks of devicestatus). otherwise, once the db gets unreasonable it is already too late to use nightscout admin tools to reduce db size since nightscout crashes and won't restart.

so, the web UI is nice but the REST API is the way i really think it needs to be managed, perhaps all the closed loops need this cleanup built in. unless the nightscout crash on full db can be fixed, at least enough to get to the admin tools...

[jpcunningh]

@tynbendad, unfortunately, if Nightscout crashes, I've found the API goes down with it. As I hit the full database events, I'm also trying to fix the crashes so it's still functional enough to use the database cleanup tools as you suggested. PR #4004 was one.

[PieterGit]

This PR has been integrated with the https://github.com/nightscout/cgm-remote-monitor/tree/needs_testing
branch. Please test that branch and report at this issue or at #4169

[PieterGit]

This PR has been tested as part of the need_testing branch of #4169
@blocklist_twitter @balshor @sulkaharo @jpcunningh @unsoluble @veryfancy @balshor @tynbendad have run this PR for a week. @tynbendad has confirmed that this PR works. No negative stuff popped up Gitter link: https://gitter.im/nightscout/public?at=5c2f33d12863d8612bd78eb0

If you encounter bugs with this PR, please create a new issue and link it to this PR.
Closing PR, will be merged with #4169

[PieterGit]

The user documentation for this feature can be improved. I logged issue nightscout/documentation#2 for that.