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

Goals per page - new reports and metrics for tracking page conversions #18221

Merged
merged 133 commits into from May 31, 2022

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Oct 26, 2021

Description:

Fixes #2030

Adds new reports for goal conversions by page url, page title and entry page to the goals overview, see the detailed specification in #2030 for full details.

System tests have now been added.

Review

@bx80 bx80 added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 26, 2021
@bx80 bx80 added this to the 4.6.0 milestone Oct 26, 2021
@bx80 bx80 self-assigned this Oct 26, 2021
@justinvelluppillai justinvelluppillai added the Needs Review PRs that need a code review label Oct 27, 2021
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Nov 12, 2021
@sgiehl sgiehl modified the milestones: 4.6.0, 4.7.0 Nov 18, 2021
@justinvelluppillai
Copy link
Contributor

@bx80 anything needed here to progress this?

@bx80
Copy link
Contributor Author

bx80 commented Nov 22, 2021

@justinvelluppillai Just needs reviewing.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Only had a very rough look through the code, as it actually doesn't seem to work.
Running the archiving on this branch results in an error for me:

ERROR [2021-11-22 15:38:22] 23504  Got invalid response from API request: ?module=API&method=CoreAdminHome.archiveReports&idSite=1&period=week&date=2021-11-15&format=json&trigger=archivephp. Response was '{"result":"error","message":"SQLSTATE[42S22]: Column not found: 1054 Unknown column 'log_conversion.server_time' in 'on clause' - in plugin Goals. #0 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(284): Piwik\\ArchiveProcessor\\PluginsArchiver->callAggregateAllPlugins('0', NULL, true) #1 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(176): Piwik\\ArchiveProcessor\\Loader->prepareAllPluginsArchive('0', NULL) #2 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(159): Piwik\\ArchiveProcessor\\Loader->insertArchiveData(0, 0) #3 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(99): Piwik\\ArchiveProcessor\\Loader->prepareArchiveImpl('VisitsSummary') #4 \/srv\/matomo\/core\/Context.php(75): Piwik\\ArchiveProcessor\\Loader->Piwik\\ArchiveProcessor\\{closure}() #5 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(103): Piwik\\Context::changeIdSite(1, Object(Closure)) #6 \/srv\/matomo\/plugins\/CoreAdminHome\/API.php(278): Piwik\\ArchiveProcessor\\Loader->prepareArchive('VisitsSummary') #7 \/srv\/matomo\/core\/Archive.php(847): Piwik\\Plugins\\CoreAdminHome\\API->archiveReports(1, Object(Piwik\\Period\\Day), '2021-11-17', '', 'VisitsSummary', '') #8 \/srv\/matomo\/core\/Archive.php(645): Piwik\\Archive->prepareArchive(Array, Object(Piwik\\Site), Object(Piwik\\Period\\Day)) #9 \/srv\/matomo\/core\/Archive.php(592): Piwik\\Archive->cacheArchiveIdsAfterLaunching(Array, Array) #10 \/srv\/matomo\/core\/Archive.php(518): Piwik\\Archive->getArchiveIds(Array) #11 \/srv\/matomo\/core\/Archive.php(328): Piwik\\Archive->get(Array, 'numeric') #12 \/srv\/matomo\/core\/ArchiveProcessor.php(630): Piwik\\Archive->getDataTableFromNumeric(Array) #13 \/srv\/matomo\/core\/ArchiveProcessor.php(265): Piwik\\ArchiveProcessor->getAggregatedNumericMetrics(Array, false) #14 \/srv\/matomo\/core\/ArchiveProcessor\/PluginsArchiver.php(306): Piwik\\ArchiveProcessor->aggregateNumericMetrics(Array) #15 \/srv\/matomo\/core\/ArchiveProcessor\/PluginsArchiver.php(104): Piwik\\ArchiveProcessor\\PluginsArchiver->aggregateMultipleVisitsMetrics() #16 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(278): Piwik\\ArchiveProcessor\\PluginsArchiver->callAggregateCoreMetrics() #17 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(176): Piwik\\ArchiveProcessor\\Loader->prepareAllPluginsArchive('22', 0) #18 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(159): Piwik\\ArchiveProcessor\\Loader->insertArchiveData('22', 0) #19 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(99): Piwik\\ArchiveProcessor\\Loader->prepareArchiveImpl(false) #20 \/srv\/matomo\/core\/Context.php(75): Piwik\\ArchiveProcessor\\Loader->Piwik\\ArchiveProcessor\\{closure}() #21 \/srv\/matomo\/core\/ArchiveProcessor\/Loader.php(103): Piwik\\Context::changeIdSite(1, Object(Closure)) #22 \/srv\/matomo\/plugins\/CoreAdminHome\/API.php(278): Piwik\\ArchiveProcessor\\Loader->prepareArchive(false) #23 [internal function]: Piwik\\Plugins\\CoreAdminHome\\API->archiveReports('1', Object(Piwik\\Period\\Week), '2021-11-15', false, false, false) #24 \/srv\/matomo\/core\/API\/Proxy.php(244): call_user_func_array(Array, Array) #25 \/srv\/matomo\/core\/Context.php(28): Piwik\\API\\Proxy->Piwik\\API\\{closure}() #26 \/srv\/matomo\/core\/API\/Proxy.php(335): Piwik\\Context::executeWithQueryParameters(Array, Object(Closure)) #27 \/srv\/matomo\/core\/API\/Request.php(266): Piwik\\API\\Proxy->call('\\\\Piwik\\\\Plugins\\\\...', 'archiveReports', Array) #28 \/srv\/matomo\/plugins\/API\/Controller.php(46): Piwik\\API\\Request->process() #29 [internal function]: Piwik\\Plugins\\API\\Controller->index() #30 \/srv\/matomo\/core\/FrontController.php(619): call_user_func_array(Array, Array) #31 \/srv\/matomo\/core\/FrontController.php(168): Piwik\\FrontController->doDispatch('API', false, Array) #32 \/srv\/matomo\/core\/dispatch.php(32): Piwik\\FrontController->dispatch() #33 \/srv\/matomo\/index.php(25): require_once('\/srv\/matomo\/cor...') #34 \/srv\/matomo\/core\/CliMulti\/RequestCommand.php(79): require_once('\/srv\/matomo\/ind...') #35 \/srv\/matomo\/vendor\/symfony\/console\/Symfony\/Component\/Console\/Command\/Command.php(257): Piwik\\CliMulti\\RequestCommand->execute(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput)) #36 \/srv\/matomo\/vendor\/symfony\/console\/Symfony\/Component\/Console\/Application.php(874): Symfony\\Component\\Console\\Command\\Command->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput)) #37 \/srv\/matomo\/vendor\/symfony\/console\/Symfony\/Component\/Console\/Application.php(195): Symfony\\Component\\Console\\Application->doRunCommand(Object(Piwik\\CliMulti\\RequestCommand), Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput)) #38 [internal function]: Symfony\\Component\\Console\\Application->doRun(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput)) #39 \/srv\/matomo\/core\/Console.php(130): call_user_func(Array, Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput)) #40 \/srv\/matomo\/core\/Access.php(670): Piwik\\Console->Piwik\\{closure}() #41 \/srv\/matomo\/core\/Console.php(131): Piwik\\Access::doAsSuperUser(Object(Closure)) #42 \/srv\/matomo\/core\/Console.php(82): Piwik\\Console->doRunImpl(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput)) #43 \/srv\/matomo\/vendor\/symfony\/console\/Symfony\/Component\/Console\/ ... ut))\n#57 \/srv\/matomo\/console(32): Symfony\\Component\\Console\\Application->run()\n#58 {main}"}'

