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

Conversation

Projects
None yet
2 participants
@krajek
Copy link
Contributor

krajek commented Sep 24, 2016

Regarding #565.
I have done just string length limiting. When this will be completed I will move on to collections size limit as discussed in #565.
Things that need to be discussed:
a) default limit - for now I set it to 1000, but I am absolutely confortable to setting no limit by default(or int.MaxValue)
b) Whether we want to allow configuration of "truncation string". For now I used … (U+2026) without configuration.

Ideas are welcomed.

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

This comment has been minimized.

@nblumhardt

nblumhardt Sep 28, 2016

Member

So that this doesn't need to be flagged as a breaking change, perhaps int.MaxValue is a better option here?

/// <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 have to be greater than 1");

This comment has been minimized.

@nblumhardt

nblumhardt Sep 28, 2016

Member

Exception messages in Serilog are all sentences, so needs a . at the end of the message. Perhaps a slight rewording "Maximum string length must be at least two." ?

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Sep 28, 2016

Thanks for picking this one up! Sorry about the slow response, just returned from a few days off.

One thing I think needs to be accounted for is:

Log.Information("The string is {S}", s);

Here, s should probably not be truncated - it's not being captured via the destructuring process that this feature aims to control.

@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Sep 28, 2016

Thanks for the review.
Initial value int.MaxValue is obviously the right choice here, done.
Rewording of exception also makes sense, done.
Regarding strings in Default destructuring mode (string s; SomeObject o):

  • Log.Information("The string is {S}", s); will NOT be limited
  • Log.Information("The string is {@S}", s); will be limited
  • Log.Information("The string is {$S}", s); will be limited
  • Log.Information("The object is {O}", o); will NOT be limited
  • Log.Information("The object is {$O}", o); will be limited
  • Log.Information("The object is {@O}", o); will be limited
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Sep 29, 2016

Shall I continue work on limiting collection size on this branch? Or maybe we can merge this one and open next PR for collections?


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

if (destructuring == Destructuring.Destructure)

This comment has been minimized.

@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.

@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.

@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.

@krajek

krajek Oct 5, 2016

Author Contributor

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

@@ -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.

@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.

@krajek

krajek Oct 5, 2016

Author Contributor

Sure, will be fixed.

@nblumhardt nblumhardt merged commit f29518b into serilog:dev Oct 5, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nblumhardt nblumhardt referenced this pull request Jan 30, 2017

Merged

2.4.0 Release #934

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.