Skip to content

Fix UTC handling for TimeStamp column with ConvertToUtc=true #660

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.

Sign up for GitHub

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

Open
marcominerva wants to merge 1 commit into
base: dev
Choose a base branch
from
+24 −1
Open

Fix UTC handling for TimeStamp column with ConvertToUtc=true #660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/Output/StandardColumnDataGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ private KeyValuePair<string, object> GetTimeStampStandardColumnNameAndValue(LogE
if (_columnOptions.TimeStamp.DataType == SqlDbType.DateTimeOffset)
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);
Copy link
Member

@ckadluba ckadluba Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@marcominerva marcominerva Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ckadluba ckadluba Dec 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested it? Give me some time, so I can try it.

Copy link
Member

@ckadluba ckadluba Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested it?

Copy link
Author

@marcominerva marcominerva Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

private string RenderLogEventColumn(LogEvent logEvent)
Expand Down
23 changes: 23 additions & 0 deletions ...ilog.Sinks.MSSqlServer.Tests/Sinks/MSSqlServer/Output/StandardColumnDataGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,29 @@ public void GetStandardColumnNameAndValueForTimeStampCreatesUtcConvertedTimeStam
Assert.Equal(new TimeSpan(0), timeStampColumnOffset.Offset);
}

[Trait("Bugfix", "#659")]
[Fact]
public void GetStandardColumnNameAndValueForTimeStampCreatesUtcDateTimeUsingUtcDateTimeProperty()
{
// Arrange
var options = new Serilog.Sinks.MSSqlServer.ColumnOptions
{
TimeStamp = { ConvertToUtc = true }
};
var testDateTimeOffset = new DateTimeOffset(2020, 1, 1, 9, 0, 0, new TimeSpan(1, 0, 0)); // Timezone +1:00
var logEvent = CreateLogEvent(testDateTimeOffset);
SetupSut(options, CultureInfo.InvariantCulture);

// Act
var column = _sut.GetStandardColumnNameAndValue(StandardColumn.TimeStamp, logEvent);

// Assert
Assert.IsType<DateTime>(column.Value);
var timestamp = (DateTime)column.Value;
Assert.Equal(testDateTimeOffset.Hour - 1, timestamp.Hour);
Assert.Equal(DateTimeKind.Utc, timestamp.Kind);
}

[Fact]
public void GetStandardColumnNameAndValueForExceptionReturnsExceptionKeyValue()
{
Expand Down
Loading