The column exists in the table, so guess it might be something with a missing table alias or similar.

tests/PHPUnit/Framework/TestRequest/Collection.php Outdated Show resolved Hide resolved
plugins/Goals/tests/System/TrackGoalsPagesPageTest.php Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Nov 23, 2021
@bx80 bx80 removed the Needs Review PRs that need a code review label Nov 23, 2021
@bx80
Copy link
Contributor Author

bx80 commented Nov 23, 2021

@sgiehl Looks like I managed to come up with a query in LogAggregator::queryConversionsByPageView() that worked in MariaDB but not in MySQL with strict mode. 🤦
I've committed a fix and tested the query on both MariaDB and MySQL, hopefully that sorts the error you encountered.

@bx80 bx80 added the Needs Review PRs that need a code review label Nov 26, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2021

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 4, 2021
@bx80 bx80 added the Needs Review PRs that need a code review label May 22, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Had a rough look through the code changes and left some comments. Once fixed, I'll do a last functional testing and merge if everything works as expected.

'period' => $periodName,
'date' => ($periodName === 'range' ? $date.','.$period->getDateEnd()->toString() : $date),
'idSite' => $idSite,
'idGoal' => $idGoal
Copy link
Member

Choose a reason for hiding this comment

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

I'd assume that this request might lag the segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgiehl It could yes, the Goals.get method is going to load the whole blob data table again which could be slow on large datasets. Since we're storing the total conversions for each goal and period in the numeric archives I think it would be more efficient to directly lookup the total we need. Goals.getConversions does this but has unfortunately been disabled on the API with the @ignore annotation for some reason.

