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

Jump to bottom

Smaller varchar size for log_action.name #14859

Merged
merged 18 commits into from Oct 9, 2019
Merged

Smaller varchar size for log_action.name #14859

merged 18 commits into from Oct 9, 2019

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Sep 5, 2019 •
edited

Follow up from #14848 refs #14535

When I tried to apply this change locally, I got this error

An error occurred when trying to change the field 'name' via

ALTER TABLE `log_action` CHANGE `name` `name` VARCHAR(65000)
 CHARACTER SET utf8
 COLLATE utf8_general_ci
 NULL
 DEFAULT ''

MySQL said: Column length too big for column 'name' (max = 21845); use BLOB or TEXT instead

It seems the column length is too big.

For log_conversion.url even only varchar(16000) works since there are so many more columns. I reckon it might be best to leave it as text. It be not good to have different lengths of url fields and not sure if by using varchar(16000) on log_conversion we might prevent in the future our chances of adding more columns...

fyi @mattab

@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 5, 2019
@tsteur tsteur added this to the 3.12.0 milestone Sep 5, 2019
@tsteur tsteur mentioned this pull request Sep 5, 2019
Closed
@diosmosis
Copy link
Member

Not sure how it's related to this PR, but the tests appear to be failing

@tsteur
Copy link
Member Author

tsteur commented Sep 5, 2019

Not understanding it... thought maybe we use really long action names but that doesn't seem to be the case in https://travis-ci.org/matomo-org/matomo/jobs/580980359#L848

@mattab
Copy link
Member

mattab commented Sep 10, 2019

Feedback:

@tsteur
Copy link
Member Author

tsteur commented Sep 11, 2019

Changed it to 4096 and fixed the tests.

The results in tests/PHPUnit/System/expected/ are correct. It is only the sorting that slightly changed because of the varchar when there is eg the same amount of visits.

The results in plugins/Contents/tests/System/expected/ are correct too. Took me quite some time to look into it and 100% understand things. The contentTarget for example changed because there are multiple impressions with the same content name but different targets see https://github.com/matomo-org/matomo/blob/3.12.0-b3/plugins/Contents/tests/Fixtures/TwoVisitsWithContents.php#L79-L84 where we use 'Click to download Piwik now', '' but also 'Click to download Piwik now', 'http://piwik.org/download'.

We actually do not show the content target in the UI (which would make it 3 level report). We're currently for some reason adding contentTarget to the report but the contentTarget value we use in the report is random if there are multiple content names/pieces with different targets. So no logic changed there and things are the same. Ideally at some point we may want to look at removing content target from the report and / or implement targets correctly see #6267 Not changing any logic there for now for this.

Ready for another review. Hoping the tests pass now.

@diosmosis
Copy link
Member

Still one test failure, but I think it's expected?

@tsteur
Copy link
Member Author

tsteur commented Sep 12, 2019

@diosmosis it seems the tests are failing now randomly and have different IDs on every request or different results in general :( the same tests are always failing in different ways. We'll need to see how this can be fixed... will likely take quite a while.

tsteur
tsteur commented Sep 30, 2019
View reviewed changes
core/DataAccess/LogAggregator.php Outdated
@@ -838,7 +838,7 @@ public function queryActionsByDimension($dimensions, $where = '', $additionalSel
}

if ($rankingQuery) {
$orderBy = '`' . Metrics::INDEX_NB_ACTIONS . '` DESC';
$orderBy = '`' . Metrics::INDEX_NB_ACTIONS . '` DESC, `name`';
Copy link
Member Author

Choose a reason for hiding this comment

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

@katebutler I haven't looked at the tests but would expect a lot of tests to fail. This we might need to change to only set it for the transitions query. Maybe would need to be some parameter?

tsteur
tsteur commented Sep 30, 2019
View reviewed changes
plugins/Contents/Archiver.php
@@ -117,7 +116,7 @@ private function aggregateDayImpressions()
$rankingQuery = null;
if ($rankingQueryLimit > 0) {
$rankingQuery = new RankingQuery($rankingQueryLimit);
$rankingQuery->addLabelColumn(array('contentPiece', 'contentTarget', 'contentName'));
$rankingQuery->addLabelColumn(array('contentPiece', 'contentName'));
Copy link
Member Author

Choose a reason for hiding this comment

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

above is a join that can be likely removed for content target

@tsteur
Copy link
Member Author

tsteur commented Oct 2, 2019

@katebutler can you also update the expected test results so the tests pass?

@tsteur
Copy link
Member Author

tsteur commented Oct 2, 2019

@katebutler is maybe this test failing still randomly? https://travis-ci.org/matomo-org/matomo/jobs/592761199#L820-L850
If that's the case we may need to have a secondary sort order column in the Sort datatable filter. This could fix this issue also for other reports in the future in https://github.com/matomo-org/matomo/blob/3.12.0-b4/core/DataTable/Filter/Truncate.php#L97 I reckon it could be good to enable $doSortBySecondaryColumn = true parameter

@tsteur
Copy link
Member Author

tsteur commented Oct 9, 2019

🚀

@tsteur tsteur merged commit 42f9f8c into 3.x-dev Oct 9, 2019
@sgiehl sgiehl deleted the 14535_3-1 branch February 10, 2020 13:00
@mattab
Copy link
Member

mattab commented Sep 29, 2020

fyi added As of Matomo 4.0.0, only the first 4096 characters of a page title / URL / any action name will be stored, even if page_maximum_length is set to a higher value. to https://matomo.org/faq/troubleshooting/faq_21239/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
3.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsteur @diosmosis @mattab