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.

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

Make sure to apply the limit filter on sites and groups #7877

Merged
merged 1 commit into from May 12, 2015
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented May 11, 2015

fixes #7854

Apart from this it also fixes the following issues:

  • Make dashboard much faster again if having many sites
  • Show the correct number of available sites in the All Websites Dashboard (groups were not counted)
  • If the first site of a dashboard belongs to a group, we make sure this group will be shown as well at the very beginning.

Roughly the problem was the following:

When requesting all the sites we move the sites having a group into group rows. This is basically a structure like this:

 * array(
 *    site1
 *    group2
 *        subtable => site3
 *                    site4
 *                    site5
 *    site6
 *    site7
 * )

When eg offset 3, limit 2 was requested, only the site 7 was returned as the offset was applied only on the first level. I tried different implementations and ways to solve this. I do now implement a custom limit filter that is applied after sort (generic filter) and before all others. Basically I turn this array into a flat structure like this

 * array(
 *    site1
 *    group2
 *    site3
 *    site4
 *    site5
 *    site6
 *    site7
 * )

and then apply the filter. In the past logic we turned all the sites into such a flat array after all filters were run but we have to do it actually after the generic filters were run to make sure we do not apply all the queued filters and processed metrics on all rows.

I tested it with 33k sites and it was quite fast. Especially since people usually do not page through 100 pages.

…ake dashboard much faster, and show the correct number of available sites
@tsteur tsteur added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label May 11, 2015
@tsteur tsteur added this to the 2.14.0 milestone May 11, 2015
@@ -61,11 +65,14 @@ public function __construct($period, $date, $segment)
$pastRow = $pastData->getRowFromLabel($idSite);
if ($pastRow) {
$pastRow->setColumn('label', $site['name']);
$pastData->setLabelsHaveChanged();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This here causes a huge performance regression when having eg 30k sites. It made it possibly even slower than before. Meaning we spent lots of time to improve performance just to make it slow again a week later. We need performance regression tests. It took minutes to request the all websites dashboard with 33k websites and it did actually not finish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, created an issue about performance testing: #7889

@tsteur tsteur added the Needs Review PRs that need a code review label May 11, 2015

foreach ($sites as $site) {
if (!empty($site['subtable'])) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this code is no longer needed since we remove all subtables in nestedLimit filter

diosmosis added a commit that referenced this pull request May 12, 2015
Fixes #7854, make sure to apply the limit filter on sites in groups instead of just groups and top level sites in all website dashboard.
@diosmosis diosmosis merged commit d7b7fa2 into master May 12, 2015
@diosmosis diosmosis deleted the 7854 branch May 12, 2015 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants