Add TimeStampWithOffset #849

gareththackeray · 2021-01-22T12:30:37Z

In a global system, it is far better to be explicit about timestamps and include the offset to remove any ambiguity.

Ideally we could switch TimeStamp to use DateTimeOffset but this would be a breaking change which could have an impact on any message consumers that were expecting an offsetless datetime format.

codecov · 2021-01-22T12:32:46Z

Codecov Report

Merging #849 (b39a76a) into master (13a11d6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
+ Coverage   84.03%   84.04%   +0.01%     
==========================================
  Files         113      113              
  Lines        2893     2896       +3     
==========================================
+ Hits         2431     2434       +3     
  Misses        462      462

Flag	Coverage Δ
linux	`84.04% <100.00%> (+0.01%)`	⬆️
macos	`53.49% <40.00%> (+0.01%)`	⬆️
windows	`53.95% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/JustSaying.Models/Message.cs	`100.00% <100.00%> (ø)`
src/JustSaying/JustSayingBus.cs	`96.05% <100.00%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a11d6...b39a76a. Read the comment docs.

martincostello · 2021-01-22T12:35:46Z

src/JustSaying.Models/Message.cs

            TimeStamp = DateTime.UtcNow;
-            Id = Guid.NewGuid();
+            TimeStampWithOffset = DateTimeOffset.Now;
+            Id = Guid.NewGuid();            


I think it would be better to put this into a local, and then reference from there. Otherwise you have the possibility of having, albeit slight in magnitude, probably just a few ticks, differences in the value of the time in the two properties.

Something like:

var now = DateTimeOffset.Now; TimeStampWithOffset = now; TimeStamp = now.UtcDateTime;

Should we perhaps call this something like UtcTimeStamp? Or TimeStampUtc?

That it has an offset is a .net implementation detail - the key bit is as you say, we are explicitly declaring it a UTC timestamp.

Also, do we want to use DateTimeOffset.UtcNow? Or is this expected to be the servers time even if that's not in UTC?

Should we perhaps call this something like UtcTimeStamp? Or TimeStampUtc?

That it has an offset is a .net implementation detail - the key bit is as you say, we are explicitly declaring it a UTC timestamp.

Hmm I don't know - I think having an offset could be useful sometimes. It gives you some extra informaion (what was the offset where this message was sent) without any loss.
We also can't guarantee it won't be overridden with a different offset.

Note that as currently written, it won't (necessarily) be "UTC" (i.e. offset 0)

Renaming it from TimeStamp would be a breaking change.

I would agree that I'd prefer to just use UTC for both, rather than introduce two different ones that make assumptions about the time zone of the executing machine (which in prod for us will be UTC anyway).

I didn't bring that up initially though as I assumed there was offline conversation between Luke, Stu and Gareth that resulted in this PR using local times in the first place.

If I had to choose I'd just have it as the UTC offset (so +00:00). It's still more informative and it's more-or-less explicit, it just lacks a non-zero offset value, which for for non-UI isn't really a problem IMHO.

As "+01:00" in the UK in the summer is just an inference of UK local time to us, it could equally be the time in the Canary Islands. Without specifying the time zone's name you can't be 100% sure of what the offset is specifically referring to anyway.

Not sure I agree Martin. The point is it could tells you what time it was for a relevant party at the time the performed the action, which is extra info. I don't think the name of the timezone is important.

This is moot for us since as you say it will always be UTC and I'm happy to go either way, but local is my preference. @stuart-lang any thoughts?

I think we need to be a bit careful about adding fields to this base class, and only do so when we have a really good reason, and scenarios that make it really worthwhile.

To add some more data, let's consider Australia's timezones. Our servers are located in ap-southeast-2, which is in Sydney. Australia has three timezones, some of which apply DST, and others that do not, at different times of the year. Given that Menulog is available all over Australia, the local timezone that an order is placed in could be quite different to the timezone that the server that handled that request is in. In this case, using this new field for anything would be quite risky, as the local time could be either ahead or behind the server time - we would need to account for it in any usages of the field. There aren't really any 'safe' assumptions we can make about the relation of the offset time and the server time, if that makes sense, which makes me question its usefulness.

In the above scenario, if we did actually want to capture the local time where the order was placed, I think it would make more sense to be explicit and to use a LocalOrderPlacedAt DateTimeOffset field on whichever messages that fulfill that flow, which would be populated from an incoming request.

I'm not flat out suggesting we don't do this, just that I think we need some solid scenarios and use cases to support its addition.

The key reason for this field isn't to capture something about timezone, it's to ensure we have an unambiguous timestamp available on the message. For JE it is really moot whether we use Now or UtcNow because the servers are set to UTC anyway.

The offset is only useful for caring about that specific message at that point in time. You can't use the offset to infer future things from it, as the offset may change during the life of the order, which again makes me wonder how useful it is to have a non-UTC value.

The only truly unambiguous way to represent the time zone is to use its name (e.g. Europe/London in IANA format).

