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

Refactor tracker code for clarity, modularity and so plugins can have more granular control over tracking #8317

Merged
merged 38 commits into from Aug 7, 2015

Conversation

Projects
None yet
4 participants
@diosmosis
Member

diosmosis commented Jul 9, 2015

This PR heavily refactors the tracking code to make it easy for plugins to tweak, change and extend tracking behavior. It moves a big chunk of the code currently in Visit::handle() to other plugins, like Goals, Actions, CustomVariables, etc.

Note: Though this PR implements enough changes for the plugin I'm working on, it doesn't completely close #8071, there's still more logic that can be moved out of core.

Concepts

This PR introduces the concept of the RequestProcessor. RequestProcessors handle and tweak tracker behavior. To learn more, read the class docs for the RequestProcessor class.

Changes

  • Added the RequestProcessor base class and added a new collection to DI, tracker.request.processors.
  • Made Tracker\Settings a stateless service and moved it to DI.
  • Gutted the Tracker\Visitor class. All the real logic was moved to a new stateless service, VisitorRecognizer. Visitor still exists because it is used in Dimension hooks, but it is at best a facade now.
  • Created a new Heartbeat plugin to hold the ping tracker behavior.
  • Moved action related tracking code in Visit::handle() to the Actions plugin, moved custom variables related tracking code to CustomVariables, moved goals related code to Goals, moved Ecommerce related code to Ecommerce and moved some core Visit handling code to CoreHome.
  • Added a method to the Plugin class, isTrackerPlugin(). Plugins that don't implement dimensions or use tracker events can override this method to make sure they are loaded during tracking (if they need it).

Future TODO

This TODO list can be added to #8071 or to a new issue:

  • Make GoalManager a stateless service and store in DI.

  • Move Action class & related code to Actions plugin. Move GoalManager to Goals plugin. Move other plugin related code to plugins from core.

  • Move current code to insert/update visit data in core to CoreHome. To do this correctly, would need a new service (eg, VisitRecorder), so VisitRequestProcessor does not get bloated (or more bloated).

  • Remove lots of tracking events, I think many (if not most) are no longer needed.

  • Create a new method in RequestProcessor specifically for manipulating request metadata. Right now, afterRequestProcessed() is used for both defining metadata that depends on other plugins' metadata and for manipulating other plugins' metadata. This means metadata that uses other plugins' metadata, can't be manipulated. Order of plugins will be come a problem eventually if many plugins get involved together...

  • Refactor the system again for clarity and ease of use. This makes it possible to split up the code and correctly scope classes, but the code is still verbose and the tracking logic is still not obvious. Before the system is made @api it should be made as easy to use as possible.

  • After it is possible to inject "component" types, RequestProcessors should use the "component" scheme.

  • Decouple VisitorRecognizer from Tracker classes and add integration test for it.

  • Do not use multi-dimensional array of properties for request metadata, instead use array of classes defined by plugins, so:

    /** @var $data MyPluginRequestMetadata */
    $data = $request->getMetadata('MyPlugin');
    

Refs #8071

@diosmosis diosmosis self-assigned this Jul 9, 2015

@diosmosis diosmosis added this to the 3.0.0 milestone Jul 9, 2015

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Jul 10, 2015

Member

@tsteur @mattab This PR isn't quite done yet (there are some TODOs in the code and I need to move/write some tests), but I think you can review it now. I need this for funnels, so it would be good if it didn't wait too long.

Member

diosmosis commented Jul 10, 2015

@tsteur @mattab This PR isn't quite done yet (there are some TODOs in the code and I need to move/write some tests), but I think you can review it now. I need this for funnels, so it would be good if it didn't wait too long.

@mattab mattab modified the milestones: 2.14.1, 3.0.0 Jul 10, 2015

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Jul 10, 2015

Member

(moved to 2.15.0)

Member

mattab commented Jul 10, 2015

(moved to 2.15.0)

@mattab mattab modified the milestones: 2.14.1, 2.15.0 Jul 10, 2015

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Jul 11, 2015

Member

FYI, this PR is done now. There may be more TODO after it is merged though.

Member

diosmosis commented Jul 11, 2015

FYI, this PR is done now. There may be more TODO after it is merged though.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Jul 12, 2015

Member

Hi @diosmosis

could you do here a little risk analysis and find out the list of plugin names that may break, because of this change? (from our open source and/or premium and/or client custom plugins)

It is acceptable breaking plugins that have a class extending Tracker\Visit in 2.15.0 (because Tracker\Visit is not tagged with @api) but it will help to know list of plugins we break here. (I write here about Tracker\Visit as you mentioned it on Slack, and maybe others parts have changed too - didn't check)

Thanks

Member

mattab commented Jul 12, 2015

Hi @diosmosis

could you do here a little risk analysis and find out the list of plugin names that may break, because of this change? (from our open source and/or premium and/or client custom plugins)

It is acceptable breaking plugins that have a class extending Tracker\Visit in 2.15.0 (because Tracker\Visit is not tagged with @api) but it will help to know list of plugins we break here. (I write here about Tracker\Visit as you mentioned it on Slack, and maybe others parts have changed too - didn't check)

Thanks

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 20, 2015

Contributor

FYI searched with https://piwikcodesearch.herokuapp.com and couldn't find community plugins extending the Visit class. Found 3 Piwik Pro plugins though: https://github.com/search?q=user%3Apiwikpro+%22Piwik%5CTracker%5CVisit%22&type=Code

Contributor

mnapoli commented Jul 20, 2015

FYI searched with https://piwikcodesearch.herokuapp.com and couldn't find community plugins extending the Visit class. Found 3 Piwik Pro plugins though: https://github.com/search?q=user%3Apiwikpro+%22Piwik%5CTracker%5CVisit%22&type=Code

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Jul 20, 2015

Member

Good to know! an issue was created in each of these plugin tracker.

Member

mattab commented Jul 20, 2015

Good to know! an issue was created in each of these plugin tracker.

Show outdated Hide outdated core/Plugin.php Outdated
Show outdated Hide outdated config/global.php Outdated
@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 5, 2015

Member

In general can we add unit / integration tests for the new files in /core?

Member

tsteur commented Aug 5, 2015

In general can we add unit / integration tests for the new files in /core?

diosmosis added some commits Jul 9, 2015

Move Actions recording to ActionsRequestProcessor from Visit::handle(…
…), and move last use of GoalManager from Visit::handle() to GoalsRequestProcessor.
Fix function signature in VisitRequestProcessor and move tests in Vis…
…itTest regarding isVisitNew to new test case for VisitRequestProcessor.
Re-add Request parameter to RequestProcessor::recordLogs(), make Goal…
…Manager stateless and store in DI, move logic in GoalManager that determined info about a tracking request to request metadata, change someGoalsConverted request metadata to goalsConverted metadata so we can see exactly which goals were converted.
Fixing regression in GoalsRequestProcessor, make sure to set visit to…
… converted if there were detected goals, but do not unset it if there are none.
@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 7, 2015

Member

Okay, I reckon we can have a look at it again when it becomes public API and hopefully by then we have maybe a different naming anyway (eg User and Session) etc

Member

tsteur commented Aug 7, 2015

Okay, I reckon we can have a look at it again when it becomes public API and hopefully by then we have maybe a different naming anyway (eg User and Session) etc

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Aug 7, 2015

Member

Did quick benchmark, Visit::handle() does not appear to benchmark. Will merge shortly so new plugin can be demo-d.

Member

diosmosis commented Aug 7, 2015

Did quick benchmark, Visit::handle() does not appear to benchmark. Will merge shortly so new plugin can be demo-d.

diosmosis added a commit that referenced this pull request Aug 7, 2015

Merge pull request #8317 from piwik/8071_tracker_req_handler
Refs #8071, refactor tracker code for clarity, modularity and so plugins can have more granular control over tracking. Introduce RequestProcessor & request metadata concepts & use to split up Visit::handle() method so tracking logic is located in the relevant plugins.

@diosmosis diosmosis merged commit 7829898 into master Aug 7, 2015

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
Scrutinizer Analysis: 37 new issues, 60 updated code elements — Tests: passed
Details

@diosmosis diosmosis deleted the 8071_tracker_req_handler branch Aug 7, 2015

diosmosis added a commit that referenced this pull request Aug 7, 2015

diosmosis added a commit that referenced this pull request Sep 1, 2015

Refs #8317, fixing regression caused by refactor, post event Tracker.…
…newVisitorInformation using reference to array, not by value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment