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

Payload limiting #866

Merged
merged 12 commits into from Oct 5, 2016
@@ -27,20 +27,24 @@ public class LoggerDestructuringConfiguration
readonly Action<Type> _addScalar;
readonly Action<IDestructuringPolicy> _addPolicy;
readonly Action<int> _setMaximumDepth;
readonly Action<int> _setMaximumStringLength;

internal LoggerDestructuringConfiguration(
LoggerConfiguration loggerConfiguration,
Action<Type> addScalar,
Action<IDestructuringPolicy> addPolicy,
Action<int> setMaximumDepth)
Action<int> setMaximumDepth,
Action<int> setMaximumStringLength)
{
if (loggerConfiguration == null) throw new ArgumentNullException(nameof(loggerConfiguration));
if (addScalar == null) throw new ArgumentNullException(nameof(addScalar));
if (addPolicy == null) throw new ArgumentNullException(nameof(addPolicy));
if (setMaximumDepth == null) throw new ArgumentNullException(nameof(setMaximumStringLength));
_loggerConfiguration = loggerConfiguration;
_addScalar = addScalar;
_addPolicy = addPolicy;
_setMaximumDepth = setMaximumDepth;
_setMaximumStringLength = setMaximumStringLength;
}

/// <summary>
@@ -149,6 +153,21 @@ public LoggerConfiguration ToMaximumDepth(int maximumDestructuringDepth)
_setMaximumDepth(maximumDestructuringDepth);
return _loggerConfiguration;
}

/// <summary>
/// When destructuring objects, string values can be restricted to specified length
/// thus avoiding bloating payload. Limit is applied to each value separately,
/// sum of length of strings can exceed limit.
/// </summary>
/// <param name="maximumStringLength">The maximum string length.</param>
/// <returns>Configuration object allowing method chaining.</returns>
/// <exception cref="ArgumentOutOfRangeException">When passed length is less or equal to 2</exception>
public LoggerConfiguration ToMaximumStringLength(int maximumStringLength)
{
if (maximumStringLength < 2) throw new ArgumentOutOfRangeException(nameof(maximumStringLength), maximumStringLength, "Maximum string length must be at least two.");
_setMaximumStringLength(maximumStringLength);
return _loggerConfiguration;
}
}
}

@@ -39,6 +39,7 @@ public class LoggerConfiguration
LogEventLevel _minimumLevel = LogEventLevel.Information;
LoggingLevelSwitch _levelSwitch;
int _maximumDestructuringDepth = 10;
int _maximumStringLength = int.MaxValue;
bool _loggerCreated;

void ApplyInheritedConfiguration(LoggerConfiguration child)
@@ -109,7 +110,8 @@ public LoggerDestructuringConfiguration Destructure
this,
_additionalScalarTypes.Add,
_additionalDestructuringPolicies.Add,
depth => _maximumDestructuringDepth = depth);
depth => _maximumDestructuringDepth = depth,
length => _maximumStringLength = length);
}
}

@@ -150,7 +152,12 @@ public Logger CreateLogger()
sink = new FilteringSink(sink, _filters, auditing);
}

var converter = new PropertyValueConverter(_maximumDestructuringDepth, _additionalScalarTypes, _additionalDestructuringPolicies, auditing);
var converter = new PropertyValueConverter(
_maximumDestructuringDepth,
_maximumStringLength,
_additionalScalarTypes,
_additionalDestructuringPolicies,
auditing);
var processor = new MessageTemplateProcessor(converter);

ILogEventEnricher enricher;
@@ -46,20 +46,24 @@ partial class PropertyValueConverter : ILogEventPropertyFactory, ILogEventProper
readonly IDestructuringPolicy[] _destructuringPolicies;
readonly IScalarConversionPolicy[] _scalarConversionPolicies;
readonly int _maximumDestructuringDepth;
readonly int _maximumStringLength;
readonly bool _propagateExceptions;

public PropertyValueConverter(
int maximumDestructuringDepth,
int maximumStringLength,
IEnumerable<Type> additionalScalarTypes,
IEnumerable<IDestructuringPolicy> additionalDestructuringPolicies,
bool propagateExceptions)
{
if (additionalScalarTypes == null) throw new ArgumentNullException(nameof(additionalScalarTypes));
if (additionalDestructuringPolicies == null) throw new ArgumentNullException(nameof(additionalDestructuringPolicies));
if (maximumDestructuringDepth < 0) throw new ArgumentOutOfRangeException(nameof(maximumDestructuringDepth));
if (maximumStringLength < 2) throw new ArgumentOutOfRangeException(nameof(maximumDestructuringDepth));

_maximumDestructuringDepth = maximumDestructuringDepth;
_propagateExceptions = propagateExceptions;
_maximumStringLength = maximumStringLength;

_scalarConversionPolicies = new IScalarConversionPolicy[]
{
@@ -121,11 +125,22 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
return new ScalarValue(null);

if (destructuring == Destructuring.Stringify)
return new ScalarValue(value.ToString());
{
return Stringify(value);
}

var valueType = value.GetType();
var limiter = new DepthLimiter(depth, _maximumDestructuringDepth, this);

if (destructuring == Destructuring.Destructure)

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Sep 30, 2016

Member

One last thought - an interesting implementation option would be to actually create an IScalarConversionPolicy for strings specifically, and (if the limit has been set), insert the policy at the head of the collection of policies. Making insertion of the policy conditional would mean the feature was truly pay-for-play, i.e. if no maximum depth was set, no policy would appear in the list and so runtime cost would be zero (no need for this if block, type comparison, etc.).

The "stringify" case might not fall out nicely from this option, but we could definitely reconsider having it apply there.

A small added bonus would be that using int? for the depth (instead of the MaxValue sentinel) would be possible without having to do a .Value runtime check.

What do you think?

This comment has been minimized.

Copy link
@krajek

krajek Sep 30, 2016

Author Contributor

I tried it and biggest drawback is that if we will use IScalarConversionPolicy as-is we are back to limiting strings in default mode Log.Information("The string is {S}", s);
In my opinion this is good thing, strings would be limited in every case, no special cases.

Also the stringified mode is still going to be handled separately, but limiting logic was extracted to separate class, so no dupication there.

To sum up, I would go for this option. Are you sure you want to keep the requirement of Log.Information("The string is {S}", s); will NOT be limited?

krajek@890f9be
I have not changed limit to int?, it will be trivial.

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 5, 2016

Member

Ah, I see - thanks, yes you're right. We should continue not to limit regular string logging.

Let's merge this one; I'm looking forward to moving onto collection size and so-on.

This comment has been minimized.

Copy link
@krajek

krajek Oct 5, 2016

Author Contributor

Thanks, I will move on to collection limit and come back with next PR.

{
var stringValue = value as string;
if (stringValue != null)
{
value = TruncateIfNecessary(stringValue);
}
}

foreach (var scalarConversionPolicy in _scalarConversionPolicies)
{
ScalarValue converted;
@@ -186,6 +201,23 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
return new ScalarValue(value.ToString());
}

private LogEventPropertyValue Stringify(object value)

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 5, 2016

Member

Nitpick (can pick this up later): the codebase doesn't use any explicit private/internal modifiers.

This comment has been minimized.

Copy link
@krajek

krajek Oct 5, 2016

Author Contributor

Sure, will be fixed.

{
var stringified = value.ToString();
var truncated = TruncateIfNecessary(stringified);
return new ScalarValue(truncated);
}

string TruncateIfNecessary(string text)
{
if (text.Length > _maximumStringLength)
{
return text.Substring(0, _maximumStringLength - 1) + "";
}

return text;
}

bool IsValueTypeDictionary(Type valueType)
{
return valueType.IsConstructedGenericType &&
@@ -241,3 +273,4 @@ internal static bool IsCompilerGeneratedType(Type type)
}
}
}

@@ -179,7 +179,7 @@ static IEnumerable<LogEventProperty> Capture(string messageTemplate, params obje
{
var mt = new MessageTemplateParser().Parse(messageTemplate);
var binder = new PropertyBinder(
new PropertyValueConverter(10, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false));
new PropertyValueConverter(10, 1000, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false));
return binder.ConstructProperties(mt, properties);
}

