Skip to content
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.

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

Support UtcTimestamp token in output template (new since Serilog 4.0) #165

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

ManuelRin
Copy link

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

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)


public override void Render(LogEvent logEvent, TextWriter output)
{
var sv = new DateTimeOffsetValue(logEvent.Timestamp);
var timestamp = _convertToUtc
? logEvent.Timestamp.ToUniversalTime()
Copy link
Member

Choose a reason for hiding this comment

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

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:

https://github.com/serilog/serilog/blob/dev/src/Serilog/Formatting/Display/MessageTemplateTextFormatter.cs#L107

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, I'll have a look into that and will address it in an update.

Copy link
Author

Choose a reason for hiding this comment

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

@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
Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

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

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?

@ManuelRin
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants