Add option to set template as body #104
Conversation
danelson
force-pushed
the
include-data-template-body
branch
from
91c7bb8
to
ec85412
Compare
| var logRecord = LogRecordBuilder.ToLogRecord(Some.SerilogEvent(), null, IncludedData.TemplateBody, new()); | ||
| Assert.NotNull(logRecord.Body); | ||
| Assert.Equal(Some.TestMessageTemplate, logRecord.Body.StringValue); | ||
| } |
There was a problem hiding this comment.
Perhaps for the test name, TemplateBodyIncludesMessageTemplateInBody()? (A bit painfully verbose but spells out the goal.)
For the Some.SerilogEvent(), the test could end up inconclusive if the sample event has a template with no holes (so the test wouldn't be able to tell the difference between the template and the rendered message). Does the SerilogEvent() method allow specifying the template, e.g.:
Some.SerilogEvent("Hello, {Name}", "world")
or could we find a way to ensure this?
There was a problem hiding this comment.
Do you mean to be explicit about the template? Some.SerilogEvent() defines a default template of "Message template {Variable}". If the default was changed to something like Message template then this test would not fail if the underlying implementation "broke". Is that what you are getting at?
In my opinion Some.SerilogEvent() is a bit odd because it lets you override the template but not the properties. I am thinking the best way to handle this might be to refactor the tests a bit. Something like
- internal static LogEvent SerilogEvent(DateTimeOffset? timestamp = null, Exception? ex = null, string messageTemplate = TestMessageTemplate)
+ internal static LogEvent DefaultSerilogEvent()
+ internal static LogEvent SerilogEvent(DateTimeOffset? timestamp = null, Exception? ex = null, string messageTemplate, IEnumerable<LogEventProperty> properties)Thoughts?
There was a problem hiding this comment.
That looks good to me 👍
(Sorry about the slow reply, spent a few nights away over the weekend, just catching up now 😅 )
There was a problem hiding this comment.
Made some updates, let me know what you think.
| { | ||
| logRecord.Body = new AnyValue | ||
| { | ||
| StringValue = renderedMessage | ||
| }; | ||
| } | ||
|
|
||
| if (includedFields.HasFlag(IncludedData.TemplateBody) && logEvent.MessageTemplate.Text.Trim() != "") |
There was a problem hiding this comment.
else if would correctly reflect the logic here
| ProcessLevel(logRecord, logEvent); | ||
| ProcessException(logRecord, logEvent); | ||
| ProcessIncludedFields(logRecord, logEvent, includedFields, activityContextCollector); | ||
|
|
||
| return logRecord; | ||
| } | ||
|
|
||
| public static void ProcessMessage(LogRecord logRecord, LogEvent logEvent, IFormatProvider? formatProvider) | ||
| public static void ProcessMessage(LogRecord logRecord, LogEvent logEvent, IncludedData includedFields, IFormatProvider? formatProvider) | ||
| { | ||
| var renderedMessage = CleanMessageTemplateFormatter.Format(logEvent.MessageTemplate, logEvent.Properties, formatProvider); |
There was a problem hiding this comment.
Rendering of the message should move inside the first if block, since it allocates/requires cycles that are wasted when the rendered message is not included (the first arm of the if block will need a nested if (renderedMessage.Trim()) inside it - but the noise seems worth it for the improvement in behavior).
|
Since this adds a feature, it should probably also bump the |
Alternative to #101