Track seconds instead of days for "days since" dimensions #15774

diosmosis · 2020-04-03T08:14:07Z

…e tests

tsteur · 2020-04-08T03:29:34Z

core/Updates/4.0.0-b1.php

@@ -109,6 +111,18 @@ public function getMigrations(Updater $updater)
        // remove old options
        $migrations[] = $this->migration->db->sql('DELETE FROM `' . Common::prefixTable('option') . '` WHERE option_name IN ("geoip.updater_period", "geoip.loc_db_url", "geoip.isp_db_url", "geoip.org_db_url")');

+        // replace days to ... dimensions w/ segments
+        foreach (['log_visit', 'log_conversion'] as $table) {


btw we should do this all in one query and not in multiple queries so we group all log_visit and all log_conversion update queries.

one UPDATE? sure, makes sense

@diosmosis are these ones actually needed? I would assume Matomo installs the columns automatically (which is also why you didn't need to add it for new installs to the mysql schema)?

oh wait I see it is needed for the update... statement, makes sense... Could technically also record that the component was updated so Matomo won't try to update the columns

like insert into option values option_value = $columnType, option_name = version_log_visit_ visitor_seconds_since_first

Is this the desired approach?

actually I think the dimensions end up getting added after the main update, so we wouldn't be able to initialize the new dimension values

yes that's what I figured too. It's needed to add them here already. Be just good to additionally record the component version so once the DB update completed it won't try to change the type of these columns but it might not be too important since it would get the same type anyway and therefore mysql wouldn't do anything.

plugins/CoreHome/Columns/VisitorSecondsSinceOrder.php

mattab · 2020-04-08T03:33:37Z

Test case:

If custom reports are using one of these dimensions we now remove, can you test it and check what will happen? will the core:archive fail, or will the "bogus" custom report be skipped and disabled automatically or so?

diosmosis · 2020-04-08T05:16:32Z

@mattab 👍 if possible will try to make it work in that case

diosmosis · 2020-04-08T11:01:48Z

@mattab it looks like if one of those dimensions is used, it is ignored and effectively removed from the list. I was hoping to be able to add the dimension back as a view on the seconds one, but it doesn't seem like it's possible to have a dimension that is a "view" of another. I also noticed w/o it, the days dimension disappears and only the seconds one is there. This seems like it would be a problem for most users since seconds is harder to use when they want days. @tsteur can you think of a way around this? Is there a way to add a dimension class w/o it being added to a table but still having an associated column so customreports can aggregate on it?

tsteur · 2020-04-08T21:23:57Z

@diosmosis Custom reports uses as unique ID the dimension id from Dimension::getId() which is by default the PluginName.DimensionClassName. Eg Goals.DaysToConversion.

So it could help to recreate the old dimension classes. The SQL segment in that class would be something like

class DaysSince...Dimension {
    protected $type = self::TYPE_NUMBER;
    protected $sqlSegment = 'ROUND(log_visit.days_since_last_visit / 86400)';
    protected $nameSingular = 'General_DaysSinceFirstVisit';
}

you could use the same class to add back the previous segments which you are currently adding in addSegments manually.

class VisitorDaysSinceFirst {
    protected $columnName = 'visitor_days_since_first';
    protected $segmentName = 'daysSinceFirstVisit';
    protected $type = self::TYPE_NUMBER;
    protected $sqlSegment = 'ROUND(log_visit.days_since_last_visit / 86400)';
    protected $nameSingular = 'General_DaysSinceFirstVisit';
}

I think this should the trick. The seconds since first XYZ we will later need to disable in Custom Reports since they would be generating too many rows and isn't really useful for Custom Reports and could cause significant performance problems

mattab · 2020-04-08T22:47:58Z

we will later need to disable in Custom Reports

Btw if we could deal with this in general that'd be great. For example a couple of times before we had the same issue as here: a user/customer used to have a plugin (for example Provider) and some custom reports used the dimension in that plugin (eg. Provider name) and then the plugin is deactivated and the archiving then fails/errors. The fix then is to delete custom reports in the DB that contain the dimension (which is not best user experience and requires support assistance)

diosmosis · 2020-05-25T07:56:49Z

@tsteur applied review comments. There's still a failing test for the 1.5 update change. Let me know if you think that's a mistake on the tests or not.

tsteur

I see now what the problem was re the 1.5 updates @diosmosis . I reckon we could change the update script to simply not specify the third parameter in the addColumn/addColumns methods where it currently sets visitor_days_since_last/visitor_days_since_first. The order shouldn't matter so we can simply remove the parameter.

Alternatively, I was thinking we could make the AddColumns smarter to automatically add it to the end if that specified column no longer exists because the order should never really matter but shouldn't be needed and maybe there are cases where for some reason the order does matter (although I can't imagine that). So for now could just remove the third parameter in those two places.

tsteur · 2020-05-26T03:32:07Z

core/Tracker/VisitorRecognizer.php

@@ -128,14 +127,13 @@ public function findKnownVisitor($configId, VisitProperties $visitProperties, Re
        }
    }

