LOOP-1201 Export of Critical Event Logs #242

darinkrauss · 2020-09-15T23:59:50Z

https://tidepool.atlassian.net/browse/LOOP-1201
Add critical event log export to several Core Data stores
Add Encodable to various Core Data models
Add modificationCounter to DeviceLogEntry
Add JSONStreamEncoder
Replace StoredSettings.InsulinModel with StoredInsulinModel
Fix PumpManagerStatus Codable for optional pumpStatusHighlight and pumpLifecycleProgress

Note: This will require @ps2 and one other reviewer. Also, I looked into breaking this into smaller PRs, but could not come up with a way to do so that made any sense. (I could have broken out the Codable implementations, but that doesn't make this PR any easier, just a bit smaller, and is not worth the work.)

Note: All of the store implementations of CriticalEventLog are very similar (with only a few differences unique to each store). Once you've understood one, you've understood 95% of the rest).

- https://tidepool.atlassian.net/browse/LOOP-1201 - Add critical event log export to several Core Data stores - Add Encodable to various Core Data models - Add modificationCounter to DeviceLogEntry - Add JSONStreamEncoder - Replace StoredSettings.InsulinModel with StoredInsulinModel - Fix PumpManagerStatus Codable for optional pumpStatusHighlight and pumpLifecycleProgress

darinkrauss · 2020-09-16T00:00:41Z

Related LoopWorkspace PR for demonstration of build: https://github.com/tidepool-org/LoopWorkspace/pull/250

ps2

LGTM. If these operations are really background operations, and we no longer need progress estimation; I'd recommend that be cleaned up; there is a lot of code around progress, and possibly some significant io due to the count(*) operations that I think are only necessary for progress reporting.

ps2 · 2020-09-16T20:12:02Z

LoopKit/CarbKit/CarbStore.swift