Sure you can say for a specific event that it happened at a wall-clock time of 09:52 to the user, but you can't rely on the offset to compute other timestamps relative to it (e.g. what is the wall-clock time an hour later), which is what makes me think you might as well just encode it as +00:00 to make it clear it's UTC.

It potentially makes things easier like "find all the events that happened between 9 and 5 local time". I have a very weak opinion on this though. If the consensus is for UTC I'll do that. And it's 2 on 1 so far.

Be good to get your thougts on my comment below as well.

slang25 · 2021-01-22T13:52:29Z

First thoughts from me; we need to be really careful about adding non-nullable property to the Message base class as it might fail deserialization from older producers. It must be able to handle the case where the property is missing in JSON.

To be clear, something we are going to be encouraging more is for people to avoid contracts libraries, and have their own local definitions of message type, so it will be common for someone to publish a JustSaying 6 message, and expect it to fully deserialize with a JustSaying 7 message base class.

gareththackeray · 2021-01-22T14:03:45Z

First thoughts from me; we need to be really careful about adding non-nullable property to the Message base class as it might fail deserialization from older producers. It must be able to handle the case where the property is missing in JSON.

That's a good point. And aside from the ability to deserialise, you have the scenario the sender doesn't have it on their schema and it deserialises ok but goes to its default value, with bugs on the consumer side ensuing.

I'll change it to nullable.

gareththackeray · 2021-01-22T15:18:21Z

src/JustSaying.Models/Message.cs

@@ -6,12 +6,14 @@ public abstract class Message
    {
        protected Message()
        {
-            TimeStamp = DateTime.UtcNow;
-            Id = Guid.NewGuid();
+            TimeStampWithOffset = DateTimeOffset.Now;


There's a problem here. If this is used as a base class on the receiving end and the publisher was using a version that didn't have the property, the ctor is going to set the value to Now and leave it like that.

I guess the publisher should set it if its null. And for those features that don't use the JS publisher, their local publishers will just need to do likewise.

Perhaps we could leave it null in the constructor, and populate it in the library before we send it? That way we keep the init out of the Models lib, and provides a possible extension point for what you were thinking of with other users possibly using a different 'base' TZ.

gkinsman · 2021-01-27T12:02:51Z

src/JustSaying/JustSayingBus.cs

@@ -247,6 +247,12 @@ private IMessagePublisher GetPublisherForMessage(Message message)
            CancellationToken cancellationToken)
        {
            attemptCount++;
+
+            if (message.TimeStampWithOffset == null)


I wonder if we should move the set of the existing TimeStamp property here as well, so that both are populated at the same time. Given the models package is being versioned as well, we can make that change safely.

gareththackeray · 2021-02-04T11:06:41Z

It turns out that, by default, DateTimes created using DateTime.UtcNow get serialised with a Z at the end to signify UTC. So although a DateTimeOffset would be preferable in a couple of ways, there is no actual issue with the current state of affairs. Closing PR.

Add TimeStampWithOffset

5c29d2e

gareththackeray requested a review from a team as a code owner January 22, 2021 12:30

gareththackeray requested a review from slang25 January 22, 2021 12:30

martincostello reviewed Jan 22, 2021

View reviewed changes

ensure TimeStamp and TimeStampWithOffset are the same

ee6f669

gareththackeray commented Jan 22, 2021

View reviewed changes

make TimeStampWithOffset nullable + don't set it in Message ctor

b39a76a

gkinsman reviewed Jan 27, 2021

View reviewed changes

Base automatically changed from master to main January 31, 2021 10:54

brainmurphy added this to the v7.0.0 milestone Feb 3, 2021

gareththackeray closed this Feb 4, 2021

gareththackeray deleted the timestampwithoffset branch February 4, 2021 11:06

martincostello removed this from the v7.0.0 milestone Feb 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TimeStampWithOffset #849

Add TimeStampWithOffset #849

gareththackeray commented Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

martincostello Jan 22, 2021

gkinsman Jan 22, 2021

gkinsman Jan 22, 2021 •

edited

Loading

gareththackeray Jan 22, 2021

martincostello Jan 22, 2021

gareththackeray Jan 22, 2021

gkinsman Jan 22, 2021 •

edited

Loading

gareththackeray Jan 22, 2021

martincostello Jan 22, 2021

gareththackeray Jan 22, 2021

slang25 commented Jan 22, 2021 •

edited

Loading

gareththackeray commented Jan 22, 2021

gareththackeray Jan 22, 2021

gkinsman Jan 22, 2021

gkinsman Jan 27, 2021

gareththackeray commented Feb 4, 2021

Add TimeStampWithOffset #849

Add TimeStampWithOffset #849

Conversation

gareththackeray commented Jan 22, 2021

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkinsman Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkinsman Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slang25 commented Jan 22, 2021 • edited Loading

gareththackeray commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gareththackeray commented Feb 4, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

gkinsman Jan 22, 2021 •

edited

Loading

gkinsman Jan 22, 2021 •

edited

Loading

slang25 commented Jan 22, 2021 •

edited

Loading