Improve page conversion attribution performance with pre-calculated field

[bx80]

Description:

Fixes #20375

• Adds a migration to create a new log_conversion.pageviews_before column (> 4.14.0 installs will already have this).
• New conversions will now write the number of pageviews in the visit prior to the conversion into this field.
• Adds a new console command core:calculate-conversion-pages to populate the new field for historic data.
• The migration will automatically attempt to populate the new field for conversions the last 48hrs, providing the host is not *.matomo.cloud and there are less than 10,000 conversions in that period.
• The pages / goals archiving query has been adjusted to use the new field instead of an expensive sub query.

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[sgiehl]

Left a couple of comments. Otherwise I was wondering if the current approach of simply replacing the archiving logic with a one based on the new column together with providing a command to recalculate the columns is suitable for us.

An alternative would be to keep the old possibility to calculate the values and decide whether to use the new or the old method after checking if the new column has values for all dates within the requested date range or not.
This would remove the requirements of having a command to update old datasets.

Using the current approach would otherwise mean that we would need to run the command for all cloud installs even if archives are already built. Otherwise we would have incorrect numbers after invalidating certain archives or if e.g. a new segment or custom report is created that triggers archiving for date ranges before the update.

@tsteur Which solution would you prefer?

[tsteur]

An alternative would be to keep the old possibility to calculate the values and decide whether to use the new or the old method after checking if the new column has values for all dates within the requested date range or not.
This would remove the requirements of having a command to update old datasets.

Using the current approach would otherwise mean that we would need to run the command for all cloud installs even if archives are already built. Otherwise we would have incorrect numbers after invalidating certain archives or if e.g. a new segment or custom report is created that triggers archiving for date ranges before the update.

On the cloud it shouldn't cause any issues unless someone invalidated historical data and this should be rare and is ok if then the data wouldn't show up correctly. All other data would be already archived using the old logic and the new logic would then start for newly tracked data. We may just have to populate the data for the last 24 hours to ensure the current day is correct. It would also apply to new segments/custom reports but we're only archiving 1 month back on Cloud so this wouldn't be too concerning and the report is potentially also not viewed too often. In the worst case we'd rather invalidate the data and re-archive it if someone asks.

I'm thinking checking all the column values be quite time consuming on the database and may take quite a bit of time. It would only apply for Cloud for invalidated data so it wouldn't be needed. On the Cloud we'd rather not have that extra query.

[github-actions]

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

[sgiehl]

@bx80 The system tests on PHP 7.2 are currently running into a segfault. I'm able to reproduce that segfault on my vm.
It seems caused by any change in this PR, as the tests are passing on 5.x-dev. I'll check which test is causing it so we can further investigate...

[sgiehl]

The segfault occurs within the test BackwardsCompatibility1XTest. That test insets a database dump of Matomo 1.x and then tries to perform a database update here:

matomo/tests/PHPUnit/System/BackwardsCompatibility1XTest.php

Line 44 in 065efa1

$result = Fixture::updateDatabase();

This segfaults when it tries to perform the custom migration you have added. Commenting that part out lets the tests pass again. Don't have time right now to investigate that further...

[bx80]

@sgiehl I was able to recreate the segfault locally with PHP 7.4.33, strangely it doesn''t occur at all with with PHP 8.2.5.

It seems to be caused by the custom migration using the plugins/Goals/Model/getActiveGoals() method, directly retrieving the goal ids with a simple query doesn't produce the segfault. I can't see any obvious reason why using that method would cause the error.

As part of debugging this I've also reworked the custom migration to more closely match the 4.0.0 custom migration by generating an array of BoundSql objects and then using the $updater->executeMigrations() method instead of executing the queries directly.

[sgiehl]

Left some more comments.
In relation to this PR we should maybe create a new FAQ to explain what to do if the page goal metrics aren't shown for older dates when e.g. a new segment is created. Running a command won't be something people might expect.
Not sure if that is something we might also need to mention somewhere in the release blog post.

[sgiehl]

Had a last look through the code and imho it looks fine now.
Added a last minor suggestions, which I will apply myself before merging it.