search

matomo-org / piwik-dotnet-tracker

C# API client SDK for the Piwik Tracking API
BSD 3-Clause "New" or "Revised" License
74 stars 47 forks source link

Invalid timestamps: Referrer, Last Order, Forced DateTime, Current TS in Cookie & First Visit #52

Closed huan086 closed 7 years ago

huan086 commented 7 years ago

There are many places where new DateTime(1970, 1, 1, 0, 0, 0) is used. Some calculation is done with this epoch and then stored in DateTimeOffset. The resulting DateTimeoffset.Offset will be dependent on the timezone settings of the computer.

As a result, the test SetAttributionInfo_WhenReferrerTimestampSpecified_IsAddedToRequest fails on my computer.

Any reason why new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc) isn't used?

julienmoumne commented 7 years ago

I am trying to assess whether this is a bug or an improvement.

I think the test was failing on your machine because of the way the test is written.

Because we were not mixing dates with different time zones while doing subtractions I believe timestamps sent to Piwik were correct.

Could you help me confirm this?

huan086 commented 7 years ago

The main culprit for the failing test is FormatTimestamp using

DateTime origin = new DateTime(1970, 1, 1, 0, 0, 0, 0);
TimeSpan diff = date - origin;

date is DateTimeOffset. When subtracting DateTime from DateTimeOffset, the DateTime is cast into DateTimeOffset. Since origin has DateTimeKind.Unspecified, it is treated as local time zone. In my case, the resultant DateTimeOffset.Offset is +8 (thus origin is treated as 8 hours before Unix epoch)

julienmoumne commented 7 years ago

Would you have an idea why the tests were passing both on travis-ci and on my machine? My machine's time zone is CET.

julienmoumne commented 7 years ago

I found the reason why tests were not failing on travis-ci nor my machine.

.Substring(0,6) was used on unix time when asserting expected values in the tests.

This allowed for a few hours of error.

We still have to assess how this was impacting the validity of the data sent to the Piwik server.

huan086 commented 7 years ago

For users using AWS and Azure, default timezone is set to UTC. So no impact for them.

If I'm not wrong, for users who had changed the timezone settings and for those who bought Virtual Private Server from their local providers, they probably have timezone set to their local timezone. They would have time of visit reported either too early or too late. For people who live in timezones with daylight savings 👎, it would be a total headache to remedy the data.