Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Analytics integration #881

Closed
iamjazzar opened this issue Nov 21, 2016 · 4 comments
Closed

Analytics integration #881

iamjazzar opened this issue Nov 21, 2016 · 4 comments

Comments

@iamjazzar
Copy link
Contributor

Hello,

I'm currently working on integrating Fabric analytics in the app, and I saw that you are working on Google Firebase Integration in #879 and this is really cool. The problem is that all events are fired in each screen using segment, regardless of the integration type.

I tried to make something generic but that leaded to a huge code change (around 57 files) which I prefer not to do because of the merge conflicts I'll face later. I actually have no questions here, but I wanted to check if you are interested in having such a change so may be I can introduce my changes in a pull request.

@iamjazzar iamjazzar changed the title Analytic integration Analytics integration Nov 21, 2016
@BenjiLee
Copy link
Contributor

Let us hear your thoughts! We have an open ticket right now where we will actually be attacking the way analytics works on android. One of the issues with segment is that an open edx user needs to use segment in order to get analytics. We've brainstormed a couple of ways to go about this but I think we will be leaning towards making a generic analytics interface.

Here is the ticket to track progress. https://openedx.atlassian.net/browse/MA-2981

The ticket is vague but at least you will be able to see the status.

@iamjazzar
Copy link
Contributor Author

Cool! Here's what I think:

Going back to the configs repo, we can see that it's all up to the developer to pick Segment and/or any other service to track the events in the app, and since we have a ready implementation for Segment, we can refactor ISegment.java to be the interface and ISegmentEmptyImpl.java to be the Empty implementation of all analytics services the user is interested in. Besides, we can take an advantage of RuntimeApplication.java that has been introduced in #583 so that any developer can define any desired analytics service and check for its flags. All these defined and configured services are going to be passed to a controller class that executes an event on all these events using Command Pattern design.

P.S. If you can correct me, does edx-platform sends all the events to SegmentIO if its configured so that you want to keep Segment enabled?

@BenjiLee
Copy link
Contributor

@miankhalid @1zaman FYI

All that makes sense. We currently have all the checks in https://github.com/edx/edx-app-android/blob/master/OpenEdXMobile/src/main/java/org/edx/mobile/base/MainApplication.java but maybe they should be moved to RuntimeApplication or something. I'm sure there will be some refactoring involved.

For the PS, At edx, we use Segment to send our events to edx-platform AND google analytics via segment. Since we are open source, we understand this doesn't work for everyone so there would be the case where segment is not enabled, but Google Analytics is.

@iamjazzar
Copy link
Contributor Author

@BenjiLee: I think we can close this as it is solved in #893. Thanks @BenjiLee and @miankhalid for helping in making this happens, and thanks @1zaman for the review. Again, sorry for not being so involved because of the time difference.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants