@andris-zalitis opened this issue on March 31st 2016

The bug #9839 happens because of the code on the client side.

When we calculate the values we're going to send to the tracker (in getRequest) we have a code like this:

   if (!cookieSessionValue) {
        // cookie 'ses' was not found: we consider this the start of a 'session'
        // here we make sure that if 'ses' cookie is deleted few times within the visit
        // and so this code path is triggered many times for one visit,
        // we only increase visitCount once per Visit window (default 30min)
        var visitDuration = configSessionCookieTimeout / 1000;
        if (!cookieVisitorIdValues.lastVisitTs
            || (nowTs - cookieVisitorIdValues.lastVisitTs) > visitDuration) {
            cookieVisitorIdValues.visitCount++;
            cookieVisitorIdValues.lastVisitTs = cookieVisitorIdValues.currentVisitTs;
        }

The idea is - if session cookie has been expired, then we must start a new session. So the timestamp of last visit now should be the value which in previous session we had as current visit timestamp.

A bit further down we send the updated lastVisitTs to the server.

Now the problem is at the point when we get to getRequest we have already overwritten currentVisitTs value in our cookie. Take a look at setVisitorIdCookie and you'll notice that we just pass nowTs the value of current visit timestamp. The problem is - we call setVisitorIdCookie at the very beginning of Tracker initialization (for me it usually doesn't complete there though because site id is not set yet, but we also call it from setSiteId and it completes then).

So this results in the situation when we overwrite currentVisitTs with a timestamp from new Date() before we call getRequest. So if the session has been expired, we will set lastVisitTs to the value of timestamp from new Date() and send it to the server. On the server we will calculate passed days since last visit timestamp and will see that the value is 0.

@tsteur commented on April 3rd 2016

I've read the description a few times and read the code a few times as it's not trivial to understand :) Thx for this fix @andris-zalitis I bet it was not trivial to find :+1:

I will write some of my thoughts down that I had so far:

I did not know that "Visits by days since last visit" is calculated based on a Tracking API parameter _viewts. The sent timestamp is only used on the first action of a new visit see https://github.com/piwik/piwik/blob/2.16.1-rc1/plugins/VisitorInterest/Columns/VisitsByDaysSinceLastVisit.php#L44-L47. The actual implementation is here: https://github.com/piwik/piwik/blob/2.16.1-rc1/core/Tracker/Request.php#L244

There's quite a bit of code for lastVisitTs in piwik.js and it seems all the code is only for setting _viewts tracking parameter. This means we don't need to think too much about side effects and only the value of the first tracking request is important. What the value of lastVisitTs is for following tracking requests is not so important. However, lastVisitTs also increases the visit count in the tracking API but this does not seem to actually create a new visit in the tracking API.

From my understanding on the initial piwik.js load we should make sure the value of lastVisitTs is actually the value of the cookie before a new timestamp is generated (which is done in this PR I think).

On following tracking requests I'm not sure if we should actually increase lastVisitTs and set it to the current timestamp or leave the value of the initial timestamp (I think this version is implemented in this PR). I don't 100% understand the comment in https://github.com/piwik/piwik/blob/2.16.1-rc1/js/piwik.js#L3749-L3754 but I believe we need to always update lastVisitTs after each tracking request but also need to make sure on the first request we actually use the timestamp from a previous visit.

I will now have a look at the PR again as I understand it better :) Sorry for maybe confusing comment :)

@tsteur commented on April 3rd 2016

Some more thoughts after looking at the PR in more detail.

I believe the current implementation of lastVisitTs is buggy even more as I believe we need to actually increase lastVisitTs after each request. Otherwise lastVisitTs always points to the start of a visit and as soon as a user is active for more than 30 minutes on a website it would increase the visit count even though there was no "pause" of 30 minutes. A comment this part is considered the start of a new visit (session) https://github.com/piwik/piwik/blob/2.16.1-rc1/js/piwik.js#L3746 but it can be for example also the case when cookies are disabled etc.

To fix this issue we likely need to set cookieVisitorIdValues.lastVisitTs = cookieVisitorIdValues.currentVisitTs; here: https://github.com/andris-zalitis/piwik/blob/9839/js/piwik.js#L3972 . We could then likely remove oldCurrentVisitTs completely and we could remove this line: https://github.com/andris-zalitis/piwik/blob/9839/js/piwik.js#L3808 which actually causes the bug.

I haven't tried it but can you try to just make those 3 changes I just mentioned and see if it fixes the issue? I could also prepare another PR to show it

@andris-zalitis commented on April 5th 2016

To fix this issue we likely need to set cookieVisitorIdValues.lastVisitTs = cookieVisitorIdValues.currentVisitTs; here: https://github.com/andris-zalitis/piwik/blob/9839/js/piwik.js#L3972 . We could then likely remove oldCurrentVisitTs completely and we could remove this line: https://github.com/andris-zalitis/piwik/blob/9839/js/piwik.js#L3808 which actually causes the bug.

Yea, I think this would work because we'd update lastVisitTs to currentVisitTs each time after preparing the request. And so the request would contain the value of previous visit ts.

I opted for not changing the existing logic, just patching so that it wouldn't clear the value we need. BTW you can read more about my findings regarding this bug here: https://forum.piwik.org/t/visitor-days-since-last-usually-has-wrong-value-0/18655

If we go by your suggestion though, can't we go even further? Do we actually need lastVisitTs to be stored in the cookie? As I understand it - it will always be equal to currentVisitTs after we execute getRequest. So with the same effect we could: 1. on the very start save currentVisitTs before we overwrite it (say in an instance variable previousVisitTs) 2. compare to this variable instead of cookieVisitorIdValues.lastVisitTs here https://github.com/piwik/piwik/blob/2.16.1-rc1/js/piwik.js#L3753-L3754 3. assign it to _viewts here https://github.com/piwik/piwik/blob/2.16.1-rc1/js/piwik.js#L3828

@tsteur commented on April 5th 2016

If we go by your suggestion though, can't we go even further?

Good point, we would likely not need it. There might be only a problem with backwards compatibility and making sure that cookies having the old format still work in case we don't store lastVisitTs anymore. So we would maybe need to still store it in cookie for BC but not use it anymore or so.

@tsteur commented on April 18th 2016

@andris-zalitis are you still working on this?

@mattab commented on May 27th 2016

Thank you for this proposed pull request.

Because it was last updated more than one month ago, it is our policy to close pull requests opened for a long time without updates. If you would like to continue work on the pull request, please simply ping us to have it re-opened (after you have pushed a new commit).

We hope you understand this and we look forward to seeing an update from you on this pull request or another one!

Thanks.

This issue was closed on May 27th 2016
Powered by GitHub Issue Mirror