Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event logging client for (Modern) Event Platform #3648

Merged
merged 56 commits into from
Sep 14, 2020

Conversation

bearloga
Copy link
Member

@bearloga bearloga commented Jul 30, 2020

Fixes Phabricator ticket: T228180

Notes

This pull request (a re-do of #3603) adds the iOS version of the Event Platform Client (EPC-iOS) for integration with the modern Event Platform. EPC is an effort to unify algorithms, identifiers, and behaviors across MediaWiki, iOS, and Android.

This initial release supports:

For additional background information on instrumentation within the Modern Event Platform, please refer to this page.

Next steps

  • While the library has already been heavily reviewed and iterated on in the previous PR, but it wouldn't hurt to review again – especially with respect to thread safety
  • Implement StorageManager and NetworkManager to make EPC operational
    • Temporarily remove storage manager facilities (except app install ID & usage data sharing opt-in/out setting retrieval) for initial release until we settle on specification of persistence
  • Restore EPC.shared singleton (was temporarily changed until storage & network managers are implemented)
  • Resolve open question around per-wiki stream configurations (in BUOD <> Apps sync agenda)
    • Will not have per-wiki stream configurations for apps, any language-specific deviations in sampling settings may need to be handled via the proposed exemptions idea
    • Apps will query 1 MW API (metawiki's) to download stream configs (side note: EventGate uses metawiki's stream configs as its source of truth)
  • Make EPC respect user's usage data sharing preference the way Event Logging does
  • Q&A functionality with a simple schema & instrument (test schema+stream for analytics events tracked in T259714)
    • Create a schema in this repository: /analytics/test/1.0.0
    • Configure a stream via this file: test.instrumentation and test.instrumentation.sampled
    • Test events verified in database/table (event.test_instrumentation)
  • Pilot instrumentation (Edit History Compare feature)
    • Schema (/analytics/mobile_apps/ios_edit_history_compare)
    • Instrumentation (EditHistoryCompareFunnel.swift)
    • Stream (ios.edit_history_compare)
  • BLOCKER EventStreamConfig API returns empty configs as [] instead of {} (T259917)
    • Merged
    • Deployed (scheduled for 2020-09-08 MediaWiki deployment train)
    • configURI changed from fake endpoint at pai-test to production endpoint at meta wiki

Future release may support:

  • stream cc-ing, which automatically logs events to multiple streams without requiring multiple calls on the instrumentation side

tonisevener and others added 4 commits July 31, 2020 16:36
Tabling it for now. It may be added at a later time once we fully figure out how it should work. That discussion is taking place in Phab:T256165
@bearloga
Copy link
Member Author

bearloga commented Aug 4, 2020

Just wanted to note that we (Product Infrastructure Data team) decided to put the stream cc-ing feature on the back-burner, as there are still some unresolved questions around how it should really work (implicit mapping as was implemented here or explicit mapping as suggested in T256169#6254835), and we didn't want this feature to be out on iOS from the get-go and potentially never on MediaWiki. All deliberation will happen in T256165.

@tonisevener
Copy link
Collaborator

@bearloga PR has been updated with StorageManager and NetworkManager implementations. I also added a couple of (identical) test log calls - one when you pull to refresh on the Explore tab and another when you tap the Settings icon on the Explore tab. I did a little bit of renaming and some more thread safety stuff in the library, hope that's cool. Let me know how this looks on your end, if it seems good we can bring it back to our side for converting a particular feature's logging & final code review.

@tonisevener tonisevener added the WIP label Aug 5, 2020
@bearloga
Copy link
Member Author

Note to self: depending on the outcome of the discussion taking place in the identifiers fragment patch (https://gerrit.wikimedia.org/r/c/schemas/event/secondary/+/615559/3/jsonschema/fragment/analytics/identifiers/current.yaml#22), session_id and device_id may need to be renamed to app_session_id and app_install_id

bearloga and others added 4 commits August 17, 2020 15:00
See `mw.user.generateRandomSessionId()` (mediawiki.user.js in MediaWiki Core)
- rename identifier fields (device_id => app_install_id, session_id => app_session_id) to be consistent with current proposal of identifiers schema fragments
- add constraint to stream config URI (will help conserve bandwidth)
@bearloga
Copy link
Member Author

@joewalsh Thanks for fixing the tests and making those updates!

Status update: We're getting closer to being able to create the pilot schema. There was a lot of discussion and iteration on https://gerrit.wikimedia.org/r/c/schemas/event/secondary/+/615559 and we've arrived at a version we're all satisfied with, so that'll be merged tomorrow (Friday) and I'll probably have pilot schema + pilot instrument up for review by EOD/EOW. Exciting!!!

Copy link
Contributor

@joewalsh joewalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great overall! I did my review in the form of proposed changes via a PR on this PR. The review comments/questions are in that PR: #3682

@tonisevener
Copy link
Collaborator

I have been reviewing this as a part of the refactor PR and from my perspective these are ready to merge into main. I had one possible cleanup comment here, but client_dt has tripped me up in the past so I could just be missing something. We're happy to discuss the refactor further in our meeting today.

@bearloga
Copy link
Member Author

From the meeting: We decided to move forward with this PR once #3682 is merged into it

@tonisevener tonisevener removed the WIP label Sep 14, 2020
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎊 Thanks everyone for their hard work on this! Very excited to get this in.

@tonisevener tonisevener merged commit 48bf450 into main Sep 14, 2020
@tonisevener tonisevener deleted the event-platform-client branch September 14, 2020 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants