@diosmosis opened this Pull Request on July 9th 2015 Member

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:

  • [x] 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 <a class='mention' href='https://github.com/api'>@api</a> 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:

    /** <a class='mention' href='https://github.com/var'>@var</a> $data MyPluginRequestMetadata */
    $data = $request->getMetadata('MyPlugin');

Refs #8071

@diosmosis commented on July 10th 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.

@mattab commented on July 10th 2015 Owner

(moved to 2.15.0)

@diosmosis commented on July 11th 2015 Member

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

@mattab commented on July 12th 2015 Owner

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 <a class='mention' href='https://github.com/api'>@api</a>) 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 commented on July 20th 2015 Member

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 commented on July 20th 2015 Owner

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

@tsteur commented on August 5th 2015 Owner

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

@tsteur commented on August 5th 2015 Owner

Also some of my comments depend on whether this becomes API or not. Will it be API?

@diosmosis commented on August 6th 2015 Member

FYI, moved the following exchange out of comments since it is now part of an outdated diff:

@tsteur:

I find this concept a bit "weird". Would it make sense to Request::setMetadata() on the request object? Visitor and request metadata doesn't sound like it belongs together maybe? Also maybe we can remove one dimension instead of $pluginName and $key just have $name if possible? Plugin name would be acceptable for me only if it was set by the platform automatically, if possible.

Is it possible to not have metadata but clear objects and what can be done on the Visitor object? Eg $visitor->setIsVisitorKnown($isKnown)? One problem that I have is that eg the core knows about plugins. Eg within core (/core) we do getRequestMetadata('CoreHome', 'isVisitorKnown'), but core should not know anything about the plugins.

@diosmosis:

If the metadata is stored in a core object, plugins cannot define their own metadata that can be changed by other plugins. Also the core object would have to have 'setters', since the logic to set them is accomplished through RequestProcessors, not externally.

Here is an alternative approach:

// in a RequestProcessor ... 
/** <a class='mention' href='https://github.com/var'>@var</a> ActionsRequestMetadata $data */
$data = $request->getMetadata('Goals');
$data->goalsConverted[] = array();

or maybe:

$data = $request->getMetadata('Piwik\Goals\Tracker\GoalsRequestMetadata');
// PHPStorm will be configured to set the type of $data to the string argument

To avoid naming collisions and strange errors, the plugin name should be used. Setting it automatically is possible, the plugin name can be detected from the namespace.

Eg within core (/core) we do getRequestMetadata('CoreHome', 'isVisitorKnown'), but core should not know anything about the plugins.

This is because I only moved what was necessary for the new plugin. There is a Future TODO item about moving the rest of the logic.

@tsteur:

At least some are expected to exist by core like CoreHome, isVisitKnown. Something like this is not metadata and would be better to have in classes/methods of core if possible (including setters). Once all is done in that plugin it could be maybe a metadata again.

@diosmosis:

Why should it be in core? Would it be unexpected for a plugin to change it? The logic that sets it isn't in core.

It can exist in a class instead of as an element in an array, see above. W/o support for generics, it will never look perfect.

@diosmosis commented on August 6th 2015 Member

Tried moving properties from VisitProperties to Visitor, can't be done currently since it is required to initialize Visitor w/ whether the visitor is known or not, and VisitProperties are needed before this point.

@tsteur commented on August 7th 2015 Owner

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 commented on August 7th 2015 Member

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

This Pull Request was closed on August 7th 2015
Powered by GitHub Issue Mirror