Avoid any archiving queries in Goals Archiver if no goals or ecommerce are active #18354

sgiehl · 2021-11-19T09:15:30Z

Description:

Note: The test changes are kind of expected. We had a couple of fixtures where ecommerce data was tracked, even though it actually wasn't enabled for that site.

For some Fixtures I have activate the ecommerce flag, while for others I kept it off on purpose. That way we can see that there are null records archived, even though some data would actually be tracked and available.

Review

tsteur

@sgiehl I suppose it won't cause any issue if we don't implement similar logic in aggregate archives? I had this done initially in https://github.com/matomo-org/matomo/compare/improvegoalarchive?expand=1 but maybe it keeps code simpler if we don't do it like it did. Just not entirely sure if there could be downsides like if you request certain days and it doesn't find that archive, whether it would maybe try to archive it. But I think it won't as long as there is a done flag there. Just double checking.

The biggest benefit of this issue is to not having to go into $this->getProcessor()->processDependentArchive when there is no goals and archive. I think that's currently still the case. Could we ideally not call it when there are no goals and no ecommerce?

This saves us having to create a temporary table for the new_visit and returning visit segment and not having to fetch these visitors (which can be very slow)

plugins/Goals/Archiver.php

sgiehl · 2021-11-22T09:51:06Z

@tsteur won't $this->getProcessor()->processDependentArchive also (only) call the goals archiver, but with a segment? It should skip doing any queries in that case as well, but create empty archives. Thought it would be good to have empty ones there as well, to ensure they won't be created later maybe.

tsteur · 2021-11-23T00:14:29Z

@sgiehl AFAIK it would still try to resolve the segment because every time we process dependent archives it's also trying to resolve the number of visits we had for that segment even before we go into the goals archiver

tsteur · 2021-11-23T00:15:00Z

The only way would be to initialise a new archive manually for that segment and writing empty entries and finishing the archive without even going in there

sgiehl · 2021-11-23T13:15:36Z

Haven't yet debugged that through, but won't those "VisitsSummary" numbers be archived by the VisitFrequency archiver nevertheless. So it should at least be no additional "work" to do 🤔

tsteur · 2021-11-23T19:12:50Z

It's something general the archiver does for any kind of archive. Even if it was a say customreports specific archive. I'm not sure if we can check for any other archives there as they may be outdated or be invalidated soon after this or something. Also would partially depend on the order in which plugins archive in the same archive etc.

sgiehl · 2021-11-23T19:38:39Z

@tsteur added some code to skip the dependent archives if there are no goals and ecommerce isn't used. I guess that's the simplest solution for now even though that might mean that those archives might be archived later when that changes.

tsteur · 2021-11-24T03:21:38Z

@sgiehl we might need to do the same for the processDependentArchive calls in aggregateMultipleReports? I'm not entirely sure what Matomo does here but usual behaviour when Matomo tries to aggregate data, and it doesn't have the data for the subperiods, then it would launch the archiving for these subperiods. Aka it might try to launch the day archive for each dependent archive. I haven't double checked but wanted to check if you tested that case maybe?

sgiehl · 2021-11-24T13:50:02Z

@tsteur Haven't tested that. But as there won't be a done flag for that segment, I'm pretty sure the day periods would then be processed. Just pushed an update so it will be skipped as well

sgiehl · 2021-11-24T14:54:49Z

tests/PHPUnit/Integration/ArchiveProcessor/LoaderTest.php

