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

Destructure support #110

Merged
merged 3 commits into from
May 14, 2018
Merged

Destructure support #110

merged 3 commits into from
May 14, 2018

Conversation

MV10
Copy link
Contributor

@MV10 MV10 commented May 6, 2018

Added support for destructure With and max-limit settings, also opening up destructuring features to additional configuration through extension methods in other packages, as discussed in #106.

  • ConfigurationReader:
    • add ApplyDestructuring
    • add FindDestructureConfigurationMethods
    • added surrogates for several destructure config methods
    • add comments explaining method-level surrogates
    • renamed FindConfigurationMethods to FindConfigurationExtensions
  • three unit tests modeled on Serilog's own destructure tests
  • README updated with a few lines of JSON demonstrating the settings
  • changes to sample program
    • config for Destructure max string length and max collection depth
    • log events to demonstrate Destructure config
    • comment explaining Filter syntax is from S.F.Expressions package

Update

  • renamed FindConfigurationExtensions toFindConfigurationExtensionMethods
  • added IDestructuringPolicy example to sample program and README docs

if (configurationAssemblies.Contains(typeof(LoggerEnrichmentConfiguration).GetTypeInfo().Assembly))
found.Add(GetSurrogateConfigurationMethod<LoggerEnrichmentConfiguration, object, object>((c, _, __) => FromLogContext(c)));

return found;
}

internal static IList<MethodInfo> FindConfigurationMethods(IReadOnlyCollection<Assembly> configurationAssemblies, Type configType)
internal static IList<MethodInfo> FindConfigurationExtensions(IReadOnlyCollection<Assembly> configurationAssemblies, Type configType)
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking : Maybe FindConfigurationExtensionMethods instead of FindConfigurationExtensions is clearer (extensions are not a thing in C#, but extension methods are :p)

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 considered the longer name, but hadn't considered the point you raise. I'll make the change (but will wait a bit for other feedback). Thanks.

@tsimbalar
Copy link
Member

There is currently an open issue in Serilog.Settings.AppSettings for support of .Destructure.* methods : serilog/serilog-settings-appsettings#20 .

It would be good to try and have the same level of support for Destructuring in both settings providers.

@MV10
Copy link
Contributor Author

MV10 commented May 7, 2018

@tsimbalar Since you referenced the AppSettings thread, for anyone doing work there I'll add this to illustrate how the destructuring section looks for this Configuration package. (This is in this PR's README but it's more discoverable here while this PR is in-flight.)

Within the Serilog section:

    "Destructure": [
      { "Name": "With", "Args": { "policy": "Sample.CustomPolicy, Sample" } },
      { "Name": "ToMaximumDepth", "Args": { "maximumDestructuringDepth": 4 } },
      { "Name": "ToMaximumStringLength", "Args": { "maximumStringLength": 100 } },
      { "Name": "ToMaximumCollectionCount", "Args": { "maximumCollectionCount": 10 } }
    ],

@MV10
Copy link
Contributor Author

MV10 commented May 14, 2018

@tsimbalar or @nblumhardt Can we get this one wrapped up? Not sure who can merge here...

@tsimbalar
Copy link
Member

Looks good to me !

I have updated my "documentation" repo with the nuget package from this PR to validate the expected behavior and it worked like a charm : https://github.com/tsimbalar/serilog-settings-comparison/blob/master/docs/README.md#custom-destructuring 🎉

I'll merge it !

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