Performance improvements for goals by pages

[bx80]

Description:

This is a rework of #18221 to improve performance and decrease memory usage when archiving.

Changes:

• Aggregation and attribution is no longer done in code and is performed as part of the database query.
• Queries to retrieve goal metric data for pages are now run individually for each goal to decrease peak load on the database server and to reduce memory usage when enriching the actions array in code.
• Page URL and page titles unit tests have been expanded to cover multiple days

Refs: L3-313

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

[justinvelluppillai]

@bx80 is this still ready for review or are you addressing @sgiehl 's previous comment still?

[bx80]

@justinvelluppillai I'm currently looking at a solution for supporting segments with the custom query. We definitely don't want to duplicate the segment query building logic, so it's probably a case of either improving the generateQuery method so it can build the complicated custom goals pages query or at least support having a pre-built full query passed to it and then apply the segment wrapping.

[justinvelluppillai]

ok thanks, I've removed the Needs review label meanwhile 👍🏽

[justinvelluppillai]

@bx80 @tsteur mentioned a few times that he's done something similar with multi attribution so maybe worth talking to him about this

[bx80]

Thanks for the feedback @sgiehl 🙂

I've applied all the suggested improvements. For large dataset performance the query was tested on a real world database where a single goal had 500k+ conversions in a single day. The new query now runs for each goal individually and it took just under 5minutes to complete archiving for this goal, whereas previously it failed to complete goal actions archiving at all. There are more details about the performance testing in the comments for L3-313

[sgiehl]

Left another comment. Otherwise this looks fine.

[tsteur]

Nice, that looks really good @bx80 . Great this got a lot simpler now and that we'll have this query use less memory soon. I didn't do an in-depth review but looks overall all good. Only left a few minor comments to consider.

[tsteur]

I haven't tested it @bx80 but looks good 👍

[bx80]

@sgiehl When you have a chance, could you please give this a final check and merge it if there are no outstanding issues? Thanks!

[sgiehl]

Left a few minor comments. Otherwise the code looks fine. Archiving seems to work as expected, even for segments.
@bx80 Did you check if the results of the tests have the correct numbers? As you change the existing test fixtures all numbers changed, which makes it hard to verify if they are "still" correct. I tried checking out the old test files, but using the new code for running the tests. The resulting files then differed slightly. Not sure if that is correct or not...

[bx80]

@sgiehl Thanks for reviewing this one, here is the spreadsheet I created to check the test results: m-2030-Goals-per-page-test-calcs.ods
It's just a set of calculations based on the visitor actions in the fixture to show the expected values for each page. I've done a comparison of the test results against the spreadsheet and from what I can see they match. It can be tricky to compare when the test results are for a single day but the spreadsheet is for the entire test period, which is why I changed some of the tests to cover a whole week period.

[sgiehl]

Had a look at the test calculations. For me that looks valid. Still not 100% sure why some other tests changed their results slightly, but maybe there was a problem before, that is now fixed?

[bx80]

Still not 100% sure why some other tests changed their results slightly, but maybe there was a problem before, that is now fixed?

I think that was probably the case. 👍 The improved tests should hopefully make it easier to check that the expected results are correct.