-    public function removeUnchangedValues(VisitProperties $visitProperties, $visit)
+    public function removeUnchangedValues(VisitProperties $visitProperties, $visit, VisitProperties $originalVisit = null)


seems $visitProperties is no longer used.

tsteur · 2020-05-26T03:33:56Z

core/Updates/4.0.0-b1.php

+        }
+
+        // remove old days_to_... columns
+        $migrations[] = $this->migration->db->dropColumn('log_visit', 'visitor_seconds_since_first');


should visitor_seconds_since_first really be removed?

tsteur · 2020-05-26T03:44:54Z

js/piwik.js

@@ -3364,10 +3356,7 @@ if (typeof window.Piwik !== 'object') {

                var cookieValue = visitorIdCookieValues.uuid + '.' +
                    visitorIdCookieValues.createTs + '.' +
-                    visitorIdCookieValues.visitCount + '.' +


@diosmosis just checking whether you checked regarding https://github.com/matomo-org/matomo/pull/15774/files#r423417650 because now nowTs is in third position vs before existing cookies would have stored the visitCount there? Not sure if we need to check in getValuesFromVisitorIdCookie whether the value looks more like a timestamp?

Also I reckon in getValuesFromVisitorIdCookie() the array indexes 4 and 6 might not always be defined?

The currentVisitTs seems no longer used in general?

diosmosis · 2020-06-04T15:12:57Z

Not sure if we need to check in getValuesFromVisitorIdCookie whether the value looks more like a timestamp?

Forgot about this, added placeholder values back in, seems like the simplest approach.

diosmosis · 2020-06-04T15:26:43Z

The currentVisitTs seems no longer used in general?

No it's not, looks like we can simplify things even more here. Don't need the old cookie values now since they are all unused.

tsteur · 2020-06-24T03:19:37Z

@diosmosis is there much left to do in this PR?

diosmosis · 2020-06-24T04:24:14Z

@tsteur this was ready for another review. I must've forgotten to ping you.

tsteur · 2020-06-24T07:14:29Z

Looks good @diosmosis

diosmosis · 2020-06-25T10:47:56Z

build js

… update after it changed.

diosmosis · 2020-06-26T13:14:29Z

@tsteur / @sgiehl I updated OmniFixture.sql to have a lower version so the update would run on it fully. This, strangely, causes the ip addresses displayed in the visitor log to change to 0.0.0.0. I looked in the SQL file, and that's the correct value. When I look in the db for the branch and in 4.x-dev, the ip address for the visits shown are 0.0.0.0. But for some reason on 4.x-dev, a completely different ip address is shown... because of the update being run I guess... not sure what the issue is, but I'm merging this. Can fix the test if anyone knows what's going on.

diosmosis added 3 commits April 1, 2020 03:04

Modify dimensions and add tests.

32e582f

unfinished debugging commit

b366553

changes to fix test, remove tracker code, add update code

6a0ae22

diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Apr 3, 2020

diosmosis added this to the 4.0.0 milestone Apr 3, 2020

diosmosis added 10 commits April 3, 2020 01:39

fix some tests

90f3489

fix migration issue

fd137cc

fixing some tests

7bb3023

fix some bugs, update some expected test files, remove _idvc from som…

2e679de

…e tests

start replacing _ects

145b5d2

Finish seconds since last ecommerce order change.

d726962

fix couple issues

732a1e7

fix a couple tests

c77fe88

fix some more tests

a5f5f93

fixing a couple more tests

d896a01

tsteur reviewed Apr 8, 2020

View reviewed changes

plugins/CoreHome/Columns/VisitorSecondsSinceOrder.php Show resolved Hide resolved

apply review feedback

00dd27b

unfinished commit

2162e8e

diosmosis added 4 commits April 9, 2020 02:16

Merge branch '4.x-dev' into 14094-time-dimensions-counter

268818e

add back dimensions =

8be7f5f

Merge branch '4.x-dev' into 14094-time-dimensions-counter

c80548a

fixing tests & removing some code

db6c654

diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Apr 18, 2020

diosmosis added 3 commits May 21, 2020 20:02

fixing some tests

25fd368

Add new column version entries into the option table.

1cd3010

try undoing 1.5 update change

d7674f9

tsteur requested changes May 26, 2020

View reviewed changes

diosmosis added 2 commits June 3, 2020 22:57

Merge branch '4.x-dev' into 14094-time-dimensions-counter

b0430b3

apply review feedback

22805be

remove more unused cookie values

e09351b

diosmosis added 2 commits June 4, 2020 15:27

rebuilt piwik.js

9bc55fc

Merge branch '4.x-dev' into 14094-time-dimensions-counter

7e66594

diosmosis added 3 commits June 25, 2020 02:20

Merge branch '4.x-dev' into 14094-time-dimensions-counter

772e624

Fix test count.

5a9776a

update test files

e01cbb3

diosmosis added 6 commits June 25, 2020 10:48

rebuilt piwik.js

795fb64

removel log analytics submodule update

3d8ee8b

update submodule

1907907

update some expected screenshots

eb38b6f

Fix versions in omnifixture since we were probably adding to the beta…

c07f81e

… update after it changed.

update screenshots

b994dd6

diosmosis merged commit de5ae85 into 4.x-dev Jun 26, 2020

diosmosis deleted the 14094-time-dimensions-counter branch June 26, 2020 13:17

sgiehl mentioned this pull request Jun 29, 2020

Updates expected test files matomo-org/plugin-CustomDimensions#159

Merged

mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Sep 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track seconds instead of days for "days since" dimensions #15774

Track seconds instead of days for "days since" dimensions #15774

diosmosis commented Apr 3, 2020

tsteur Apr 8, 2020

diosmosis Apr 8, 2020

tsteur May 12, 2020

tsteur May 12, 2020

diosmosis May 18, 2020

diosmosis May 18, 2020

tsteur May 18, 2020

mattab commented Apr 8, 2020 •

edited

diosmosis commented Apr 8, 2020

diosmosis commented Apr 8, 2020

tsteur commented Apr 8, 2020

mattab commented Apr 8, 2020

diosmosis commented May 25, 2020

tsteur left a comment

tsteur May 26, 2020

tsteur May 26, 2020

tsteur May 26, 2020

diosmosis commented Jun 4, 2020

diosmosis commented Jun 4, 2020

tsteur commented Jun 24, 2020

diosmosis commented Jun 24, 2020

tsteur commented Jun 24, 2020

diosmosis commented Jun 25, 2020

diosmosis commented Jun 26, 2020

Navigation Menu

Track seconds instead of days for "days since" dimensions #15774

Track seconds instead of days for "days since" dimensions #15774

Conversation

diosmosis commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab commented Apr 8, 2020 • edited

diosmosis commented Apr 8, 2020

diosmosis commented Apr 8, 2020

tsteur commented Apr 8, 2020

mattab commented Apr 8, 2020

diosmosis commented May 25, 2020

tsteur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Jun 4, 2020

diosmosis commented Jun 4, 2020

tsteur commented Jun 24, 2020

diosmosis commented Jun 24, 2020

tsteur commented Jun 24, 2020

diosmosis commented Jun 25, 2020

diosmosis commented Jun 26, 2020

mattab commented Apr 8, 2020 •

edited