Support UtcTimestamp token in output template (new since Serilog 4.0) #165
Conversation
…plate (OutputTemplateRenderer) Implements serilog#164
| { | ||
| var sv = new DateTimeOffsetValue(logEvent.Timestamp); | ||
| var timestamp = _convertToUtc | ||
| ? logEvent.Timestamp.ToUniversalTime() |
There was a problem hiding this comment.
Unfortunately DateTimeOffset ISO formatting in UTC renders as the unanticipated T00:00 rather than the more common and compact Z notation. You should be able to see this by running the code with {UtcTimestamp} and no format string.
MessageTemplateTextFormatter gets around this using .UtcDateTime and renders the value as a DateTime with DateTimeKind.Utc:
There was a problem hiding this comment.
Thanks for pointing that out, I'll have a look into that and will address it in an update.
There was a problem hiding this comment.
@nblumhardt, the issue you pointed out is now addressed in the latest update. Rendering {UtcTimestamp} is now done as a DateTime.
- assert that UtcTimestamp in default format (no custom format string) renders without the +00:00 time-zone suffix. Relates to serilog#164
…standard formating - keep render performance in mind / i.e. keep optimizations from serilog#134 - ensure UtcTimestamp without format string is rendered without " +00:00" suffix by rendering as DateTime (same behavior as Serilog's MessageTemplateTextFormatter) Relates to serilog#164
There was a problem hiding this comment.
Looks great, thanks for the folllow-up @ManuelRin 👍
I think the implementation is good to go; is there any chance of squeezing in a test that checks that "{Timestamp:o}" still renders a +02:00 style ISO-string, while "{UtcTimestamp:o}" renders Z?
Sure, I can do that in the upcoming days. |
…n enhanced - round-trip standard format string added to tests - for Timestamp the +00:00 format is expected, for UtcTimestamp the Z suffix is expected Relates to serilog#164
… Theory to improve maintainability For better maintainability the multi-line strings with several tokens and formats of Fact unit tests are replaced by Theory with InlineData for better maintainability/readability. Relates to serilog#164
|
Apparently I forgot to follow-up on this PR. Now I found the time to finally add the round-trip format string cases to the unit tests as suggested by @nblumhardt. I also switched the Fact tests with multiline input/expected values to Theory tests with individual InlineData. I think this provides better readability and maintainability. Please accept this PR for the next Serilog.Sinks.Console release or let me know if any more updates are necessary. Best, Manuel |
|
Fantastic - thanks! |
|
Hi @nblumhardt, I understand that this made it into the prerelease 6.0.1-dev-00953 on NuGet.org. Is there a roadmap / planned time when this could be merged into the main branch and find its way into a new non-prerelease production release, e.g. 6.0.1 or 6.1.0? I'd appreciate a rough timeline. Thanks! |
|
Thanks for the nudge! Opened #171, will move it through the pipeline ASAP. |
What issue does this PR address?
Addresses #164 Support for Serilog 4.0 new built-in UtcTimestamp token in output template was missing in console sink.
Does this PR introduce a breaking change?
Recognize UtcTimestamp as a built-in token in output templates. May impact scenarios where UtcTimestamp was present as a user defined property. Same effect as in serilog/serilog#2051.
Please check if the PR fulfills these requirements