Smaller varchar size for log_action.name #14859

tsteur · 2019-09-05T00:49:56Z

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

diosmosis · 2019-09-05T01:31:48Z

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

tsteur · 2019-09-05T01:37:38Z

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 · 2019-09-10T15:07:54Z

Feedback:

Since we truncate URLs after 1024 characters by default, which can be customised in config.ini.php see: https://matomo.org/faq/troubleshooting/faq_21239/
it should be enough to create the VARCHAR with 4096 characters or so? 16k characters seems overkill and also afraid it could prevent us adding new columns in the future.
we could also update FAQ https://matomo.org/faq/troubleshooting/faq_21239/ to refer to this new hard data limit?

tsteur · 2019-09-11T00:07:41Z

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 · 2019-09-11T05:21:08Z

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

tsteur · 2019-09-12T00:03:18Z

@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.

… 14535_3-1

tsteur · 2019-09-30T03:46:41Z

core/DataAccess/LogAggregator.php

@@ -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`';


@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 · 2019-09-30T21:06:35Z

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'));


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

tsteur · 2019-10-02T03:17:19Z

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

tsteur · 2019-10-02T23:25:34Z

@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 · 2019-10-09T21:36:14Z

🚀

mattab · 2020-09-29T03:14:29Z

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/

tsteur added 2 commits September 5, 2019 12:45

Smaller varchar size for log_action.name

930be96

Update 4.0.0-b1.php

4b89bb2

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 added this to the 3.12.0 milestone Sep 5, 2019

tsteur mentioned this pull request Sep 5, 2019

log_visit.referer_url and log_action.name as TEXT instead of VARCHAR cause a lot of tmp table on disk #14535

Closed

tsteur added 2 commits September 7, 2019 08:45

Update Mysql.php

dbcad1f

Update 4.0.0-b1.php

972110a

tsteur added 4 commits September 11, 2019 08:38

Update Mysql.php

888b2da

Update 4.0.0-b1.php

93c2b39

Merge branch '3.x-dev' into 14535_3-1

97a6fa2

fix tests

2383504

update tests

49399ef

tsteur and others added 4 commits September 27, 2019 08:45

hide subtables id which are randomly changing

89e2833

Remove contentTarget from archived content data

a676eb9

Merge branch '14535_3-1' of https://github.com/matomo-org/matomo into…

37197a8

… 14535_3-1

Add secondary order by name to LogAggregator query

99cdb0f

tsteur commented Sep 30, 2019

View reviewed changes

Move logic for secondary order by into Transitions API

2b6231f

Remove contentTarget from expected XML files for BlobReportingLimitTest

d679ce5

Add order by to contents archiver

41a3955

katebutler added 2 commits October 9, 2019 09:38

Add order by to events archiver

481fd3b

Refactoring to trigger build

c37be69

tsteur merged commit 42f9f8c into 3.x-dev Oct 9, 2019

sgiehl deleted the 14535_3-1 branch February 10, 2020 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smaller varchar size for log_action.name #14859

Smaller varchar size for log_action.name #14859

tsteur commented Sep 5, 2019 •

edited

Loading

diosmosis commented Sep 5, 2019

tsteur commented Sep 5, 2019

mattab commented Sep 10, 2019

tsteur commented Sep 11, 2019

diosmosis commented Sep 11, 2019

tsteur commented Sep 12, 2019

tsteur Sep 30, 2019

tsteur Sep 30, 2019

tsteur commented Oct 2, 2019

tsteur commented Oct 2, 2019

tsteur commented Oct 9, 2019

mattab commented Sep 29, 2020

Smaller varchar size for log_action.name #14859

Smaller varchar size for log_action.name #14859

Conversation

tsteur commented Sep 5, 2019 • edited Loading

diosmosis commented Sep 5, 2019

tsteur commented Sep 5, 2019

mattab commented Sep 10, 2019

tsteur commented Sep 11, 2019

diosmosis commented Sep 11, 2019

tsteur commented Sep 12, 2019

tsteur Sep 30, 2019

Choose a reason for hiding this comment

tsteur Sep 30, 2019

Choose a reason for hiding this comment

tsteur commented Oct 2, 2019

tsteur commented Oct 2, 2019

tsteur commented Oct 9, 2019

mattab commented Sep 29, 2020

tsteur commented Sep 5, 2019 •

edited

Loading