I've committed a change where the conversion total is retrieved directly using the Archive::getNumeric method instead of an API call. This should be a lot faster with no API overhead and only a single value being queried instead of deserializing a datatable, applying filters, etc.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left another comment.
One more thing that came to my mind: Did you check how the archiving behaves if no goals are configured and ecommerce is disabled? Are any queries fired nevertheless and could maybe be avoided? 🤔

foreach ($goalIds as $idGoal => $g) {

$date = ($periodName === 'range' ? $date.','.$period->getDateEnd()->toString() : $date);
$archive = Archive::build($idSite, $periodName, $date);
Copy link
Member

Choose a reason for hiding this comment

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

This one actually lags the segment as well. I'm wondering if it might be easier to move that to the Actions API. So we fetch the goal totals there and then pass the fetched totals to the filter call. That way it's a lot easier to access the required archive parameters and we don't need to rely on the datatable metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, cleaner to have the goals totals injected into the filter than have the filter accessing archive data directly. I've committed an update, but I'm not sure that it will speed things up much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I don't think this is going to work, filterActionsDataTable can accept a DataTable\Map as well as a DataTable so the goal retrieval will need to happen at the filter level since we may be dealing with multiple different datatables. I'll revert the change.

@bx80
Copy link
Contributor Author

bx80 commented May 25, 2022

Did you check how the archiving behaves if no goals are configured and ecommerce is disabled? Are any queries fired nevertheless and could maybe be avoided?

An attempt to lookup goal conversion totals is only made for goals that are contained in the row metadata. So if there isn't any goal metadata then no queries will be made and the filter will not run at all.

This does mean looping the table checking the metadata on each row and putting the goal ids into an array, but this should be fast with in-memory data, the alternative would be to make another API call to get all the available goals for the site and then check totals for each of these goals even if they are not present in the table - I think this would be much slower, especially if the site has dozens of goals but the data table is only being viewed for a single goal.

@bx80 bx80 removed the Needs Review PRs that need a code review label May 25, 2022
@bx80
Copy link
Contributor Author

bx80 commented May 25, 2022

There are some failed tests I'll need to check.


// For each goal in the table retrieve the period total conversions and add to the array
foreach ($goalIds as $idGoal => $g) {
$archive = Archive::build($idSite, $period, $date);
Copy link
Member

Choose a reason for hiding this comment

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

Should that Archive::build include the segment? Otherwise we will compare segmented action data, with unsegmented conversion numbers. Or is that intended. If so it would be good to add a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should probably include the segment as we'll want to show the viewed before rate relative to the conversions that happened for the segment. I've included the segment in the archive::build call.

@bx80
Copy link
Contributor Author

bx80 commented May 31, 2022

The missing conversion rate formatting on the GoalsPages_individual_goal_updated.png UI test should be fixed once #19295 is merged into 4.x-dev

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

The tests are now fixed. Awesome to get this finally merged 🎉

@sgiehl sgiehl merged commit 70b004c into 4.x-dev May 31, 2022
@sgiehl sgiehl deleted the m-2030-goals-per-page branch May 31, 2022 17:08
@sgiehl sgiehl mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
4 participants