Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix aggregation of additional action metrics #22348

Open
wants to merge 10 commits into
base: 5.x-dev
Choose a base branch
from
Open

Fix aggregation of additional action metrics #22348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f01204
Correctly set column aggegation methods for action reports
sgiehl Jun 27, 2024
f5b896c
don't provide a max value if no values are available at all
sgiehl Jun 27, 2024
a6c5d20
update expected test files
sgiehl Jun 27, 2024
c5cde81
update submodule
sgiehl Jun 27, 2024
fe2599b
Fix min/max checks
sgiehl Jul 1, 2024
c3d4a93
Ensure to store null for min/max event value metric if no values are …
sgiehl Jul 2, 2024
b922e1b
don't use numeric check to not break percentage values
sgiehl Jul 2, 2024
6e22bed
Updates expected test files
sgiehl Jul 2, 2024
05580b4
Ensure correct aggregation of min/max event value metrics
sgiehl Jul 2, 2024
3eb9156
apply review feedback
sgiehl Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions core/DataTable/Row.php
View file
Open in desktop
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);
}
sgiehl marked this conversation as resolved.
Show resolved Hide resolved
break;
case 'min':
sgiehl marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading