@MichaelHeerklotz opened this Pull Request on September 27th 2019 Contributor

Since some time the archive job crashes in my setup with

Trying to sum unsupported operands for column nb_conversions_stddev_samp in row with label = 35: NULL + string - in plugin AbTesting

What I found out after some debuging:

The AbTesting plugin causes sumRowArray($thisColumnValue, $columnToSumValue) to be called

a) with a float value in $thisColumnValue and an array in $columnToSumValue
In this case the if (!isset($newValue[$arrayIndex])) { check is triggered, however, because $newValue is a float, PHP will not convert $newValue to an array, and the $newValue[$arrayIndex] will pass a value of "null" to the recursive sumRowArray() call. This will trigger the above exception.

Because the documentation of sumRowArray() allows $columnToSumValue to be an array, this is most likely a bug.

b) with an array $thisColumnValue and a float value in $columnToSumValue
This case is not allowed by the function documentation, but maybe it makes sense to handle this case, too?

These changes should be reviewed with care, as I do not yet fully understand why the AbTesting plugin is causing these problems.

@diosmosis commented on September 30th 2019 Member

CC @tsteur as this relates to AbTesting

@tsteur commented on September 30th 2019 Member

The PR is likely fine to merge but need to look further into it. AFAIK from what I see for stddev_samp it should not go in there though as it should apply a custom aggregation function see in the Archiver class around line 363-383. I'll need to look into it a bit more though.

Maybe it changed which test is applied. Like it used to use a TTest because a goal allowed multiple conversions in one version and this is no longer the case. Then old reports would maybe still have that metric even though it's maybe no longer used.

@tsteur commented on September 30th 2019 Member

@MichaelHeerklotz

could you maybe try this patch in a/b testing instead?

diff --git a/Archiver.php b/Archiver.php
index d0cc250..67d5629 100644
--- a/Archiver.php
+++ b/Archiver.php
@@ -348,11 +348,7 @@ class Archiver extends \Piwik\Plugin\Archiver
         $ttestMetrics = array();

         foreach ($experimentMetrics as $metric) {
-            $bestStrategy = $this->strategy->getBestStrategyForMetric($metric, $experiment['idexperiment'], $experiment['idsite']);
-
-            if ($bestStrategy === Strategy::TTEST) {
-                $ttestMetrics[] = $metric;
-            }
+            $ttestMetrics[] = $metric;
         }

         $unique = $this->aggregator->getUniqueVisitors($experiment, $onlyEntered = false);

I've been looking at this PR for quite a while and not sure if we should merge or not. In theory it looks good. Bit scared it may sum wrong values when there is eg a zero index in columnToSumValue which refers to something completely else in https://github.com/matomo-org/matomo/pull/14917/files#diff-83d3b778656df9b44f52c78fd88cb56dR634. If there's some wrong data summed it may be very hard to find the cause for that. Ideally when there are arrays we maybe try to fix this in the archivers and API. At the same time this might not always be possible or not trivial. Especially when changing reports over time etc...

@mattab commented on October 20th 2019 Member

Hi @MichaelHeerklotz - Thanks for creating the PR.
Could you maybe try the patch suggested in the comment above and confirm if it fixes the issue for you?

@MichaelHeerklotz commented on November 4th 2019 Contributor

Hello @tsteur and @mattab, after applying the patch I have not seen this error for some weeks now, so the problem seems to be fixed

@tsteur commented on November 4th 2019 Member

@MichaelHeerklotz thanks for confirming. We will release an update for A/B Testing and I reckon we can close this PR for now?

This Pull Request was closed on November 4th 2019
Powered by GitHub Issue Mirror