@@ -181,28 +177,12 @@ public function test_pluginOnlyArchivingDoesNotRelaunchChildArchives_whenReusing
            ],
            [
                'idarchive' => '3',


The changes to this files are caused by the fact, that the dependent archives are no longer been processed when no goals are available and ecommerce is disabled. Therefor the Goals archives (for segments) disappeared. The VisitsSummary archives are still present, as they are archived by the VisitFrequency archiver.

sgiehl · 2021-12-21T10:47:13Z

@tsteur I tried working further on that. But actually I did not yet find a solution that would really work.
What we actually tried to avoid is processing the VisitsSummary metrics when the dependent segment metrics for Goals are processed, when it is actually not needed.
Tried inserting an empty archive (so there is a done flag for goals), as well as changing the code, so VisitsSummary won't be processed at all if only one plugin is requested. In theory that works. Running the archiving only creates archives and done flags for the Goals plugin. The VisitsSummary metrics aren't archived, at least not in the Goals plugin.
The VisitFrequency plugin will always archive the VisitsSummary metrics from the same segments. So that will be done in most cases nevertheless.
But that actually does not really solve the problem. Because as soon as the report Goals.get is requested all those VisitsSummary archives would be processed, which is caused by:

matomo/plugins/Goals/API.php

Lines 472 to 500 in d5997bd

    
           $segments = array( 
        
               '' => false, 
        
               '_new_visit' => VisitFrequencyAPI::NEW_VISITOR_SEGMENT, 
        
               '_returning_visit' => VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT 
        
           ); 
        
           foreach ($segments as $appendToMetricName => $predefinedSegment) { 
        
               if (!empty($predefinedSegment)) { 
        
                   // we are disabling the archiving of these segments as the archiver archives them already using 
        
                   // archiveProcessDependend logic. Otherwise we would eg archive reports that we don't need: 
        
                   // userid=5;visitorType%3D%3Dnew;visitorType%3D%3Dreturning%2CvisitorType%3D%3DreturningCustomer 
        
                   // userid=5;visitorType%3D%3Dreturning%2CvisitorType%3D%3DreturningCustomer;visitorType%3D%3Dnew; 
        
                   // it would also archive dependends for these segments that we already combined here and then combine 
        
                   // segments again when archiving dependends 
        
                   Archiver::$ARCHIVE_DEPENDENT = false; 
        
               } 
        
               $segmentToUse = $this->appendSegment($segment, $predefinedSegment); 
        
               /** @var DataTable|DataTable\Map $tableSegmented */ 
        
               $tableSegmented = Request::processRequest('Goals.getMetrics', array( 
        
                   'segment' => $segmentToUse, 
        
                   'idSite'  => $idSite, 
        
                   'period'  => $period, 
        
                   'date'    => $date, 
        
                   'idGoal'  => $idGoal, 
        
                   'columns' => $columns, 
        
                   'showAllGoalSpecificMetrics' => $showAllGoalSpecificMetrics, 
        
                   'format_metrics' => !empty($compare) ? 0 : Common::getRequestVar('format_metrics', 'bc'), 
        
               ), $default = []);

The getMetrics API tries to fetch the numeric record for nb_visits, which is a VisitsSummary metric...

The only useful solution I still would see is to process the dependent archives for goals in all cases (like it was in the beginning). The day archiver inserts empty archives and skips all queries. That for sure will also cause the core metrics being processed (which VisitsFrequency would do nevertheless).

~~Nevertheless I've tried to do some other changes that might prevent that the requested plugin is lost during the archiving process.~~ Had to revert that, as it broke almost all tests.

tsteur · 2021-12-22T02:34:36Z

👍 we can keep it basically to as before. The only other thing would be that if no goals and no ecommerce is configured, to manually create these archives

Not sure if it's worth looking at this or not? I'm generally thinking it could make archiving quite a bit faster when we don't need to do the processDependentArchives as segments are always a bit slow to resolve to create this temporary table etc. But it's also not the end of the world if we just keep it now as is in the PR and always process the dependent archives.

sgiehl · 2021-12-22T08:32:59Z

@tsteur I tried that, but it led to strange behavior when the VisitFrequency plugin was disabled.
Running the normal archiving created an empty done flag for Goals only. So far so good, but once the report Goals.get was requested an additional VisitsSummary done flag for those segment appeared, without having any additional data. Didn't debug that through, but guess that might cause other issues if that segment would be used later for other purpose, as the core metrics might stay empty.

tsteur

Makes sense. We wouldn't want to create any such archive record for the VisitsSummary plugin either as we would be writing 0 entries probably when there could be visits etc.

Left only one comment to look at. Not sure we need to do anything there. Then good to merge 👍

tsteur · 2021-12-22T20:03:31Z

plugins/Goals/Archiver.php


        /*
         *  Archive General Goal metrics
         */
        $goalIdsToSum = GoalManager::getGoalIds($this->getProcessor()->getParams()->getSite()->getId());

        //Ecommerce
-        $goalIdsToSum[] = GoalManager::IDGOAL_ORDER;
-        $goalIdsToSum[] = GoalManager::IDGOAL_CART; //bug here if idgoal=1
+        $goalIdsToSum = array_merge($goalIdsToSum, $this->getEcommerceIdGoals());


should we only add the ecommerceGoalIds if ecommerce is enabled? and then $this->getProcessor()->aggregateNumericMetrics($fieldsToSum); we could run only if $fieldsToSum is not empty? $this->getProcessor()->aggregateNumericMetrics($fieldsToSum);

I have added a check for ecommerce plugin. But the $goalIdsToSum array actually always contains the entry false, for processing the overview. Not sure if we should skip that one if no other goal is processed or not.

Guess we could always keep the false 👍

github-actions · 2021-12-30T01:48:48Z

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

github-actions · 2022-01-06T01:54:55Z

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

sgiehl · 2022-01-11T12:22:56Z

@tsteur I've updated the tests. From my side everything should be done. Let me know if we can merge or if anything else needs to be adjusted.

tsteur · 2022-01-11T19:16:51Z

sounds good to merge @sgiehl 👍

sgiehl added 2 commits November 18, 2021 16:28

Avoid querying data in Goals Archiver if feature is actually unused

f28fdba

update test fixture

6c7019c

sgiehl added this to the 4.7.0 milestone Nov 19, 2021

update expected test files that changed due to activating ecommerce

54bef43

sgiehl force-pushed the skipgoalarchiving branch from 03368c4 to 54bef43 Compare November 19, 2021 10:08

sgiehl added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Nov 19, 2021

sgiehl marked this pull request as ready for review November 19, 2021 10:27

deactivate ecommerce in tests

fde393d

sgiehl force-pushed the skipgoalarchiving branch from 31ed8cf to fde393d Compare November 19, 2021 11:47

updates expected ui files

2ae7ddc

tsteur requested changes Nov 22, 2021

View reviewed changes

plugins/Goals/Archiver.php Show resolved Hide resolved

sgiehl added 3 commits November 22, 2021 10:51

Merge branch '4.x-dev' into skipgoalarchiving

c029539

Merge branch '4.x-dev' into skipgoalarchiving

306170b

Merge branch '4.x-dev' into skipgoalarchiving

9dd0901

tsteur mentioned this pull request Nov 23, 2021

Dependent archives never processed for All visits segment causing some data to not show #18362

Closed

sgiehl added 2 commits November 23, 2021 20:36

do not process dependent archives if no goals or ecommerce is active

819bc84

Merge branch '4.x-dev' into skipgoalarchiving

77c75ae

apply review feedback

78ea524

sgiehl added 2 commits November 24, 2021 14:59

don't aggregate ecommerce items if plugin is not active

72e3f4e

updates tests

7540b6d

sgiehl commented Nov 24, 2021

View reviewed changes

github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 7, 2021

sgiehl added Do not close PRs with this label won't be marked as stale by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Dec 7, 2021

Merge branch '4.x-dev' into skipgoalarchiving

bb97183

sgiehl force-pushed the skipgoalarchiving branch from 71cb056 to bb97183 Compare December 14, 2021 20:13

sgiehl added 2 commits December 20, 2021 14:41

Merge branch '4.x-dev' into skipgoalarchiving

74a6e78

always process dependent archives

5ad92ab

sgiehl force-pushed the skipgoalarchiving branch from 3847e3c to 5ad92ab Compare December 21, 2021 12:22

sgiehl added 4 commits December 21, 2021 14:28

fix test

bf8c295

Merge branch '4.x-dev' into skipgoalarchiving

6d3676a

fix test

4916ebf

Merge branch '4.x-dev' into skipgoalarchiving

63bcf0c

tsteur reviewed Dec 22, 2021

View reviewed changes

github-actions bot added Stale The label used by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Dec 30, 2021

github-actions bot added Stale The label used by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Jan 6, 2022

sgiehl added 4 commits January 10, 2022 17:37

Merge branch '4.x-dev' into skipgoalarchiving

46019e3

process ecommerce metrics only when plugin enabled

62471a9

updates expected test files

fbb2d65

updates expected UI test files

eba7616

sgiehl merged commit 0e5a50e into 4.x-dev Jan 11, 2022

sgiehl deleted the skipgoalarchiving branch January 11, 2022 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid any archiving queries in Goals Archiver if no goals or ecommerce are active #18354

Avoid any archiving queries in Goals Archiver if no goals or ecommerce are active #18354

sgiehl commented Nov 19, 2021 •

edited

tsteur left a comment

sgiehl commented Nov 22, 2021

tsteur commented Nov 23, 2021

tsteur commented Nov 23, 2021

sgiehl commented Nov 23, 2021

tsteur commented Nov 23, 2021

sgiehl commented Nov 23, 2021

tsteur commented Nov 24, 2021

sgiehl commented Nov 24, 2021

sgiehl Nov 24, 2021

sgiehl commented Dec 21, 2021 •

edited

tsteur commented Dec 22, 2021

sgiehl commented Dec 22, 2021

tsteur left a comment

tsteur Dec 22, 2021

sgiehl Jan 10, 2022

tsteur Jan 11, 2022

github-actions bot commented Dec 30, 2021

github-actions bot commented Jan 6, 2022

sgiehl commented Jan 11, 2022

tsteur commented Jan 11, 2022

Avoid any archiving queries in Goals Archiver if no goals or ecommerce are active #18354

Avoid any archiving queries in Goals Archiver if no goals or ecommerce are active #18354

Conversation

sgiehl commented Nov 19, 2021 • edited

Description:

Review

tsteur left a comment

Choose a reason for hiding this comment

sgiehl commented Nov 22, 2021

tsteur commented Nov 23, 2021

tsteur commented Nov 23, 2021

sgiehl commented Nov 23, 2021

tsteur commented Nov 23, 2021

sgiehl commented Nov 23, 2021

tsteur commented Nov 24, 2021

sgiehl commented Nov 24, 2021

sgiehl Nov 24, 2021

Choose a reason for hiding this comment

sgiehl commented Dec 21, 2021 • edited

tsteur commented Dec 22, 2021

sgiehl commented Dec 22, 2021

tsteur left a comment

Choose a reason for hiding this comment

tsteur Dec 22, 2021

Choose a reason for hiding this comment

sgiehl Jan 10, 2022

Choose a reason for hiding this comment

tsteur Jan 11, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 30, 2021

github-actions bot commented Jan 6, 2022

sgiehl commented Jan 11, 2022

tsteur commented Jan 11, 2022

sgiehl commented Nov 19, 2021 •

edited

sgiehl commented Dec 21, 2021 •

edited