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

Optimize AddPropertyIfAbsent #1872

Merged
merged 6 commits into from
May 3, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Serilog/Capturing/DepthLimiter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public LogEventPropertyValue CreatePropertyValue(object? value, Destructuring de
return result;
}

LogEventPropertyValue ILogEventPropertyValueFactory.CreatePropertyValue(object value, bool destructureObjects)
LogEventPropertyValue ILogEventPropertyValueFactory.CreatePropertyValue(object? value, bool destructureObjects)
{
var storedDepth = _currentDepth;

Expand Down
2 changes: 1 addition & 1 deletion src/Serilog/Capturing/MessageTemplateProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

namespace Serilog.Capturing;

class MessageTemplateProcessor : ILogEventPropertyFactory
class MessageTemplateProcessor : ILogEventPropertyFactory, ILogEventPropertyValueFactory
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this interface was just missed.

{
readonly MessageTemplateCache _parser = new(new MessageTemplateParser());
readonly PropertyBinder _propertyBinder;
Expand Down
5 changes: 2 additions & 3 deletions src/Serilog/Core/Enrichers/PropertyEnricher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class PropertyEnricher : ILogEventEnricher
/// </summary>
/// <param name="name">The name of the property.</param>
/// <param name="value">The value of the property.</param>
/// <param name="destructureObjects">If true, and the value is a non-primitive, non-array type,
/// <param name="destructureObjects">If <see langword="true"/>, and the value is a non-primitive, non-array type,
/// then the value will be converted to a structure; otherwise, unknown types will
/// be converted to scalars, which are generally stored as strings.</param>
/// <exception cref="ArgumentNullException">When <paramref name="name"/> is <code>null</code></exception>
Expand All @@ -54,7 +54,6 @@ public void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory)
Guard.AgainstNull(logEvent);
Guard.AgainstNull(propertyFactory);

var property = propertyFactory.CreateProperty(_name, _value, _destructureObjects);
logEvent.AddPropertyIfAbsent(property);
logEvent.AddPropertyIfAbsent(propertyFactory, _name, _value, _destructureObjects);
}
}
2 changes: 1 addition & 1 deletion src/Serilog/Core/ILogEventPropertyValueFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ public interface ILogEventPropertyValueFactory
/// then the value will be converted to a structure; otherwise, unknown types will
/// be converted to scalars, which are generally stored as strings.</param>
/// <returns>The value.</returns>
LogEventPropertyValue CreatePropertyValue(object value, bool destructureObjects = false);
LogEventPropertyValue CreatePropertyValue(object? value, bool destructureObjects = false);
sungam3r marked this conversation as resolved.
Show resolved Hide resolved
}
19 changes: 18 additions & 1 deletion src/Serilog/Events/LogEvent.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2013-2015 Serilog Contributors
// Copyright 2013-2015 Serilog Contributors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -107,6 +107,8 @@ public string RenderMessage(IFormatProvider? formatProvider = null)
/// </summary>
public IReadOnlyDictionary<string, LogEventPropertyValue> Properties => _properties;

internal Dictionary<string, LogEventPropertyValue> PropertiesDictionary => _properties;

/// <summary>
/// An exception associated with the event, or null.
/// </summary>
Expand Down Expand Up @@ -192,3 +194,18 @@ internal LogEvent Copy()
properties);
}
}

internal static class LogEventExtensions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nblumhardt For now it's internal but it may be changed to public for use in dependent packages. For example, in serilog-extensions-logging, see EnrichAndCreateScopeItem:

var property = propertyFactory.CreateProperty(key, value, destructureObject);
logEvent.AddPropertyIfAbsent(property);

{
public static void AddPropertyIfAbsent(this LogEvent evt, ILogEventPropertyFactory factory, string name, object? value, bool destructureObjects = false)
Copy link
Member

Choose a reason for hiding this comment

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

Since we already end up adding an internal property to LogEvent, we could just skip that and make this overload of AddPropertyIfAbsent an internal method on LogEvent itself, for now?

I can see how dependent libraries would benefit from making this accessible, but in that case we'd have to expose something on LogEvent anyway, so we might as well keep everything tight/encapsulated for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
if (!evt.Properties.ContainsKey(name))
{
evt.PropertiesDictionary.Add(
name,
factory is ILogEventPropertyValueFactory factory2
? factory2.CreatePropertyValue(value, destructureObjects)
: factory.CreateProperty(name, value, destructureObjects).Value);
}
}
}
2 changes: 1 addition & 1 deletion src/Serilog/Log.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,7 @@ public static void Fatal(Exception? exception, string messageTemplate, params ob
/// <param name="property">The resulting property.</param>
/// <returns>True if the property could be bound, otherwise false (<summary>ILogger</summary>
/// methods never throw exceptions).</returns>
public static bool BindProperty(string propertyName, object value, bool destructureObjects, [NotNullWhen(true)] out LogEventProperty? property)
public static bool BindProperty(string propertyName, object? value, bool destructureObjects, [NotNullWhen(true)] out LogEventProperty? property)
{
return Logger.BindProperty(propertyName, value, destructureObjects, out property);
}
Expand Down
4 changes: 2 additions & 2 deletions test/Serilog.ApprovalTests/Serilog.approved.txt
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ namespace Serilog.Core
}
public interface ILogEventPropertyValueFactory
{
Serilog.Events.LogEventPropertyValue CreatePropertyValue(object value, bool destructureObjects = false);
Serilog.Events.LogEventPropertyValue CreatePropertyValue(object? value, bool destructureObjects = false);
}
public interface ILogEventSink
{
Expand Down Expand Up @@ -629,7 +629,7 @@ namespace Serilog
public static Serilog.ILogger Logger { get; set; }
[Serilog.Core.MessageTemplateFormatMethod("messageTemplate")]
public static bool BindMessageTemplate(string messageTemplate, object?[] propertyValues, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out Serilog.Events.MessageTemplate? parsedTemplate, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out System.Collections.Generic.IEnumerable<Serilog.Events.LogEventProperty>? boundProperties) { }
public static bool BindProperty(string propertyName, object value, bool destructureObjects, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out Serilog.Events.LogEventProperty? property) { }
public static bool BindProperty(string propertyName, object? value, bool destructureObjects, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out Serilog.Events.LogEventProperty? property) { }
public static void CloseAndFlush() { }
public static System.Threading.Tasks.ValueTask CloseAndFlushAsync() { }
[Serilog.Core.MessageTemplateFormatMethod("messageTemplate")]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
namespace Serilog.PerformanceTests;

[MemoryDiagnoser]
public class LogContextEnrichmentBenchmark
{
ILogger _bare = null!, _enriched = null!;
Expand Down