Skip to content

Commit

Permalink
Fix aggregation of additional action metrics (#22348)
Browse files Browse the repository at this point in the history 
* Correctly set column aggegation methods for action reports

* don't provide a max value if no values are available at all

* update expected test files

* update submodule

* Fix min/max checks

* Ensure to store null for min/max event value metric if no values are available

* don't use numeric check to not break percentage values

* Updates expected test files

* Ensure correct aggregation of min/max event value metrics

* apply review feedback
  • Loading branch information
@sgiehl
sgiehl authored Jul 15, 2024
1 parent 97efb82 commit 5863dc9
Show file tree
Hide file tree
Showing 131 changed files with 4,419 additions and 4,387 deletions.
17 changes: 14 additions & 3 deletions core/DataTable/Row.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,12 +521,18 @@ private function getColumnValuesMerged($operation, $thisColumnValue, $columnToSu
$newValue = null;
break;
case 'max':
$newValue = max($thisColumnValue, $columnToSumValue);
if ($this->isValueConsideredEmpty($thisColumnValue)) {
$newValue = $columnToSumValue;
} elseif ($this->isValueConsideredEmpty($columnToSumValue)) {
$newValue = $thisColumnValue;
} else {
$newValue = max($thisColumnValue, $columnToSumValue);
}
break;
case 'min':
if (!$thisColumnValue) {
if ($this->isValueConsideredEmpty($thisColumnValue)) {
$newValue = $columnToSumValue;
} elseif (!$columnToSumValue) {
} elseif ($this->isValueConsideredEmpty($columnToSumValue)) {
$newValue = $thisColumnValue;
} else {
$newValue = min($thisColumnValue, $columnToSumValue);
Expand Down Expand Up @@ -558,6 +564,11 @@ private function getColumnValuesMerged($operation, $thisColumnValue, $columnToSu
return $newValue;
}

private function isValueConsideredEmpty($value): bool
{
return in_array($value, [null, false, ''], true);
}

/**
* Sums the metadata in `$rowToSum` with the metadata in `$this` row.
*
Expand Down
4 changes: 3 additions & 1 deletion plugins/Actions/API.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,11 +513,13 @@ protected function doFilterPageDatatableSearch($callBackParameters, $table, $sea
*/
private function filterActionsDataTable($dataTable, $isPageTitleType)
{
$dataTable->filter(function ($dataTable) {
$dataTable->setMetadata(DataTable::COLUMN_AGGREGATION_OPS_METADATA_NAME, Metrics::getColumnsAggregationOperation());
});
// Must be applied before Sort in this case, since the DataTable can contain both int and strings indexes
// (in the transition period between pre 1.2 and post 1.2 datatable structure)
$dataTable->filter('Piwik\Plugins\Actions\DataTable\Filter\Actions', [$isPageTitleType]);
$dataTable->filter('Piwik\Plugins\Goals\DataTable\Filter\CalculateConversionPageRate');

return $dataTable;
}

Expand Down
17 changes: 13 additions & 4 deletions plugins/Actions/Metrics.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,19 @@ class Metrics
PiwikMetrics::INDEX_PAGE_EXIT_NB_UNIQ_VISITORS,
);

public static $columnsAggregationOperation = array(
PiwikMetrics::INDEX_PAGE_MAX_TIME_GENERATION => 'max',
PiwikMetrics::INDEX_PAGE_MIN_TIME_GENERATION => 'min'
);
public static function getColumnsAggregationOperation()
{
$operations = [];
$actionMetrics = self::getActionMetrics();

foreach ($actionMetrics as $actionMetric => $definition) {
if (!empty($definition['aggregation']) && $definition['aggregation'] !== 'sum') {
$operations[$actionMetric] = $definition['aggregation'];
}
}

return $operations;
}

public static function getActionMetrics()
{
Expand Down
6 changes: 3 additions & 3 deletions plugins/Actions/RecordBuilders/ActionReports.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ public function getRecordMetadata(ArchiveProcessor $archiveProcessor): array
->setMaxRowsInTable(ArchivingHelper::$maximumRowsInDataTableSiteSearch),

Record::make(Record::TYPE_BLOB, Archiver::PAGE_URLS_RECORD_NAME)
->setBlobColumnAggregationOps(Metrics::$columnsAggregationOperation),
->setBlobColumnAggregationOps(Metrics::getColumnsAggregationOperation()),
Record::make(Record::TYPE_BLOB, Archiver::PAGE_TITLES_RECORD_NAME)
->setBlobColumnAggregationOps(Metrics::$columnsAggregationOperation),
->setBlobColumnAggregationOps(Metrics::getColumnsAggregationOperation()),

Record::make(Record::TYPE_BLOB, Archiver::DOWNLOADS_RECORD_NAME),
Record::make(Record::TYPE_BLOB, Archiver::OUTLINKS_RECORD_NAME),
Expand Down Expand Up @@ -184,7 +184,7 @@ private function makeReportTables(): array
|| $type == Action::TYPE_PAGE_TITLE
) {
// for page urls and page titles, performance metrics exist and have to be aggregated correctly
$dataTable->setMetadata(DataTable::COLUMN_AGGREGATION_OPS_METADATA_NAME, Metrics::$columnsAggregationOperation);
$dataTable->setMetadata(DataTable::COLUMN_AGGREGATION_OPS_METADATA_NAME, Metrics::getColumnsAggregationOperation());
}

$result[$type] = $dataTable;
Expand Down
2 changes: 1 addition & 1 deletion plugins/Bandwidth
Submodule Bandwidth updated 1 files
+2 −2 tests/Integration/BandwidthTest.php
24 changes: 12 additions & 12 deletions plugins/Contents/tests/System/expected/test_Contents__Actions.getPageUrls_month.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,23 @@
<nb_hits>2</nb_hits>
<sum_time_spent>540</sum_time_spent>
<nb_hits_with_time_network>2</nb_hits_with_time_network>
<min_time_network>0.033</min_time_network>
<max_time_network>0.033</max_time_network>
<min_time_network>0.0330</min_time_network>
<max_time_network>0.0330</max_time_network>
<nb_hits_with_time_server>2</nb_hits_with_time_server>
<min_time_server>0.325</min_time_server>
<max_time_server>0.325</max_time_server>
<min_time_server>0.3250</min_time_server>
<max_time_server>0.3250</max_time_server>
<nb_hits_with_time_transfer>2</nb_hits_with_time_transfer>
<min_time_transfer>0.124</min_time_transfer>
<max_time_transfer>0.124</max_time_transfer>
<min_time_transfer>0.1240</min_time_transfer>
<max_time_transfer>0.1240</max_time_transfer>
<nb_hits_with_time_dom_processing>2</nb_hits_with_time_dom_processing>
<min_time_dom_processing>0.356</min_time_dom_processing>
<max_time_dom_processing>0.356</max_time_dom_processing>
<min_time_dom_processing>0.3560</min_time_dom_processing>
<max_time_dom_processing>0.3560</max_time_dom_processing>
<nb_hits_with_time_dom_completion>2</nb_hits_with_time_dom_completion>
<min_time_dom_completion>0.215</min_time_dom_completion>
<max_time_dom_completion>0.215</max_time_dom_completion>
<min_time_dom_completion>0.2150</min_time_dom_completion>
<max_time_dom_completion>0.2150</max_time_dom_completion>
<nb_hits_with_time_on_load>2</nb_hits_with_time_on_load>
<min_time_on_load>0.099</min_time_on_load>
<max_time_on_load>0.099</max_time_on_load>
<min_time_on_load>0.0990</min_time_on_load>
<max_time_on_load>0.0990</max_time_on_load>
<sum_bandwidth>0</sum_bandwidth>
<nb_hits_with_bandwidth>0</nb_hits_with_bandwidth>
<min_bandwidth />
Expand Down
9 changes: 9 additions & 0 deletions plugins/Events/API.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
namespace Piwik\Plugins\Events;

use Piwik\Archive;
use Piwik\DataTable;
use Piwik\Metrics;
use Piwik\Piwik;

/**
Expand Down Expand Up @@ -154,6 +156,13 @@ protected function getDataTable($name, $idSite, $period, $date, $segment, $expan

$dataTable = Archive::createDataTableFromArchive($recordName, $idSite, $period, $date, $segment, $expanded, $flat, $idSubtable);

$dataTable->filter(function ($dataTable) {
$dataTable->setMetadata(DataTable::COLUMN_AGGREGATION_OPS_METADATA_NAME, [
Metrics::INDEX_EVENT_MIN_EVENT_VALUE => 'min',
Metrics::INDEX_EVENT_MAX_EVENT_VALUE => 'max',
]);
});

if ($flat) {
$dataTable->filterSubtables('Piwik\Plugins\Events\DataTable\Filter\ReplaceEventNameNotSet');
} else {
Expand Down
7 changes: 4 additions & 3 deletions plugins/Events/RecordBuilders/EventReports.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ public function getRecordMetadata(ArchiveProcessor $archiveProcessor): array

foreach ($records as $record) {
$record->setMaxRowsInTable($maximumRowsInDataTable)
->setMaxRowsInSubtable($maximumRowsInSubDataTable);
->setMaxRowsInSubtable($maximumRowsInSubDataTable)
->setBlobColumnAggregationOps($this->columnAggregationOps);
}

return $records;
Expand Down Expand Up @@ -198,8 +199,8 @@ protected function aggregateEventRow(array &$reports, array $row): void
Metrics::INDEX_EVENT_NB_HITS => $row[Metrics::INDEX_EVENT_NB_HITS] ?? 0,
Metrics::INDEX_EVENT_NB_HITS_WITH_VALUE => $row[Metrics::INDEX_EVENT_NB_HITS_WITH_VALUE] ?? 0,
Metrics::INDEX_EVENT_SUM_EVENT_VALUE => round($row[Metrics::INDEX_EVENT_SUM_EVENT_VALUE] ?? 0, 2),
Metrics::INDEX_EVENT_MIN_EVENT_VALUE => round($row[Metrics::INDEX_EVENT_MIN_EVENT_VALUE] ?? false, 2),
Metrics::INDEX_EVENT_MAX_EVENT_VALUE => round($row[Metrics::INDEX_EVENT_MAX_EVENT_VALUE] ?? 0, 2),
Metrics::INDEX_EVENT_MIN_EVENT_VALUE => is_numeric($row[Metrics::INDEX_EVENT_MIN_EVENT_VALUE]) ? round($row[Metrics::INDEX_EVENT_MIN_EVENT_VALUE], 2) : null,
Metrics::INDEX_EVENT_MAX_EVENT_VALUE => is_numeric($row[Metrics::INDEX_EVENT_MAX_EVENT_VALUE]) ? round($row[Metrics::INDEX_EVENT_MAX_EVENT_VALUE], 2) : null,
];

$topLevelRow = $table->sumRowWithLabel($mainLabel, $columns, $this->columnAggregationOps);
Expand Down
48 changes: 24 additions & 24 deletions plugins/PagePerformance/tests/System/expected/test___Actions.getPageTitles_month.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,23 @@
<nb_hits>10</nb_hits>
<sum_time_spent>0</sum_time_spent>
<nb_hits_with_time_network>10</nb_hits_with_time_network>
<min_time_network>0.094</min_time_network>
<max_time_network>0.276</max_time_network>
<min_time_network>0.0000</min_time_network>
<max_time_network>0.0980</max_time_network>
<nb_hits_with_time_server>10</nb_hits_with_time_server>
<min_time_server>0.496</min_time_server>
<max_time_server>0.801</max_time_server>
<min_time_server>0.0770</min_time_server>
<max_time_server>0.2690</max_time_server>
<nb_hits_with_time_transfer>10</nb_hits_with_time_transfer>
<min_time_transfer>1.268</min_time_transfer>
<max_time_transfer>1.683</max_time_transfer>
<min_time_transfer>0.1950</min_time_transfer>
<max_time_transfer>0.4120</max_time_transfer>
<nb_hits_with_time_dom_processing>10</nb_hits_with_time_dom_processing>
<min_time_dom_processing>4.989</min_time_dom_processing>
<max_time_dom_processing>5.678</max_time_dom_processing>
<min_time_dom_processing>0.8990</min_time_dom_processing>
<max_time_dom_processing>1.3000</max_time_dom_processing>
<nb_hits_with_time_dom_completion>10</nb_hits_with_time_dom_completion>
<min_time_dom_completion>1.464</min_time_dom_completion>
<max_time_dom_completion>1.683</max_time_dom_completion>
<min_time_dom_completion>0.1950</min_time_dom_completion>
<max_time_dom_completion>0.4440</max_time_dom_completion>
<nb_hits_with_time_on_load>10</nb_hits_with_time_on_load>
<min_time_on_load>0.498</min_time_on_load>
<max_time_on_load>1.126</max_time_on_load>
<min_time_on_load>0.0660</min_time_on_load>
<max_time_on_load>0.2580</max_time_on_load>
<sum_bandwidth>0</sum_bandwidth>
<nb_hits_with_bandwidth>0</nb_hits_with_bandwidth>
<min_bandwidth />
Expand All @@ -49,23 +49,23 @@
<nb_hits>5</nb_hits>
<sum_time_spent>0</sum_time_spent>
<nb_hits_with_time_network>5</nb_hits_with_time_network>
<min_time_network>0.132</min_time_network>
<max_time_network>0.132</max_time_network>
<min_time_network>0.0120</min_time_network>
<max_time_network>0.0660</max_time_network>
<nb_hits_with_time_server>5</nb_hits_with_time_server>
<min_time_server>1.162</min_time_server>
<max_time_server>1.162</max_time_server>
<min_time_server>0.1500</min_time_server>
<max_time_server>0.3550</max_time_server>
<nb_hits_with_time_transfer>5</nb_hits_with_time_transfer>
<min_time_transfer>1.512</min_time_transfer>
<max_time_transfer>1.512</max_time_transfer>
<min_time_transfer>0.1360</min_time_transfer>
<max_time_transfer>0.4440</max_time_transfer>
<nb_hits_with_time_dom_processing>5</nb_hits_with_time_dom_processing>
<min_time_dom_processing>5.549</min_time_dom_processing>
<max_time_dom_processing>5.549</max_time_dom_processing>
<min_time_dom_processing>0.8880</min_time_dom_processing>
<max_time_dom_processing>1.3000</max_time_dom_processing>
<nb_hits_with_time_dom_completion>5</nb_hits_with_time_dom_completion>
<min_time_dom_completion>1.975</min_time_dom_completion>
<max_time_dom_completion>1.975</max_time_dom_completion>
<min_time_dom_completion>0.2990</min_time_dom_completion>
<max_time_dom_completion>0.5120</max_time_dom_completion>
<nb_hits_with_time_on_load>5</nb_hits_with_time_on_load>
<min_time_on_load>1.129</min_time_on_load>
<max_time_on_load>1.129</max_time_on_load>
<min_time_on_load>0.0990</min_time_on_load>
<max_time_on_load>0.3330</max_time_on_load>
<sum_bandwidth>0</sum_bandwidth>
<nb_hits_with_bandwidth>0</nb_hits_with_bandwidth>
<min_bandwidth />
Expand Down
48 changes: 24 additions & 24 deletions plugins/PagePerformance/tests/System/expected/test___Actions.getPageUrls_day.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,23 @@
<nb_hits>1</nb_hits>
<sum_time_spent>0</sum_time_spent>
<nb_hits_with_time_network>1</nb_hits_with_time_network>
<min_time_network>0.012</min_time_network>
<max_time_network>0.012</max_time_network>
<min_time_network>0.0120</min_time_network>
<max_time_network>0.0120</max_time_network>
<nb_hits_with_time_server>1</nb_hits_with_time_server>
<min_time_server>0.15</min_time_server>
<max_time_server>0.15</max_time_server>
<min_time_server>0.1500</min_time_server>
<max_time_server>0.1500</max_time_server>
<nb_hits_with_time_transfer>1</nb_hits_with_time_transfer>
<min_time_transfer>0.333</min_time_transfer>
<max_time_transfer>0.333</max_time_transfer>
<min_time_transfer>0.3330</min_time_transfer>
<max_time_transfer>0.3330</max_time_transfer>
<nb_hits_with_time_dom_processing>1</nb_hits_with_time_dom_processing>
<min_time_dom_processing>1.101</min_time_dom_processing>
<max_time_dom_processing>1.101</max_time_dom_processing>
<min_time_dom_processing>1.1010</min_time_dom_processing>
<max_time_dom_processing>1.1010</max_time_dom_processing>
<nb_hits_with_time_dom_completion>1</nb_hits_with_time_dom_completion>
<min_time_dom_completion>0.369</min_time_dom_completion>
<max_time_dom_completion>0.369</max_time_dom_completion>
<min_time_dom_completion>0.3690</min_time_dom_completion>
<max_time_dom_completion>0.3690</max_time_dom_completion>
<nb_hits_with_time_on_load>1</nb_hits_with_time_on_load>
<min_time_on_load>0.15</min_time_on_load>
<max_time_on_load>0.15</max_time_on_load>
<min_time_on_load>0.1500</min_time_on_load>
<max_time_on_load>0.1500</max_time_on_load>
<sum_bandwidth>0</sum_bandwidth>
<nb_hits_with_bandwidth>0</nb_hits_with_bandwidth>
<min_bandwidth />
Expand Down Expand Up @@ -133,23 +133,23 @@
<nb_hits>1</nb_hits>
<sum_time_spent>0</sum_time_spent>
<nb_hits_with_time_network>1</nb_hits_with_time_network>
<min_time_network>0.036</min_time_network>
<max_time_network>0.036</max_time_network>
<min_time_network>0.0360</min_time_network>
<max_time_network>0.0360</max_time_network>
<nb_hits_with_time_server>1</nb_hits_with_time_server>
<min_time_server>0.077</min_time_server>
<max_time_server>0.077</max_time_server>
<min_time_server>0.0770</min_time_server>
<max_time_server>0.0770</max_time_server>
<nb_hits_with_time_transfer>1</nb_hits_with_time_transfer>
<min_time_transfer>0.412</min_time_transfer>
<max_time_transfer>0.412</max_time_transfer>
<min_time_transfer>0.4120</min_time_transfer>
<max_time_transfer>0.4120</max_time_transfer>
<nb_hits_with_time_dom_processing>1</nb_hits_with_time_dom_processing>
<min_time_dom_processing>1.055</min_time_dom_processing>
<max_time_dom_processing>1.055</max_time_dom_processing>
<min_time_dom_processing>1.0550</min_time_dom_processing>
<max_time_dom_processing>1.0550</max_time_dom_processing>
<nb_hits_with_time_dom_completion>1</nb_hits_with_time_dom_completion>
<min_time_dom_completion>0.333</min_time_dom_completion>
<max_time_dom_completion>0.333</max_time_dom_completion>
<min_time_dom_completion>0.3330</min_time_dom_completion>
<max_time_dom_completion>0.3330</max_time_dom_completion>
<nb_hits_with_time_on_load>1</nb_hits_with_time_on_load>
<min_time_on_load>0.077</min_time_on_load>
<max_time_on_load>0.077</max_time_on_load>
<min_time_on_load>0.0770</min_time_on_load>
<max_time_on_load>0.0770</max_time_on_load>
<sum_bandwidth>0</sum_bandwidth>
<nb_hits_with_bandwidth>0</nb_hits_with_bandwidth>
<min_bandwidth />
Expand Down
Loading

0 comments on commit 5863dc9

Please sign in to comment.