RedisTimeSeries / NRedisTimeSeries

.Net Client for RedisTimeSeries
https://redistimeseries.io
BSD 3-Clause "New" or "Revised" License
28 stars 11 forks source link

Refactor TimeStamp class to UNIXTimeStamp #27

Open DvirDukhan opened 4 years ago

DvirDukhan commented 4 years ago

As a follow up to #20 DateTime should be converted to UNIXTimeStamp so this library API be fully compliant with RedisTimeSeries native operations. This change will trigger 2.0.x Release Side effects:

  1. This change will no longer allow users to store DateTime values without losing information since the translation from DateTime to UNIX timestamp and DateTime again will cause losing information resolution.
shaunsales commented 4 years ago

Is there currently a proposed solution for this? Personally I think it's a good change to make, I lost quite a bit of time on this issue starting out and I think others might face the same problems.

The workaround that I've implemented so far is very basic, but gets the job done. I only work with the long type and just convert the parameters inline with the extension methods below;

public static long ToUnixMs(this DateTime timestamp)
{
    return new DateTimeOffset(timestamp).ToUnixTimeMilliseconds();
}

public static DateTime FromUnixMs(this long unixMs)
{
    return DateTimeOffset.FromUnixTimeMilliseconds(unixMs).UtcDateTime;
}

It's worth nothing that currently the range of Unix Ms supported by .NET is larger than the range supported by RedisTimeSeries. For example;

var epochMinus1d = new DateTimeOffset(DateTime.UnixEpoch.AddDays(-1)).ToUnixTimeMilliseconds();
Console.WriteLine(epochMinus1d);
// Output: -86400000

var dateTime = DateTimeOffset.FromUnixTimeMilliseconds(epochMinus1d).UtcDateTime;
Console.WriteLine(dateTime);
// Output: 12/31/1969 12:00:00 AM

I've logged an issue regarding negative timestamp support at redistimeseries/redistimeseries#434

Regarding the loss of information, if we clearly outline that RedisTimeSeries is accurate to the resolution of 1ms in the documentation I think that's OK, especially if we no longer throw errors when adding samples at or before the current timestamp. Ideally if we insert the same timestamp multiple times we can choose how it changes the value data, like overwrite, add or throw error etc.

DvirDukhan commented 4 years ago

The plan is:

shaunsales commented 4 years ago

Could we call it RedisTimeStamp, and create some argument range checks. Let me know if you'd like me to do the PR for this - I have some time this week.

DvirDukhan commented 4 years ago

@shaunsales Your help is highly appreciated, thanks. What are the range checks for? negative timestamps? I would like the new type name will be as descriptive as possible, so perhaps RTSTimeStamp (RTS = RedisTimeSeries)? As this is not actually a pure Redis datatype.

shaunsales commented 4 years ago

@DvirDukhan I've completed a first pass of implementing the new TsTimeStamp type. It can only be constructed with valid data from either long or DateTime types. For auto-timestamping, I've overloaded methods to not require timestamp parameters and inform the user in the code hints. You can see the commit here: shaunsales/NRedisTimeSeries@dc2b675

On my TODO list is;

DvirDukhan commented 4 years ago

@shaunsales please open a PR so we can iterate there