Implements a new mechanism for analytics event tracking.

[diegoreymendez]

This PR proposes a new mechanism for analytics event tracking.

Existing mechanisms:

There are two existing mechanisms for tracking analytics. The older one requires that we add events to a huge list in the WPShared pod. See here.

The downsides are evident. The most important one being it forced us to release a new version of the Shared pod each time an event was added, removed or changed.

The newer one was recently proposed by @leandroalonso. It's much better than the old mechanism in that it allows us to define the events without having to release a new version of the shared pod. See here.

While there's a very clear upside in this new mechanism, it still resulted in a big list of event names within WPiOS.

Additionally, the logic for defining the event properties is not bundled together with the events - it's still the responsibility of the caller to define those properties.

About this proposal:

This proposal promotes a new mechanism for events to be defined in a way that all of their construction logic is bundled in a single spot. Events can also be declared and defined closer to where they belong.

This mechanism can co-exist with what we currently have, allowing us to incrementally migrate new events to it.

Pros:

1. Events can now be declared and defined closer to where they're used (if we want to). It's highly unlikely that events will be relevant across different flows, so having a huge list of events is not really an advantage.
2. The logic for building events can now be bundled in a single spot.
3. Strongly-typed and parametrised event properties on event construction (see the examples in the inline documentation).
4. Quite easy to use.

Cons:

1. Yet a new mechanism for tracking events. That said, this would be true for any architecture change we do. Additionally, we can post about it to try and align people to start using it more.
2. Feel free to raise any other cons you can see.

Example implementation:

As seen in this in-progress PR: https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/335/files

Event declaration + definition:

extension AnalyticsEvent {
     enum LoginStep: String {
         case start
         case success
     }

     static func login(step: LoginStep) -> AnalyticsEvent {
         let properties = [
             "source": "default",
             "flow": "google_login",
             "step": step.rawValue,
         ]

         return AnalyticsEvent(name: "login", properties: properties)
     }
 }

Tracking the event:

WPAnalytics.track(.login(step: .start))

Request for input:

I wasn't sure at first if to push this. @bummytime convinced me to push the proposal as a PR and seek feedback on it.

It took me a while to reach this solution - the initial implementation wasn't as clean. Right now my personal take is that it's quite cleaner and will help us make events easier to deal with and maintain.

@leandroalonso - Since you worked on this in the past. @jleandroperez - Since you helped me think about this more. @mindgraffiti, @ScoutHarris - I'm planning to use this for the new events in unified Google login and all of our upcoming work if it's approved. @aerych - Because I want your input too!

Also ping @frosty, fyi

[leandroalonso]

I like that idea, @diegoreymendez !

Also, the current WPAnalyticsEvent.swift is not that big. Tbh, most event there are mine :P

I can help by migrating them.

One last thing though, maybe you want to add some documentation inside WPiOS' WPAnalyticsEvent.swift so everyone is aware that there's a new way of creating events.

[diegoreymendez]

@leandroalonso - Thanks! I can also help you migrate events on the side if you want.

I think we just need to make sure nothing breaks and priorities are maintained (in my case I gotta make sure I deliver tracking in UL&S project).

[aerych]

Thanks for the ping! I'm a fan of the change. I like the idea of moving event definitions and constructors close to where the events are used. There might be a few cases where an event is triggered in multiple unrelated places but that should be the exception rather than the rule and I can't think of a particular instance off the top of my head. Would absolutely love to see the day we can get rid of that gigantic enum and just pass strings. With only a single tracker there's not much need for the enum to provide the additional layer of abstraction. (Just my two cents since we're on the topic.)

[leandroalonso]

@aerych

I can't think of a particular instance off the top of my head

I have a very good example (I was also thinking about that same thing). We have duplicated events in the Prepublishing Nudges and Post Settings. Eg.: editor_post_tags_changed (then a via property that can be settings or prepublishing).

Anyway, I don't think this is an issue. This event will probably exist in its own file and that's ok. :)

[diegoreymendez]

The PR is now ready for review, and I'll add all as reviewers all of the people that I pinged. I'm planning to merge this by Monday 27 at 12:00 pm GMT-3 as long as I have at least 1 approved review, and no rejections.

The provided time should be enough to that I can use it for the work I'm doing, but at the same time let everyone chime in.

If you need more time for whatever reason, please let me know!

[diegoreymendez]

@leandroalonso wrote:

I have a very good example (I was also thinking about that same thing). We have duplicated events in the Prepublishing Nudges and Post Settings. Eg.: editor_post_tags_changed (then a via property that can be settings or prepublishing).

Anyway, I don't think this is an issue. This event will probably exist in its own file and that's ok. :)

Exactly! As you mentioned, there should be enough freedom to declare the events wherever it's most appropriate. In the case of a shared event it will probably be declared in a shared location (or if it's more appropriate, declared redundantly in more than one spot).

[diegoreymendez]

@ScoutHarris - You're right. Changed!

[frosty]

Diego and I already spoke about this, but for completeness, I'll share my comments here. I think this looks like a great approach. My only real reservation is that it potentially spreads the analytics code throughout the app – the current approach means that if you need to find a particular event you have one (I guess now two with WPAnalyticsEvent) place to look.

For example, say you wanted to know what analytics events we use around domain credit redemption. You can open up WPAnalytics.h and instantly see:

WPAnalyticsStatDomainCreditPromptShown,
WPAnalyticsStatDomainCreditRedemptionSuccess,
WPAnalyticsStatDomainCreditRedemptionTapped,

With this new approach, you'd have to find the domain credit components in the app, and look for where any of them declare analytics events. This isn't a super common occurrence, but the question does sometimes come up around what we're tracking in a particular section of the app.

That said, there may be some way we could help make this kind of thing easier – for example standardizing where we declare these (like Component+Analytics.swift) or we might be able to write a script to pull out a list of all of these definitions.

I think on the whole the benefits outweigh this concern, but I just wanted to note it here.

[diegoreymendez]

Thanks @frosty . In light of these points I'll see how defining new events looks in the work that I'm doing, and consider alternatives to make it better if it feels too spread-out.

Maybe a specific analytics file in each pod / app / component, as you mentioned.

[diegoreymendez]

Thanks for the input everyone! As mentioned above, I'm merging now since I've an approval and no rejections.