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

Support Serilog expressions in sink configuration #276

Closed
ZBlocker655 opened this issue Aug 19, 2021 · 5 comments · Fixed by #281
Closed

Support Serilog expressions in sink configuration #276

ZBlocker655 opened this issue Aug 19, 2021 · 5 comments · Fixed by #281

Comments

@ZBlocker655
Copy link

Inspired by this Stack Overflow question

Rather than create a formatter class to pull in an expression for sink configuration in appsettings, why not provide direct support for expression configuration for a sink?

Example:

{
  "Name": "Console",
  "Args": { "expression": "[{@t:HH:mm:ss} {@l:u3} ({SourceContext})] {@m}" }
}
@nblumhardt
Copy link
Member

Thanks, Zach!

The properties in Args are an exact mapping to the arguments of the Console() method; expression isn't one of these (although the ByExcluding() and ByIncluding() methods do have that argument name).

The corresponding one that we'd need to set for Console() etc. is formatter.

If we pass a string through for formatter today, it's regarded as the type name of an ITextFormatter implementation (or a static member name on one of those).

I think there would be a few ways to make this work without breakage -

  1. Provide a way for argument types to accept arguments, e.g."formatter": "Serilog.Expressions.TemplateExpressionTemplate, Serilog.Expressions(\'[{@t:HH:mm:ss} {@l:u3} ({SourceContext})] {@m}\')"

  2. Add an additional parameter alongside Name and Args:

{
  "Name": "Console",
  "Args": {
    "formatter": {
        "type": "Serilog.Expressions.TemplateExpressionTemplate, Serilog.Expressions",
        "template": "[{@t:HH:mm:ss} {@l:u3} ({SourceContext})] {@m}"
    }
  }
}
  1. Fall back to constructing an ExpressionTemplate automatically, whenever a formatter cannot be mapped to a valid concrete type or property:
{
  "Name": "Console",
  "Args": { "formatter": "[{@t:HH:mm:ss} {@l:u3} ({SourceContext})] {@m}" }
}
  1. Special-case "expression" and substitute for an ITextFormatter "formatter" argument where one appears:
{
  "Name": "Console",
  "Args": { "expression": "[{@t:HH:mm:ss} {@l:u3} ({SourceContext})] {@m}" }
}

Pros/cons/variants of each - just getting the options out there :-)

@skomis-mm
Copy link
Contributor

Hi @ZBlocker655 , @nblumhardt .

I like the second option - add generic support to public constructor's arguments. I would add $ prefix to special type argument to distinguish from constructor's argument names.
I think we have other sinks with complex types in configuration extension methods - that would be usefull too.

"Args": {
      "formatter": {
            // $type is mandatory because of abstract nature of argument type 
            "$type": "Serilog.Expressions.TemplateExpressionTemplate, Serilog.Expressions", 
            "template": "...",
      },
       // $type is optional. By default binds to argument type if it is not abstract
      "concreteComplexTypeArg": {          
           "arg1":  "..",
            ..
      }
  }    

Will also solve #225

@ZBlocker655
Copy link
Author

I think I agree with @skomis-mm that option 2 is the optimal one. I like option 4 for its simplicity and expressiveness, but I'm also leery of making "special cases" that might go against convention (in this case, the convention of exposing underlying class properties.

Option 3 also feels like a special case, though if it's not too much of an ugly stuck-on bit I think it would be nice as well.

@nblumhardt
Copy link
Member

Great - also a +1 from me, for option 2.

$type may be a bit inconvenient because of the frequent requirement to set configuration options through environment variables via shell scripts, where $ often indicates a shell variable reference. Agree that it's a nicer name to use if possible, though.

@skomis-mm
Copy link
Contributor

skomis-mm commented Aug 24, 2021

Right, just forgot about environment variables and corresponding issue.

We have a bit stucked PR on this. Will look into it if it can be restored.

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 a pull request may close this issue.

3 participants