@diosmosis opened this Pull Request on December 4th 2018 Member

When a timezone is specified w/ a string like 'today', the result of Date::factory() is incorrect since it will get today in UTC and then apply the timezone. Since today in UTC removes the hour/minutes, the adjusted time can move into a different, incorrect day. (eg, timezone is UTC-5, time is 1 am Dec 3, UTC today is Dec 3 midnight, applying timezone yields Dec 2 19:00). Fixed by calculating timezone & date together using DateTime.

I'm not sure if this will have a performance penalty, hopefully not.

Refs #13786

@tsteur commented on December 4th 2018 Member

@mskala could you give this a try?

@mskala commented on December 4th 2018

It looks good so far.

@diosmosis commented on December 4th 2018 Member

Note: need to add some tests & I think yesterdaySameTime needs a change.

@mskala commented on December 5th 2018

No, I'm afraid it isn't quite right. Now - December 4, 22:00 EST, which notably is December 5 UTC - I'm getting all zeroes in the "Visits Overview" box. I think that box (which worked correctly before the patch) may now be getting the time zone correction applied twice or something of the kind. All the reports under headings like "Behaviour" and "Acquisition" are saying "There is no data for this report."

@diosmosis commented on December 9th 2018 Member

New changes:

  • Noticed dates were getting timezones applied more than they should be. The timestamp is meant to be in UTC and getTimestamp() is meant to apply the timezone, but Date::factory() was applying the timestamp again.
  • The timestamp application in getTimestamp() didn't seem to work for me. adjustForTimezone works, however, so switched to using that.
  • today could only be computed by first finding 'today' in the requested timezone using now in the requested timezone, then undoing that and getting that time in UTC, then using that in a Date instance so eventually the timezone application in getTimestamp() would convert it to the right time.

Think this change should do the trick. EDIT: or maybe not since the build is failing.

CC @tsteur

@tsteur commented on December 9th 2018 Member

It's hard to test and reproduce these things (and probably lots of different combinations). I reckon if user confirms it works it should be fine to merge and ideally we release a new beta soon.

@diosmosis commented on December 9th 2018 Member

Will try and fix the build tomorrow, don't want anyone to use the code until every test passes.

@tsteur commented on December 9th 2018 Member

Totally 👍

@diosmosis commented on December 10th 2018 Member

@tsteur fixed the build, looks like the timezone calculation was actually reversed before (UTC+1 calculated as UTC-1).

@mskala commented on December 10th 2018

I don't think this latest patch works at all. With a few days of testing it's giving me weird results, including multiple consecutive days that show the same summary statistics, and stats not restarting at zero at local midnight. I'm finding it hard to characterize exactly what's wrong - it's not as simple as just showing yesterday's stats in place of today, or resetting things at UTC midnight instead of local midnight - and it seems to differ between different Web sites that collect their stats on the same Matomo server. My best guess is that there's something cache-related being screwed up, but resetting the cache doesn't seem to fix it. I just hope that the problem is only in display, so that when there's a version that works, the data collected during these few days won't be trash.

@diosmosis commented on December 10th 2018 Member

@mskala That's a bit harder to diagnose from this end. Could you elaborate on "including multiple consecutive days that show the same summary statistics, and stats not restarting at zero at local midnight"? Screenshots might help if that's possible.

@diosmosis commented on December 10th 2018 Member

@mskala Actually, I think you're right, this change isn't right, I would undo it. If the data doesn't look right, you may have to re-archive it, but I think the underlying log data should be fine.

I'll let you know explicitly when I have something that's worth testing.

@mskala commented on December 10th 2018

See screenshot.. the Visits Over Time graph shows exactly 37 visits with bounce rate of 77% for both December 8 and 9, which seems like a surprising coincidence. At one point today, shortly after midnight this morning, it was also showing that same number of visits and bounce rate for December 10 and in the Visits Overview box, with new visits not resulting in changes to the Visits Overview box. Some, not all, of the other Web sites on this Matomo installation were showing similar behaviour (identical stats for Dec 8 and 9). However, I haven't been able to reproduce the three-days-in-a-row behaviour when trying it just now. I've been through several replacements of Date.php since.


@diosmosis commented on December 12th 2018 Member

@mskala this PR is ready for another test, make sure to change Date.php & Range.php. These changes shouldn't affect any other part of matomo, so no side effects, but I noticed much of how Date.php deals w/ timezones is just off, so there may be other issues like this elsewhere.

@mskala commented on December 14th 2018

Okay, I think this one works. I've run it for a couple of days, testing stuff like before and after midnight local and before and after midnight UTC, and I haven't caught it in any incorrect behaviour. Looks good!

@diosmosis commented on December 14th 2018 Member

Thanks for testing @mskala!

This Pull Request was closed on December 14th 2018
Powered by GitHub Issue Mirror