Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
huan086 opened this issue Apr 17, 2017 · 5 comments
Labels
Milestone

Comments

@huan086
Copy link
Contributor

huan086 commented Apr 17, 2017

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?

huan086 added a commit to huan086/piwik-dotnet-tracker that referenced this issue Apr 17, 2017
@julienmoumne julienmoumne changed the title Incorrect Unix epoch? Incorrect Referrer Timestamp Apr 19, 2017
@julienmoumne julienmoumne added this to the v3.0.0 milestone Apr 19, 2017
@julienmoumne julienmoumne changed the title Incorrect Referrer Timestamp Always initialize DateTime in UTC Apr 19, 2017
@julienmoumne julienmoumne changed the title Always initialize DateTime in UTC Factor out time constants Apr 19, 2017
@julienmoumne
Copy link
Member

julienmoumne commented Apr 19, 2017

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?

@julienmoumne julienmoumne changed the title Factor out time constants Make tests independent of the system local Apr 19, 2017
julienmoumne added a commit that referenced this issue Apr 19, 2017
Always initalize DateTime in UTC. Fixes #52.
@julienmoumne julienmoumne changed the title Make tests independent of the system local Make tests independent of the system time zone Apr 19, 2017
@huan086
Copy link
Contributor Author

huan086 commented Apr 20, 2017

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

huan086 commented Apr 20, 2017

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.

@julienmoumne julienmoumne changed the title Make tests independent of the system time zone Invalid timestamps: Referrer, Last Order, Forced DateTime, Current TS in Cookie & First Visit Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants