Skip to content

Specifying .Destructure.ByTransforming<T>() in the config #106

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

Closed
zippy1981 opened this issue Apr 30, 2018 · 39 comments
Closed

Specifying .Destructure.ByTransforming<T>() in the config #106

zippy1981 opened this issue Apr 30, 2018 · 39 comments

Comments

@zippy1981
Copy link

I have migrated recently from defining Serilog config in code to appsettings.json.

I would like to be able to specify things like .Destructure.ByTransforming<CultureInfo>(c => new { c.LCID, c.Name }). Even simplified syntax would be fine.

@MV10
Copy link
Contributor

MV10 commented Apr 30, 2018

I think it would have to be new config syntax specifically customized for this purpose. Regardless of the sources used to build the final IConfiguration, every value ends up as a string (or a collection, which, if you drill down further, will end up as string values). I have not been able to find a good, concise way to represent delegates (Serilog uses a Func but essentially the same thing) in a string form that can later be loaded in an executable form (short of actually compiling it, which seems extreme). I ran into this problem trying to adapt the Serilog SQL Sink's property-filter option which also takes a delegate. (If you want to see really ugly code, go look up a serialized delegate and/or expression tree... similar to the near-gibberish you get in a stack trace when something complicated like async LINQ goes wrong.)

I think it would require a whole new top level section (like Using), then perhaps a list of the types to transform, each containing a list of the type members to copy into the new (transformed) instance. I haven't looked at how Serilog handles destructuring (except during config) but it sounds like a huge amount of reflection work to me. I suspect it would require a new generic class in this project that would carry the type info resulting from the config to set itself up as the delegate.

I doubt there would be a good way to capture the "where" condition for ByTransformingWhere as a config element.

@MV10
Copy link
Contributor

MV10 commented May 1, 2018

After thinking about this a lot more, I'm pretty sure I can populate Func<T,U> arguments, but it will require a Rosyln dependency which might be more overhead than is appropriate for this package (so don't get too excited). I'll do some experimentation.

@nblumhardt Nick, opinions? For Serilog in general, I'm not completely sure how broadly useful Func argument handling might be. Mostly right now it seems like a fun thing to try (in the "Hold my beer, watch this!" sense).

@nblumhardt
Copy link
Member

I think this is getting beyond what we want to attempt using config. (Definitely in the "hold my beer" category :-D )

Supporting Destructure.WithMyCustomPolicy() in config seems like a saner route - aligned with how we plug in sinks, enrichers and so-on. Not sure what level of support we currently have for it, though.

@MV10
Copy link
Contributor

MV10 commented May 1, 2018

Are you referring to With<TDestructuringPolicy>?

I do feel like I see a lot of Func usage, though. Just not sure how often it comes into play for config. Joking aside I don't think it would be too hard. I just ran a quick test build and Roslyn is a lot tighter than I'd expected.

@nblumhardt
Copy link
Member

Are you referring to With?

No, the config system works based on extension methods (for better or worse :-)), so Destructure.WithMyCustomPolicy() is mechanically the same as WriteTo.MyCustomSink().

@nblumhardt
Copy link
Member

I just ran a quick test build and Roslyn is a lot tighter than I'd expected.

Sounds interesting, but still seems like more than we'd want to take on, here. Perhaps a middle ground would be to create an extension library to support:

// `ByTransforming()` is an extension method in a separate lib,
// that accepts C# and invokes Roslyn...
Destructure.ByTransforming("... some C# code...")

This would allow integration into config using the standard mechanism. We already do this for Serilog.Filters.Expressions - check out the README/code there to see how it all fits together (pretty neatly!)

@MV10
Copy link
Contributor

MV10 commented May 1, 2018

Where is that extension method?

@nblumhardt
Copy link
Member

@MV10
Copy link
Contributor

MV10 commented May 1, 2018

Interesting. I was actually considering a Roslyn-based... ah just saw your other reply -- yes, an extension library is what I was sort of working my way towards. But I'm not quite clear how it would integrate with configuration without Func support from this project's ObjectArgumentValue class that turns the config data into the type required by the config argument.

I'll have to study the expression project more. This level of flexibility is very intriguing to me.

@MV10
Copy link
Contributor

MV10 commented May 2, 2018

@nblumhardt This afternoon I whipped up a quick-and-dirty extension for Filter.ByIncludingOnly that parses and compiles a matching expression via Roslyn. Pretty much any Func-driven configuration point could be extended in this fashion. There is also a Destructure extension in there but I haven't written anything in the sample code for it yet.

https://github.com/MV10/serilog-extensions-roslyn

There are two things I don't like.

There are a few seconds of startup overhead to grab all the loaded assemblies and namespaces. It probably wouldn't be a big deal in a long-running system like a web app, and for performance-sensitive scenarios, I have ideas to optimize this by adding overloads as well as setup methods that would allow users to provide lists of Types to reference (I think I could build what I need from there).

I also don't like using GetAwaiter().GetResults() but I don't want to force anyone to mutate their entire code base to async/await. Probably if this were a real project I'd just add Task variants with the conventional Async suffix on the method names and let the consumers choose.

The Roslyn part is remarkably simple (which is amazing to me).

Edit - The other thing I'll try tomorrow is populating the filter expression with this Config package, but it's just a string so there really isn't any reason it shouldn't work.

@nblumhardt
Copy link
Member

@MV10 that's very slick :-)

If you decide to keep pushing it towards "production" usability, we can link it from the "community projects" wiki page: https://github.com/serilog/serilog/wiki/Community-Projects

@zippy1981
Copy link
Author

This is really awesome @MV10 I just might end up using that.

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

@zippy1981 Plenty of work to do but it should do the trick.

@nblumhardt I'm definitely planning to forge ahead with this one as soon as I get that SQL PR wrapped up.

It's really only useful in the context of configuration (I can't see any point in writing a lambda in a string for config-by-code), and I'm not sure naming by technique (Roslyn) is better than naming for intent. I'm thinking maybe Serilog.Settings.Delegates instead.

If you like, I'd be happy to turn it over to the Serilog project in general, I don't feel any particular need to personally own it in perpetuity (though I'd stay involved under Serilog as well).

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

I suppose this also means I'll be adding ApplyDestructure to the IConfiguration portion of this config package. (Edit ... which probably also argues for making this an official Serilog package.)

@tsimbalar
Copy link
Member

Not sure if this is related and may find some commonality, but I have a repo in here, Serilog.Settings.Code, where I load the config as C# from an arbitrary file (typically a .csx file) ... in case that helps :)

It is based on Microsoft.CodeAnalysis.CSharp.Scripting which itself is based on Roslyn.

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

@tsimbalar That's a cool idea.

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

Confession time... I was surprised the predicate in Destructure.ByTransformingWhere takes a Type as input. I can't seem to find any examples of usage and the purpose isn't clear to me. (I expected the predicate input would also be TValue to make a decision based on the content of the object being logged.)

Hmm, is this perhaps a way to reference a static switch on the input Type?

Even more surprising to me, returning true throws an exception. My test mimics the usual example of logging user-login and stripping a password value. The non-predicate transform works great. If the predicate naively returns false (as below, no conditionals, just burp up a bool) the transformation policy is not applied and the full account data is written, but when the predicate returns true it throws Capturing the property value threw an exception: InvalidCastException. (To be clear, this isn't using config/Roslyn at all in this case, I was trying to set up configuration through code just to figure out how it works.)

What am I missing here? 😕

Log.Logger = new LoggerConfiguration()
    .Destructure.ByTransformingWhere<Account>(
        t => false,
        a => new { a.id, a.Username, a.AccountType })
    .WriteTo.Console().CreateLogger();
Log.Information("Login for {@User}", acct);

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

I would still like clarification about ByTransformingWhere as described above, but our "Hold my beer, watch this" moment has arrived -- destructuring works. Also the Filter entries are loaded from a config file in the updated example. New repo:

https://github.com/MV10/serilog-settings-delegates

An example (configured in code since the Serilog Config package doesn't support destructing yet):

string transType = "Sample.Account";
string trans = "a => new { a.id, a.Username, a.AccountType }";
string pred = "t => false";

Log.Logger = new LoggerConfiguration()
    .Destructure.ByTransformingWhere(pred, transType, trans)
    .WriteTo.Console().CreateLogger();

DestructureExamples_GenerateLogs();

One annoying side-effect I'm researching is that it was necessary to resort to dynamic because C# doesn't allow runtime type declaration. There is this hairy bit of reflection:

private static dynamic EvaluateFuncTValueObject(string transformedType, string transformation)
{
    Type TValue = ReflectionHelper.IncludeType(transformedType);
    Type funcType = typeof(Func<,>).MakeGenericType(new Type[] { TValue, typeof(object) });
    var evalMethod = typeof(CSharpScript).GetMethods()
        .FirstOrDefault(m => m.Name.Equals("EvaluateAsync") && m.IsGenericMethod)
        .MakeGenericMethod(funcType);
    dynamic compiledFunc = evalMethod.InvokeAsync(null, new object[] { transformation, ReflectionHelper.scriptOptions, null, null, null }).GetAwaiter().GetResult();
    return compiledFunc;
}

In a perfect world, the next-to-last line would be:

funcType compiledFunc = evalMethod.InvokeAsync(...)

...but that results in a 'funcType' is a variable but is used like a type error. Returning a dynamic reference works, except that the CLR adds a $type property which Serilog dutifully emits. (People ask about this same issue regularly in relation to serialization, here for example.) Unfortunately Convert.ChangeType isn't an option because Func doesn't have that conversion interface, for obvious reasons.

As a result you get junk like this in the output (the first is with a predicate returning false, the second is with destructuring applied):

image

image

Finally, I experimented with an optional list of types to reference/import, but it was only a tiny fraction of a second faster than just referencing all loaded assemblies and namespaces.

@tsimbalar
Copy link
Member

@MV10 the InvalidCastException issue looks very similar to serilog/serilog#1061 ... it may be the expected behavior, but the API is not so intuitive in there

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

@tsimbalar That does look related, but now I'm even more confused ... if I'm following that discussion properly, it seems like the only predicate that makes sense would be something like type => typeof(Type).IsAssignableFrom(type) which leads me to wonder why it's a parameter versus baked in.

I'd been assuming Destructure.ByTransforming would only be called for the target type, too, which I think is what sleemer is saying at the end of the thread:

I expected that when you use ByTransforming it would be called only for types T

Edit: Also, if that's what ByTransformingWhere is supposed to do, it isn't going to work with dynamic.

@adamchester
Copy link
Member

As the creator of ByTransformingWhere, I can confirm the intent was to allow processing types “assignable from” (and especially reflection types like System.Type).

I can also confirm it’s a confusing method signature, and I am very open to proposals for a better way.

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

@adamchester I'm still trying to understand what purpose it serves, though. Does the non-where version just call the transformation on any type (and, I suppose, fail silently unless the type happens to match)?

Ah... looks like I need to digest this serilog/serilog#730.

@adamchester
Copy link
Member

adamchester commented May 3, 2018

The purpose it serves is to control the type matching predicate (“where”).

The non-where version always does an EXACT type match, and so you can never match reflection objects like System.Type.

If this is confusing, try to destructure something like this:

Log.Information(“The type was {@Type}”, typeof(String));

See if you ever make the destructure configuration “match” this.

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

Thanks for the clarification. I think I follow. Maybe the method name could have clarified this somewhat (perhaps ByTransformingReflectionType or some such -- I realize it isn't necessarily only reflection but realistically that's what it's all about). I suppose it's too late for to go refactoring names, but definitely hard to grok from the signature. Is it documented somewhere? I did a lot of searching but came up empty-handed.

I don't think a dynamic could ever work here so I'm thinking my extension of that should just be dropped.

@adamchester
Copy link
Member

@MV10 what about this?

void Main() {

    using (var logger = new LoggerConfiguration()
        // .Destructure.ByTransforming<dynamic>(d => d.ToPrettyJson())
        .Destructure.ByTransformingWhere<dynamic>(predicate: IsDynamic, transformation: dy => dy.ToPrettyJson())
        .WriteTo.Console()
        .CreateLogger())
    {
        dynamic MyDynamic = new System.Dynamic.ExpandoObject();
        MyDynamic.P1 = "value of property 1";
        MyDynamic.P2 = "value of property 2";
        MyDynamic.P3 = 3;
        logger.Information("test {@Object}", MyDynamic);
       // => [13:22:53 INF] test [{"Key": "P1", "Value": "value of property 1", "$type": "KeyValuePair`2"}, {"Key": "P2", "Value": "value of property 2", "$type": "KeyValuePair`2"}, {"Key": "P3", "Value": 3, "$type": "KeyValuePair`2"}]
    }
}

static bool IsDynamic(Type t)
{
    return t.GetCustomAttribute(typeof(System.Runtime.CompilerServices.DynamicAttribute)) != null;
}

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

@adamchester I recall reading that an expando, an anonymous dynamic, and a dynamic wrapping a strong type are very different.

Hmm, maybe I don't understand the purpose after all. It doesn't look to me like dy.ToPrettyJson() is ever called. Your commented output is exactly what you'd get with {@Object} without any destructuring at all. I haven't referenced whatever package has pretty-json but if I do some other destrcturing (like new { n.P2 }) it doesn't happen with the code above.

Whatever I do, I always get the full output, just like your comment:

[{"Key": "P1", "Value": "value of property 1", "$type": "KeyValuePair2"}, {"Key": "P2", "Value": "value of property 2", "$type": "KeyValuePair2"}, {"Key": "P3", "Value": 3, "$type": "KeyValuePair2"}]

Going back to your earlier String example, I get a namespace from the following, so I partially get it. Just having trouble seeing how to combine this with a dynamic (particularly if I'm remembering correctly that dynamic-wrapping-a-type is it's own weird thing).

Log.Logger = new LoggerConfiguration()
    .Destructure.ByTransformingWhere<Type>(
        predicate: t => typeof(Type).IsAssignableFrom(t), 
        transformation: a => new { a.Namespace })
    .WriteTo.Console().CreateLogger();

Log.Information("The type was {@Type}", typeof(String));

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

Ah, this combination works... dynamic as the generic type, but since it does wrap a concrete type (even if it's just Type) the CLR is able to convert so it's treated as assignable. Neat. So I guess the solution is to figure out how to always assign dynamic as the generic type specifier. And that should be "interesting" since typeof(dynamic) isn't a real thing.

Log.Logger = new LoggerConfiguration()
    .Destructure.ByTransformingWhere<dynamic>(
        predicate: t => typeof(Type).IsAssignableFrom(t),
        transformation: n => new { n.Namespace })
    .WriteTo.Console().CreateLogger();

Log.Information("The type was {@Type}", typeof(String)); // this will match (will apply transformation)
DestructureExamples_GenerateLogs(); // will not match (no transformation)

@adamchester
Copy link
Member

adamchester commented May 4, 2018

Sorry for the bad example, I should have verified it more before posting. The point I was attempting to make was showing a usage of the method ByTransformingWhere<T> which wasn't using reflection types and wasn't based on typeof(Type).IsAssignableFrom. Perhaps this example will demonstrate better what other possibilities there are:

void Main()
{
    using (var logger = new LoggerConfiguration()
        .Destructure.ByTransformingWhere<dynamic>(
            predicate: QuacksLikeAVersion,
            transformation: dy => new
            {
                Version = $"{dy.Major}.{dy.Minor}.{dy.Build}",
                OriginalTypeName = dy.GetType().Name as string,
            })
        .WriteTo.Console(outputTemplate: " * {Message}{NewLine}")
        .CreateLogger())
    {
        logger.Information("Does it quack like a version? {@Object}", new { Build = 1, Major = 1, Minor = 1 });
        logger.Information("Does it quack like a version? {@Object}", new Version(1, 2, 3, 4));
        logger.Information("Does it quack like a version? {@Object}", DBNull.Value);
        logger.Information("Does it quack like a version? {@Object}", new VersionQuacker());
    }
}

static bool QuacksLikeAVersion(Type theType)
{
    return new[] { "Build", "Major", "Minor" }.All(n => theType.GetProperties().Any(p => p.Name == n));
}

class VersionQuacker
{
    public int Major { get; } = 2;
    public int Minor { get; } = 3;
    public int Build { get; } = 1;
}

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

Ten thumbs-up for what I assume is an awesome duck-typing pun. Seriously though, that's crystal-clear and pretty cool. Thank you. One of us should PR that into the Serilog wiki topic on destructuring!

Now digging into the arcane guts of generics type-array storage trying to figure out how to trick MakeGenericType into accepting the equivalent of <dynamic> through reflection. (Ugh.)

@zippy1981
Copy link
Author

Should I close this issue or keep this open under the assumption that what @MV10 is writing is a prototype for a PR into this repo?

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

Sorry, I feel a bit like I hijacked your thread, but personally I'd like to see it remain open. The use-case you asked about is already working. I don't know where this will lead, but I feel enough pieces are already working that it's worth more of my time. Besides, the wife's out of town this weekend, it'll keep me out of the bars.

The wrap-up of much of the recent discussion is that I strongly doubt ByTransformingWhere<dynamic> is possible. There is too much black-box magic around dynamic in C#. It appears impossible to use reflection to construct a type representing Func<dynamic, object> -- I've tried many, many different approaches but it always ends up as Func<object, object>, meaning it lacks the dynamic trickery we need for transformation. I'm currently trying to make concrete-typed ByTransformingWhere work properly.

Edit for posterity: It turns out dynamic turns into a bunch of reflection-like IL into the separate Dynamic Language Runtime, which is definitely not accessible via reflection. So that question is definitively settled as far as I'm concerned. (No, I'm not about to bust into emitting IL... not enough beers in the world for that. Not today, anyway.)

But as I said earlier, ByTransforming<T> is definitely already working (if you can live with the extra $type property), and both types of expression-based Filter methods are also working cleanly. I haven't really looked into Sink and Audit config yet to determine whether there are delegated arguments which this approach could service.

@zippy1981
Copy link
Author

@MV10 I have not had enough time to Digest the code or comments here that you wrote. I really appreciate what you did. I'm ok with you leaving this open. Just curious as to what the end game is.

I asked if something is possible, you wrote a thing to make it possible. I'll probably use that thing. However, this is my first go round with Serilog, and the logging changes I made showed a lot of things to me.

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

The end-game is up to Nick. The Filter delegates work with this Configuration repo today, but if he thinks there is enough value in what I'm doing, I'd have to update the Configuration repo to support Destructure.ByTransforming (and all the other destrcture config items, which is easy and I'm willing to do). To me, that step argues for making this delegates system part of Serilog. (I'd still like to see it become part of the Config package, it's a relatively small bit of code, although Nick hasn't warmed up to that idea).

The other thing I'd like to see is Serilog itself filter out that ugly $type property on dynamic types but I'm not sure of the impact, I haven't really looked closely at the code where that would happen.

As for catching up, I wouldn't waste too much time on all my blather (much of it was Adam very patiently helping me wrap my head around the predicate-based transformation method). One way or another (either as part of Serilog or in my own repo) I'll end up documenting it.

@MV10
Copy link
Contributor

MV10 commented May 4, 2018

@nblumhardt I've done all I can in terms of exploring this capability. Sink and Audit configs don't have anything that would benefit from this right now. My repo is up to date and has several new examples.

Recap:

  • The code works with Serilog.Settings.Configuration for Filter.ByIncludingOnly and ByExcluding
  • Destructure.ByTransforming and ByTransformingWhere works (for now, only if configured from code)
  • Destructure support using configuration requires updating Serilog.Settings.Configuration
  • Adding destructure Config support implies a package dependency... or adding the code directly

Arguments for adding the code directly:

  • There is very little code involved (extensions not yet in Serilog Config, plus ~50 lines of helper code)
  • It would make Serilog.Settings.Configuration nearly feature-complete in terms of config capability
  • Adds no execution overhead, only one-time setup delay if used (tested ~4 sec under ASP.NET Core)
  • It will become relatively easy to support Func configurations for any future Serilog packages
  • Destructure has no Config support today

Nick, I'm happy to do a proof-of-concept fork of Serilog.Settings.Configuration if you're willing to consider a PR adding this. The real thing would need a little extra try/catch work, tests, etc. but the hard work is done.

Filtering examples using configuration:

{
  "Serilog": {
    "Using": [ "Serilog.Sinks.Console" ],
    "WriteTo": [ "Console" ],
    "Filter": [
      {
        "Name": "ByIncludingOnly",
        "Args": { "inclusionPredicate": "Matching.WithProperty<string>(\"Word\", w => w.StartsWith(\"a\"))" }
      },
      {
        "Name": "ByExcluding",
        "Args": { "exclusionPredicate": "Matching.WithProperty<string>(\"Word\", w => w.Equals(\"ad\"))" }
      }
    ]
  }
}

If the Serilog config package were updated for destructuring, it would look like this:

{
  "Serilog": {
    "Using": [ "Serilog.Sinks.Console" ],
    "WriteTo": [ "Console" ],
    "Destructure": [
      {
        "Name": "ByTransformingWhere",
        "Args": {
          "predicate": "t => typeof(Account).Equals(t)",
          "transformedType": "Sample.Account",
          "transformation": "a => new { a.id, a.Username, a.AccountType }"
        }
      }
    ]
  }
}

@nblumhardt
Copy link
Member

Thanks @MV10!

I'm against bringing more functionality into this repository for the time being, just because we're already having a hard time getting optimal support for the features already here :-) .. having this via your repo as an add-on package would be great for those who want it in the interim. Thoughts?

@MV10
Copy link
Contributor

MV10 commented May 5, 2018

Honestly, to me it doesn't seem to have a lot of value as a stand-alone since that basically relegates it to a Filter extension. The destructure support is pointless without corresponding support in this Config package.

Frankly, I'm just one guy, not an organization of any sort or anyone of any particular renown, so I'm not too interested in supporting a package under my own name. I'd be happy to sign on as the S.S.Config guy if that's what it takes, it's a little different when I'm not flying solo, and I'm pretty heavily invested in the future of the Config package. (Plus I enjoy it, for whatever that's worth.)

Or we can chalk it up to a good time and close as won't implement. I'm pretty sure there is no other (reasonable) route to predicate/expression support from config.

@MV10
Copy link
Contributor

MV10 commented May 5, 2018

I suppose I could add support for the other less-complex destructure features (i.e. add ApplyDestructure to this Config package), then this extension would plug in, just as it does with filters. Still not real keen on sole ownership of a separate dependency but it would work.

But I guess that's more functionality in the package, too...?

@MV10
Copy link
Contributor

MV10 commented May 6, 2018

Starting work on a PR for the other less complicated destructure syntax (i.e. that won't require any new features) so that delegates can be added as a separate extensions package.

So, @zippy1981 I guess the answer to your end-game question is that I'll be managing a nuget package with this feature. Give me a bit more time, ultimately I want parity with "real" Serilog package quality -- docs, tests, Travis, and AppVeyor support. Or if anyone else feels like working on a PR in my repo for that (while I add destructure to Config), please feel free.

@MV10
Copy link
Contributor

MV10 commented Aug 23, 2018

This is finally live and available on NuGet.

https://github.com/MV10/serilog-settings-delegates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants