Page MenuHomePhabricator

Convert one iOS schema to new MEP system
Closed, DuplicatePublic

Description

Suggest that we consider the History schema as it is small and clean and recently worked on, so its well known. But defer to you all for final decision.

[details to be filled in]

Event Timeline

LGoto triaged this task as Medium priority.Aug 10 2020, 6:53 PM
LGoto moved this task from Needs Triage to Product Backlog on the Wikipedia-iOS-App-Backlog board.

@mpopov Just assigning tasks on our blocked column so we know who's court it's in. Let us know when this is ready again for PR review. Thanks!

mpopov subscribed.

Ready for review!

@mpopov @jlinehan To limit risk with this new library, how would y'all feel about only deploying v1 in one of our public beta builds? Our latest beta has 544 unique installs so far, but we might be able to find ways to bump those numbers up if that's too low to get a good gauge on how the system is working. Let us know what you think.

@mpopov @jlinehan To limit risk with this new library, how would y'all feel about only deploying v1 in one of our public beta builds? Our latest beta has 544 unique installs so far, but we might be able to find ways to bump those numbers up if that's too low to get a good gauge on how the system is working. Let us know what you think.

@Tsevener Hmm, that is likely going to be too low at least for our goal of understanding what's changed with a side-by-side comparison and using this as a pilot to guide implementation on other platforms. But safety comes first and I defer to you about what you need. Saying we did start out in the beta build, what kind of timeline would we be looking at for entering production?

@jlinehan it depends on the release of iOS14, which we don't have a set date on but seems likely this month. We'll update as soon as we know. But it would be a few weeks after that release goes out. So if iOS14 ends up being late September, we're looking at mid-late October for 6.7.1, which is also when we'd want to have v2 of the library done and out in production.

@Tsevener Just so we're on the same page here: 6.7 will be beta-only and will ship with EPCv1 and 6.7.1 will be the non-beta release and would ideally ship with EPCv2 but at the very least v1?

@jlinehan @mpopov actually let me review the PR and watch the traffic. If I don't see anything concerning there I'm good with this going out to production in 6.7.0 as originally planned. If something is concerning, another thought I had is we still have it in 6.7.0 but wrap it up in our soon-to-be A/B test system (not related to EPC) that we will be implementing for our first experiment, to lessen the impact but still give you enough data to be informative on your side. But let's not worry about that for now until I look at everything closer.

I have one more ask before I do final review - are you both okay with merging this PR into the library for v1? The goal was to make the calls more structured, so we aren't tinkering with the dictionaries themselves as much. It's the Codable/Generics thing we discussed last week and @mpopov tried - @JoeWalsh and I basically decided that this library can only be logged from Swift. When we do convert Objective-C calls in the future it won't be much lift to convert the funnels to Swift, and it would be moving in the right direction of where we want to go with the codebase anyway.

...it would be a few weeks after that release goes out [...] looking at mid-late October for 6.7.1

This seems fine to me, no problem. There's no need to step up numbers if we are essentially doing a dry run in the beta build.

which is also when we'd want to have v2 of the library done and out in production.

About this timeline, I'd need to take a closer look at what we have slated for v2 before saying anything concrete. If we were just moving v1 or v1.x into production, that might be different. We've added an agenda item for the next apps sync but maybe we should set up a time to talk on this sooner.

are you both okay with merging this PR into the library for v1?

I'll have to review it but because of the upcoming minor release we have time to try things out in v1 and then tune, so at a high level I think this is fine. What would help me here is to meet with you and @JoeWalsh and talk through how the legacy system vs this system might be used/called, to make sure that we aren't just mapping the structures of the legacy system onto the new system. I think we haven't yet talked in a more holistic way about how the instrumentation might be organized, structured, etc., and I think Joe's patch speaks more to that. We also saw a little of this legacy mapping in some of the network controller patch.

What do you think about coming together sometime this week for an hour or something?

I think we haven't yet talked in a more holistic way about how the instrumentation might be organized, structured, etc., and I think Joe's patch speaks more to that.

Correct, the patch is mostly about making the instrumentation side a bit easier to use & maintain long term.

What do you think about coming together sometime this week for an hour or something?

Works for me

I just uploaded what I would like to be the final commit for v1 – the initial release of the Event Platform Client for iOS and the pilot instrumentation. The library is now using the production endpoint for retrieving stream configs, so the PR is now ready to merge into main and ship with 6.7, unless there are any objections.

@JoeWalsh: I had a chance to check out the PR-to-the-PR (insert "yo dawg I heard you like pull requests" meme here) and there's a lot of changes and ideas mixed together in there. Some are optimizations and some are fundamental shifts to the concepts informing the design of the system, which is to say it gives us plenty to geek out and deliberate about :)

As with the other suggestions/ideas which came up during code review that have been added to T261987 for v2, I don't think we should rush through #3682 to try to get it into v1. We should take the time to agree on the direction, resolve open questions, properly review it, and iterate as needed.

@mpopov Thanks for checking the PR on a PR! Apologies if it felt mixed or unclear. The goal was to make the library easier for us to use and maintain going forward by replacing Objective-C structures and norms with their Swift equivalents. We stopped new development in Objective-C over two years ago, so it’s important that the library fits in with Swift norms. We’re willing to take the tradeoff of updating any legacy Objective-C logging code.

I didn’t intend it to be a fundamental shift, but if it is raising fundamental design questions, let's resolve them before merging to main and shipping. I've pushed an update that removes the change to the structure of the resulting POST body, so that's one less change to consider.

@ABorbaWMF I'm not sure if there's much for QA to test here, but I would just do some basic regression testing after exploring the article history & diffs screens (we are logging to the new system from those screens).