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

Architecture overhaul (SDK 2.0) #148

Merged
merged 22 commits into from Feb 11, 2017
Merged

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Feb 5, 2017

Architecture rework for a 2.0 release.

Notes:

  • Improved testing by allowing better injection mock objects. More test control and increased test coverage.
  • auth_token which was previous depcrecated has now been removed from code, including all QueryParams that only work in conjunction with a token. IOS SDK also does not support it and using it was semi broken because the token was only supplied when doing POST, but not in GET requests.
  • Each Tracker now has it's own settings object, which paves the way for multiple Trackers (with different endpoints). To support different preferences, but still allow changing domain names or site IDs, a unique name needs to be supplied during Tracker creation. The class LegacySettingsPorter ports previous settings over to the first Tracker created.
  • As per @eldk's idea, an opt-out action now takes effect retroactively and also wipes out the Dispatchers queue and offline cache.

Event.class is responsible for encoding, not Tracker.class or Dispatcher.class, this simplifies testing as less parsing/conversion is necessary, meaning more directly testing what needs to be tested in each class.
Removed QueryParams that only work in conjunction with an auth_token.
It was not working correctly anyways as only POST not GET transmissions contained an auth_token.
DryRun option can now be set per Tracker.
…houldn't enforce this.

By moving it to Piwik.class and making the constructor public we allow devs more choice over `Tracker` creation (especially using multiple trackers.
Refactored classes to allow for better testing and injecting mocks.
@d4rken d4rken changed the title Architecture overhaul Architecture overhaul (SDK 2.0) Feb 5, 2017
Each Tracker can now have their owns settings.
Previously Tracker objects would affect each others settings.
Each Tracker now gets a unique name to allow retaining settings even if URL or ID changes.
@d4rken
Copy link
Member Author

d4rken commented Feb 5, 2017

@eldk d4rken@54658f0

@dotsbb dotsbb self-requested a review February 6, 2017 11:50
@dotsbb
Copy link
Member

dotsbb commented Feb 6, 2017

COVERAGE INCREASED (+2.3%) TO 92.459%

Core = Piwik, Tracker, TrackMe, QueryParams (+ internal dispatcher implementation)
Extra = Everything else.
…ables with Custom Dimensions.

Also: Previous use of CustomVariables was flawed. Transmission with every track was superfluos (waste of traffic).
@d4rken
Copy link
Member Author

d4rken commented Feb 6, 2017

  • CustomVariables have been deprecated. Most noticeably Tracker.setVisitCustomVariable has been removed and should now be used like TrackHelper.track(CustomVariables.toVisitVariables()).screen(...).with(Tracker) or TrackHelper.track().visitVariables(....).screen(...).with(Tracker). While a bit more work than before this makes it clear that visit scope variables have to be transmitted (not just set to the tracker). Not only was the previous approach missleading, but we transmitted the visit scoped variables with every track request, even if they didn't change which wasted traffic. The new use pattern is similar to how the replacement for variables, "custom dimensions", is used.
  • To promote writing better code and making clear how Piwik works, the CustomDimensions extension of TrackMe has been removed. Instead there are now helper methods to set/get custom dimensions from TrackMe's and our friendly little TrackHelper now has a Screen.dimension as replacement for the deprecated Screen.variable method.
  • TrackHelper.dimension has been added to allow tracking dimensions conventiently with any typ of action in TrackHelper.

Usage examples can be seen here: ba25abf#diff-a9f2a402bc64d1dc87638db64836ec35

@d4rken
Copy link
Member Author

d4rken commented Feb 6, 2017

Migrating from CustomVariables to CustomDimensions is not possible without data loss, or is it?
Added a few more (deprecate tagged) helper methods (I still use custom vars myself).

@dotsbb dotsbb removed their request for review February 6, 2017 20:47
@d4rken
Copy link
Member Author

d4rken commented Feb 6, 2017

I know it's quite a handful of changes, but I felt that this was necessary to future proof the SDK. A modern Piwik 3.0 deserves a modern Android SDK...

I need help with making sure that the server-side analytics is not affected by these changes. I've done testing in my own app and with the example app, would be good if we could get a few more apps to test that switching doesn't alter analytics.

Guide:

  • Run existing app with old sdk
  • Check analytics
  • Integrate new sdk
  • Give feedback on new sdk api
  • Run existing app with new sdk
  • Check analytics
  • Give feedback

looks at the usual suspects 😉 @dotsbb @eldk

@dotsbb dotsbb self-requested a review February 6, 2017 21:21
@dotsbb
Copy link
Member

dotsbb commented Feb 6, 2017

A modern Piwik 3.0 deserves a modern Android SDK...

Brilliant!

@d4rken I'll look at this for sure, but it will take some time, so please be patient ;)

BTW what do you think about releasing current dev branch with offline mode as 1.1.0 version?

@eldk
Copy link
Contributor

eldk commented Feb 7, 2017

Hello,
Thanks @d4rken ,

I have done a release (stable) of my app with what @dotsbb call 1.1.0 and add +optout, optin wifi, optin always mode (only one check).

I will include and test 2.0 in my beta release in next days (one week max), I have a bunch of thing to do on other stuff.

Maybe it's a good idea to release a 1.1.0, keep dev for 1.1.n, and have a 2.0 beta branch with its own dev ?

Thanks,

Eric

@d4rken
Copy link
Member Author

d4rken commented Feb 7, 2017

BTW what do you think about releasing current dev branch with offline mode as 1.1.0 version?

Don't really like it, because the offline mode is a change that will affect not only analytics but also app behavior (storage, traffic). I would prefer this comes along with a breaking change so devs are aware of what's new.

I think we could change the dev code branch such that offline mode is disabled by default, tag that as v1.1.0 and release.

Maybe it's a good idea to release a 1.1.0, keep dev for 1.1.n, and have a 2.0 beta branch with its own dev ?

I don't think it's necessary to split development of 1.X and 2.X, after some testing/review from you @dotsbb we can merge this into dev.

Worst case: We make a PR against master to fix a bug 😁 .

@d4rken
Copy link
Member Author

d4rken commented Feb 7, 2017

I have done a release (stable) of my app with what @dotsbb call 1.1.0 and add +optout, optin wifi, optin always mode (only one check).

Could you make a PR of these changes against the current dev branch? I would like to look at them and maybe we include it in a 1.1.0 release.

What do you think about disabling offline mode by default?

@eldk
Copy link
Contributor

eldk commented Feb 7, 2017

What do you think about disabling offline mode by default?
I think this should be the dev choice.

As I use "Opt-in" in my app, user must say YES to send datas, if user don't, no datas are collected and dispatched, so no connexion needed, no connexion choice to do.

Some others devs, may prefer to use "Opt-out" : user must say NO, if user don't, datas are collected and dispatched, so connexion choice may be given.

In EU, and in France (I better know), from legal point of view, "Opt-in" is always the final choice, on existing legislation or when a new one is made.

As I have used/tested some other tools, before to make PIWIK choice, a lot of them are "Opt-out", or better done to use it.

So for some, enable connexion by default will be good, others may have or prefer to have it defaulted to disable.

Could you make a PR of these changes against the current dev branch?

I will do it before end of week

@d4rken
Copy link
Member Author

d4rken commented Feb 7, 2017

@eldk I was refering to offline mode, not all data collection. I think we should disable offline mode by default in 1.X and enable it by default in 2.X.

@d4rken d4rken changed the base branch from dev to 2.0 February 11, 2017 09:09
@d4rken d4rken merged commit 5109328 into matomo-org:2.0 Feb 11, 2017
@d4rken d4rken deleted the pr-architecture branch February 11, 2017 09:10
@d4rken
Copy link
Member Author

d4rken commented Feb 11, 2017

I've gone with @eldk's idea and created a 2.0 branch that we can later merge into dev when it's done. This makes it makes easier to submit further PRs against the 2.0 branch (as you will see in a min 😛).

@mattab mattab mentioned this pull request Apr 15, 2017
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants