Page MenuHomePhabricator

MEP Client iOS (Revision)
Open, MediumPublic

Description

With initial ("v1") release of EPC-iOS getting finalized, there are some changes/improvements and features we're noticing that should be included in a revision ("v2").

TODO/Wishlist

Top priority

  • Restoring event persistence so we accumulate events in offline mode (this is an alternate, and probably more reliable way to accomplish offline event logging than waitsForConnectivity). We will need to persist both input and output buffers so we don't lose them between app termination and launching, before they are able to POST. For future reference, the version of the library at this commit has the StorageManager functionality in place.

Others

Related Objects

Event Timeline

mpopov triaged this task as Low priority.Sep 3 2020, 8:17 PM
mpopov created this task.
Tsevener added a subscriber: Tsevener.

Updated the task description to lay out restoring StorageManager (or however we want to do it), also removed mentions of NSCoding -> Codable since that's making it into v1.

Mainly so this doesn't get lost, I think we should also look at my delta investigation to consider plugging some of the holes with this beyond adding persistence. Specifically:

  1. The legacy system EventLoggingService.swift has Wifi-only logic for a day that then switches to LTE. That could be the cause of some discrepancies between the systems, so should we duplicate this logic into the EPC library?
  2. We should dig into which error types are considered network failures that merit retries between both systems and understand if they need to match.
  3. We should dig into which HTTP status codes are considered successful posts between both systems and understand if they need to match.
jlinehan raised the priority of this task from Low to Medium.
jlinehan moved this task from Task Backlog to Next on the Product-Data-Infrastructure board.

In https://github.com/wikimedia/wikipedia-ios/commit/9be85f1303d3d41e9cbc8f408342aa69d604dd61 (the first of 2 commits removing the EPC storage manager), @mpopov sez:

There are some open questions around storage behavior we need to resolve.

What are those questions? Are they written down somewhere already?

What are those questions? Are they written down somewhere already?

I think buried somewhere in some Slack DM or in the PR's comments. But for easy access – they're questions of:

  • how many events/requests?
  • for how long?
  • whether at all?
  • how exactly, from technical implementation perspective?
    • whether on iOS we should use simple storage where app install ID is stored
    • or Core Data framework (Toni's original implementation for queued-up http requests)

around

  • input buffer of events which have been logged/submitted but not processed due to unavailability of stream configs
  • output buffer of events which are waiting to be HTTP POSTed

@Mholloway @mpopov

For a bit of background on my approach at the time, it was heavily inspired by the legacy system. The meat of that is in EventLoggingService.swift. Though whereas that is more of a self contained single class, I teased apart the pieces that involved data persistence / Core Data stuff and moved them into a separate StorageManager class. The same goes for any networking calls, I moved those into a separate NetworkManager class. I still picture it like that for this v2, the EPC class being the entry point and center of a lot of the business logic, but delegating any specific persistence or network calls out to StorageManager and NetworkManager (which EPC instantiates and owns). Ideally only StorageManager would have knowledge of Core Data stuff.

how exactly, from technical implementation perspective?
whether on iOS we should use simple storage where app install ID is stored
or Core Data framework (Toni's original implementation for queued-up http requests)

Currently appInstallID is also stored in Core Data - it's inside the legacy system's Core Data database (EventLogging.xcdatamodeld). Since that value needed to be the same it made sense to keep it there and have EPC ask the legacy system for it. I would prefer that we create an entirely new Core Data database to persist anything new from EPC there (that would mean a new xcdatamodeld), just to have a clean separation. I had it that way back in the original StorageManager commits for reference. The only gross thing that I never solved was having NSCoding peppered everywhere. Since then we have replaced data serialization with Codable in EPC, and I would like to find a way to keep that over NSCoding since it's more recent and Swiftier. @Mholloway definitely reach out to me if you have any roadblocks or questions with these pieces. I'm not certain on how to do it without digging in. At this point it's just a preference in my head.

The top priority (storage manager) is code-complete, but let's leave this umbrella task in the backlog for the other odds and ends like batching submissions, revisiting thread safety, etc.

StorageManager PR (now merged into main): https://github.com/wikimedia/wikipedia-ios/pull/3806

@Mholloway @JMinor Should I spin off the top priority item into a separate task? Wasn't sure if QA needed to do any testing with it separately or if we needed to track that it is past code review stage and merged on our 6.8 board.

I'll defer to @JMinor on the task tracking stuff. It's fine to reopen T259269, unless you want a new task specifically to track post-merge work.

We discussed this briefly in a board grooming meeting yesterday and would appreciate a review by the iOS team about which of the remaining items are still needed or wanted, and how they should be prioritized (individually or collectively). As far as I can tell, the remaining "possibly moot if persistence is solved" items still remain open and valid, but I could be wrong.

Also,

  • The legacy system EventLoggingService.swift has Wifi-only logic for a day that then switches to LTE. That could be the cause of some discrepancies between the systems, so should we duplicate this logic into the EPC library?

seems like overkill to me, and I would personally vote against replicating it in the new setup.

@Mholloway can we close this ticket based on the current library work we are doing?

OK if I keep this open for consideration while doing the library consolidation work, and close it out when that's finished?