@@ -126,7 +126,7 @@ static string Render(string messageTemplate, params object[] properties)
static string Render(IFormatProvider formatProvider, string messageTemplate, params object[] properties)
{
var mt = new MessageTemplateParser().Parse(messageTemplate);
var binder = new PropertyBinder(new PropertyValueConverter(10, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false));
var binder = new PropertyBinder(new PropertyValueConverter(10, 1000, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false));
var props = binder.ConstructProperties(mt, properties);
var output = new StringBuilder();
var writer = new StringWriter(output);
@@ -27,7 +27,7 @@ namespace Serilog.Tests.Events
public class LogEventPropertyValueTests
{
readonly PropertyValueConverter _converter =
new PropertyValueConverter(10, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false);
new PropertyValueConverter(10, 1000, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false);

[Fact]
public void AnEnumIsConvertedToANonStringScalarValue()
@@ -3,6 +3,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices.ComTypes;
using Xunit;
using Serilog.Core;
using Serilog.Core.Filters;
@@ -277,6 +278,153 @@ public void MaximumDestructuringDepthIsEffective()
Assert.DoesNotContain(xs, "D");
}

[Fact]
public void MaximumStringLengthThrowsForLimitLowerThan2()
{
var ex = Assert.Throws<ArgumentOutOfRangeException>(
() => new LoggerConfiguration().Destructure.ToMaximumStringLength(1));
Assert.Equal(1, ex.ActualValue);
}

[Fact]
public void MaximumStringLengthNOTEffectiveForString()
{
var x = "ABCD";

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(3)
.CreateLogger();

log.Information("{X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Equal("\"ABCD\"", limitedText);
}

[Fact]
public void MaximumStringLengthEffectiveForCapturedString()
{
var x = "ABCD";

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(3)
.CreateLogger();

log.Information("{@X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Equal("\"AB…\"", limitedText);
}

[Fact]
public void MaximumStringLengthEffectiveForStringifiedString()
{
var x = "ABCD";

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(3)
.CreateLogger();

log.Information("{$X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Equal("\"AB…\"", limitedText);
}

[Theory]
[InlineData("1234", "12…", 3)]
[InlineData("123", "123", 3)]
public void MaximumStringLengthEffectiveForCapturedObject(string text, string textAfter, int limit)
{
var x = new
{
TooLongText = text
};

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(limit)
.CreateLogger();

log.Information("{@X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Contains(textAfter, limitedText);
}

[Fact]
public void MaximumStringLengthEffectiveForStringifiedObject()
{
var x = new ToStringOfLength(4);

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(3)
.CreateLogger();

log.Information("{$X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Contains("##…", limitedText);
}

[Fact]
public void MaximumStringNOTLengthEffectiveForObject()
{
var x = new ToStringOfLength(4);

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(3)
.CreateLogger();

log.Information("{X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Contains("####", limitedText);
}

[Fact]
public void MaximumStringLengthNOTEffectiveForObject()
{
var x = new ToStringOfLength(4);

LogEvent evt = null;
var log = new LoggerConfiguration()
.WriteTo.Sink(new DelegatingSink(e => evt = e))
.Destructure.ToMaximumStringLength(3)
.CreateLogger();

log.Information("{X}", x);
var limitedText = evt.Properties["X"].ToString();

Assert.Contains("####", limitedText);
}

class ToStringOfLength
{
private int _toStringOfLength;

public ToStringOfLength(int toStringOfLength)
{
_toStringOfLength = toStringOfLength;
}

public override string ToString()
{
return new string('#', _toStringOfLength);
}
}

[Fact]
public void DynamicallySwitchingSinkRestrictsOutput()
{
@@ -13,7 +13,7 @@ namespace Serilog.Tests.Parameters
public class PropertyValueConverterTests
{
readonly PropertyValueConverter _converter =
new PropertyValueConverter(10, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false);
new PropertyValueConverter(10, 1000, Enumerable.Empty<Type>(), Enumerable.Empty<IDestructuringPolicy>(), false);

[Fact]
public void UnderDestructuringAByteArrayIsAScalarValue()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.