Fix UTC handling for TimeStamp column with ConvertToUtc=true #660
Conversation
Previously, the TimeStamp column stored local DateTime values even when ConvertToUtc was enabled, unless the column type was DateTimeOffset. This change ensures that UtcDateTime is used when ConvertToUtc is true, so timestamps are always stored in UTC as intended. Adds a unit test (Bugfix serilog-mssql#659) to verify correct UTC storage and DateTimeKind.
| return new KeyValuePair<string, object>(_columnOptions.TimeStamp.ColumnName, dateTimeOffset); | ||
|
|
||
| return new KeyValuePair<string, object>(_columnOptions.TimeStamp.ColumnName, dateTimeOffset.DateTime); | ||
| return new KeyValuePair<string, object>(_columnOptions.TimeStamp.ColumnName, _columnOptions.TimeStamp.ConvertToUtc ? dateTimeOffset.UtcDateTime : dateTimeOffset.DateTime); |
There was a problem hiding this comment.
Did this change really affect the timestamp value in the LogEvent output? IIRC the method is only used when rendering the TimeStamp column. LogEvent is rendered separately.
Anyway, if you make this change you would convert the value written to the TimeStamp column to UTC even if the column has a data type which does not store the timezone information. We deliberately only enabled this if the data type of the column is SqlDbType.DateTimeOffset when first adding ConvertToUtc. This does not mean, that we cannot change it to be that way. Let me think about it.
There was a problem hiding this comment.
Did this change really affect the timestamp value in the LogEvent output? IIRC the method is only used when rendering the TimeStamp column. LogEvent is rendered separately.
Yes, it works because in this way the Timestamp is returned as DateTime with DateTimeKind.Utc, so when serialized to JSON it will be rendered with the Z modifier.
Anyway, if you make this change you would convert the value written to the TimeStamp column to UTC even if the column has a data type which does not store the timezone information. We deliberately only enabled this if the data type of the column is SqlDbType.DateTimeOffset when first adding ConvertToUtc. This does not mean, that we cannot change it to be that way. Let me think about it.
If the Timestamp property in the LogEvent JSON lacks the Z modifier when convertToUtc=true, the deserialized value will have DateTimeKind.Unspecified, which may lead to unexpected behavior. Using DateTimeKind.Utc ensures consistent behavior.
There was a problem hiding this comment.
Have you tested it? Give me some time, so I can try it.
There was a problem hiding this comment.
Yes, I have added an explicit test to verify this new behavior, and I have also created a sample project with a project reference to the modified package to ensure that it really works “in the field”.
No problem, we’re not in a hurry—take all the time you need. Thank you for your support!
2 participants
This pull request addresses a bug related to timestamp handling in the MSSqlServer sink, ensuring that timestamps are correctly converted to UTC when the
ConvertToUtcoption is set. It also adds a corresponding unit test to verify this behavior.Bugfix: Timestamp UTC conversion
GetTimeStampStandardColumnNameAndValueinStandardColumnDataGenerator.csto useUtcDateTimefor the timestamp value whenConvertToUtcis enabled, ensuring correct UTC storage.Testing improvements
GetStandardColumnNameAndValueForTimeStampCreatesUtcDateTimeUsingUtcDateTimePropertyinStandardColumnDataGeneratorTests.csto verify that the timestamp is converted to UTC and stored as aDateTimewithDateTimeKind.UtcwhenConvertToUtcis set.Closes #659