+// MARK: - Critical Event Log Export
+
+extension CarbStore: CriticalEventLog {


Agree with Rick; if this work is really isolated to an extension, putting it into a separate file might make this a little more navigable.

Okay, I can do this, but if we are going this way, we should make this a general, well-known policy. We haven't been doing this before.

I don't feel that strongly, though, FWIW

Yeah, it's not a big issue for me either; just seems a little more navigable.

I'd be fine if we don't do this here.

The code in this extension uses the managed object context, which is a private property. To move this extension out of the main file would require changing the property access from private to internal. I believe keeping the property private is more important than moving the extension to a separate file. Going to leave as-is unless anyone objects.

ps2 · 2020-09-16T20:13:50Z

LoopKit/CriticalEventLog.swift

+
+import Foundation
+
+public protocol EstimatedDurationProgressor {


The idea of cancelable progress and rolled up tasks seems really similar to the built in Progress class: https://developer.apple.com/documentation/foundation/progress

On a larger scale; if this is happening in the background, do we need progress reporting?

We need progress for the full export in case the nightly exports have (for whatever reason) not completed. It is an edge case, but it definitely needs progress. (Also, with my current work looking into a different zip implementation, it looks like it may take longer than 3-5 seconds for the full export to complete so we at least need a progress bar.)

I did look into Progress and at the time did not think it appropriate. I will revisit.

Updated to use Progress. Required a number of downstream changes, but should be good now. ✅

ps2 · 2020-09-16T20:25:45Z

LoopKit/DeviceManager/DeviceLog/PersistentDeviceLog.swift

+                let request: NSFetchRequest<DeviceLogEntry> = DeviceLogEntry.fetchRequest()
+                request.predicate = self.exportDatePredicate(startDate: startDate, endDate: endDate)
+
+                let objectCount = try self.managedObjectContext.count(for: request)


count(*) operations often require a full table scan (and thus a lot of io). I'm not sure that's the case here or how heavily you're relying on this being performant, but thought I'd mention it. Count can be faster than an actual fetch because you save on hydration; but since IO is usually the slowest part of the query, the count operation might be nearly as slow an actual fetch of all the objects involved.

We need progress (as discussed above) and progress highly depends upon the amount of data, thus the need for the count. The count has only a little effect on overall duration. (It was necessary to add indexes on dosing decisions and device comms table for exporting in general, not specific to progress, so this solves most of the issue).

rickpasetto

Wow, this is a lot of work! Great job, Darin!
Sorry, but I have a few questions that I need answered before I can approve. Happy to hop on a call to discuss.

rickpasetto · 2020-09-17T00:30:50Z

LoopKit/CarbKit/CarbStore.swift


+// MARK: - Critical Event Log Export
+
+extension CarbStore: CriticalEventLog {


I don't feel that strongly, though, FWIW

rickpasetto · 2020-09-17T00:34:41Z

LoopKit/CriticalEventLog.swift

+    ///   - stream: The output stream to write the critical event log to. Typically writes JSON UTF-8 text.
+    ///   - progressor: The estimated duration progress to use to check if cancelled and report progress.
+    /// - Returns: Any error that occurs during the export, or nil if successful.
+    func export(startDate: Date, endDate: Date, to stream: OutputStream, progressor: EstimatedDurationProgressor) -> Error?


[nit] Consider returning Result<Void, Error> or, if more appropriate, having the function throw

Per discussion, going to leave as-is.

rickpasetto · 2020-09-17T00:37:50Z

LoopKit/Extensions/OutputStream.swift

+
+import Foundation
+
+extension OutputStream {


[nit] unit test for this?

Good catch, done. ✅

rickpasetto · 2020-09-17T00:39:28Z

LoopKit/InsulinKit/DoseStore.swift

+        return result!
+    }
+
+    public func export(startDate: Date, endDate: Date, to stream: OutputStream, progressor: EstimatedDurationProgressor) -> Error? {


These export(...) functions on the Stores are so similar. Is there a way to factor them into common code?

I gave it a try and found there are too many differences between the different stores for it to be straightforward. It is possible, of course, but there would have to be at least on "customizer" function to account for the store differences and the final code would be less understandable.

For example, each store counts each object as a different amount towards overall progress (which is because each object takes a different amount of time to query, serialize, and write - there is a very large difference in object size between, say, alerts and dosing decisions). So each store would have to customize this cost while the common code is iterating over and exporting each object.

Also, while most stores use a modificationCounter property to sort and query on, the CarbStore uses an anchorKey property. These properties are used to provide a stable sort for paging data during an export.

These differences and others would make the code much less understandable if we tried to make a common "export" function.

Long term we could work on making the stores more similar in functionality (and property names) and this might be easier.

✅

rickpasetto · 2020-09-17T00:41:30Z

LoopKit/SettingsStore.swift

        self.syncIdentifier = syncIdentifier
    }
-
-    public struct InsulinModel: Codable, Equatable {


Hooray!! 💯

rickpasetto · 2020-09-17T00:45:35Z

LoopKit/CarbKit/CarbStore.swift

+
+    private func exportDatePredicate(startDate: Date, endDate: Date? = nil) -> NSPredicate {
+        var addedDatePredicate = NSPredicate(format: "addedDate >= %@", startDate as NSDate)
+        var supercededDatePredicate = NSPredicate(format: "supercededDate >= %@", startDate as NSDate)


I have a roughly similar question about this that I had with AlertStore. If you're looking for something "after" startDate, and supercededDate is never before addedDate, then why do you have to include it in the predicate?

Similarly for endDate: what if the record is modified with supercededDate that is later than an addedDate but endDate is in between them? i.e.: addedDate < endDate < supercededDate?

Since historical exports are captured on a daily basis it is necessary to cover the use case of the carb object added during one daily period and the carb object superseded in a subsequent daily period. If endDate is nil, then the final predicate can simplified slightly at the expense of slightly more complicated code, but that is not the common use case. (Most exports are historical and those will have an endDate specified.) Going to keep as-is.

rickpasetto

LGTM! 🥍

darinkrauss requested review from nhamming, ps2 and rickpasetto September 15, 2020 23:59

ps2 approved these changes Sep 16, 2020

View reviewed changes

rickpasetto reviewed Sep 17, 2020

View reviewed changes

Darin Krauss added 2 commits September 17, 2020 19:50

Updates from PR feedback

c4f9fb2

Additional updates from PR feedback

709677e

darinkrauss requested a review from rickpasetto September 18, 2020 23:42

rickpasetto approved these changes Sep 19, 2020

View reviewed changes

Merge branch 'dev' into darinkrauss/LOOP-1201-export-critical-event-logs

3daef79

darinkrauss merged commit d3851ce into dev Sep 19, 2020

darinkrauss deleted the darinkrauss/LOOP-1201-export-critical-event-logs branch September 19, 2020 04:11


		// MARK: - Critical Event Log Export

		extension CarbStore: CriticalEventLog {


		import Foundation

		public protocol EstimatedDurationProgressor {

LOOP-1201 Export of Critical Event Logs #242

LOOP-1201 Export of Critical Event Logs #242

Uh oh!

Conversation

darinkrauss commented Sep 15, 2020

Uh oh!

darinkrauss commented Sep 16, 2020

Uh oh!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickpasetto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darinkrauss Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickpasetto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

darinkrauss Sep 18, 2020 •

edited

Loading