Skip to content

Conversation

@nathangavin
Copy link
Contributor

@nathangavin nathangavin commented Apr 15, 2025

Description:

Adds the WITH ROLLUP solution for Custom Dimension reports to improve aggregation accuracy while maintaining performance. New feature is wrapped in a feature flag.

Review

@nathangavin nathangavin marked this pull request as ready for review May 2, 2025 05:55
@nathangavin nathangavin requested a review from a team May 2, 2025 05:55
@nathangavin nathangavin added the Needs Review PRs that need a code review label May 2, 2025
@nathangavin nathangavin requested a review from mneudert May 6, 2025 20:03
@nathangavin
Copy link
Contributor Author

For some reason plugins/CustomDimensions/tests/System/ApiTest::testRankingLimit() is now producing different results each time it is run. @mneudert any idea why this might be?

@mneudert
Copy link
Member

mneudert commented May 7, 2025

Those random failures are caused by the (now) missing COALESCE(), bringing back the randomness already documented in the BlobReportLimitingTest.

In the by_archiving_query dataset the SQL query is grouping values into the "Others" row. And the order of custom_dimension_1 + url results are quite random. If you run the ranking query used in that dataset multiple times you can nicely observe that randomness (you might need to reconnect to MySQL for each query run if you don't see random results).

I can see two solutions for this:

  • skip that dataset with the feature flag inactive
  • change the fixture to ensure the result is deterministic

Probably not a bad choice to just skip it, as the BlobReportLimitingTest already documented the randomness.

@nathangavin nathangavin requested a review from a team May 8, 2025 05:20
@nathangavin
Copy link
Contributor Author

I decided to skip the test for the standard ranking query test.

@nathangavin nathangavin merged commit 4298579 into 5.x-dev May 11, 2025
26 of 27 checks passed
@nathangavin nathangavin deleted the dev-19070 branch May 11, 2025 21:23
spludlow pushed a commit that referenced this pull request May 21, 2025
…#23227)

* Add feature flag and update test to use feature flag

* Add rollup behaviour to CustomDimension SQL query generation

* Fix bugs in SQL statement with rollup

* Resolve SQL query bugs

* Process rolled up values correctly

* Fix PHPCS

* Fix PHPCS

* Update Unit tests for ranking query

* Fix PHPCS

* Update broken tests

* Update expected test files

* Add missing expected files

* Revert "Update expected test files"

This reverts commit e5bdd0f.

* Update test to view correct expected files

* Added missing expected files

* fix feature flag detection

* Update ApiTest to remove testSuffix bug

* Update expected test files

* Update UI tests broken by new feature

* Wrap new logic around a check for feature flag

* Update expected test files to fix regression issue

* Add feature flag trigger into test

* Fix UI tests broken by test fixture update

* Revert separate functions for withRollup logic

* Update test suite to include ranking limit test withoutnew feature

* Fix formatting in test

* PHPCS fix

* Update Fixture to use correct dimension

* test fix of regression bug

* test fix to regression bug

* Wrap COALESCE around feature flag

* Remove test case from base ranking query test

* PHPCS fix

* Update expected test file

* Housekeeping

---------

Co-authored-by: Marc Neudert <marc@innocraft.com>
@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed Needs Review PRs that need a code review labels Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.

Development

Successfully merging this pull request may close these issues.

4 participants