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

LoggingFilterSwitch support #162

Merged
merged 6 commits into from
Nov 23, 2020
Merged

LoggingFilterSwitch support #162

merged 6 commits into from
Nov 23, 2020

Conversation

skomis-mm
Copy link
Contributor

Added support for LoggingFilterSwitch from Serilog.Filters.Expressions to be used like LoggingLevelSwitch:

  • new FilterSwitches config section
  • dynamic filtering control

@skomis-mm
Copy link
Contributor Author

skomis-mm commented Jan 9, 2019

Probably makes sense to introduce unified Switches section with the following schema:

"Switches": { "name": "@switch", "type": "level|filter", "value": "string" }

and deprecate current LevelSwitches section

@tsimbalar
Copy link
Member

I think it's better to keep 2 separate sections for LevelSwitches and FilterSwitches for now, at least so we kind of preserve some sort of symmetry with https://github.com/serilog/serilog-settings-appsettings , where all we have is key/value pairs and it would be hard to introduce an extra type attribute.

Deprecating LevelSwitches would mean we still have to support it for quite a while and make it obvous to the user that the syntax has changed ... which is kind of hard to do in a JSON file :P

Just my 2 cents :)

Copy link
Member

@tsimbalar tsimbalar left a comment

Choose a reason for hiding this comment

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

LGTM ! 🚀

@skomis-mm
Copy link
Contributor Author

skomis-mm commented Jan 10, 2019

It is probably a matter of taste but new section looks like "homemade" for me :) Deprecating something in json indeed a problem in general (JSON schema does not have spec for this, yet), but probably won't be a big problem for Serilog (considering supporting both). At the same time new section could be a good starting point and can be deprecated in the future as well (if ever).
Kindly pinging @nblumhardt @MV10 if you have something to say.

@tsimbalar
Copy link
Member

fair enough :)
We'll have to remember to update the docs in that case, in https://github.com/serilog/serilog-sinks-seq#dynamic-log-level-control in particular.

@nblumhardt
Copy link
Member

I see how, now that there are two *Switches sections, they seem a bit inconsistent with the way the rest of the JSON is laid out.

Agreed it's a matter of taste; just putting one more option on the table, we could try stepping up one level of abstraction and introduce an (admittedly incomplete) declarations section (straw-man syntax):

"Declare": [
  {"Name": "$serverUrl", "Value": "https://some.example.com"},
  {"Name": "$filterSwitch",
   "Value": "Application = 'Sample'",
   "Type": "Serilog.Filters.Expressions.LoggingFilterSwitch"},
  {"Name": "$levelSwitch",
   "Value": "Information",
   "Type": "Serilog.Core.LoggingLevelSwitch"},
]

Lots of hand-waving and unanswered questions in there (do we really need the $ prefix when we declare names? What type conversions do we special-case? How are variables used when there might be ambiguities in existing values? etc.) but from a 1000-foot view, it seems like considering this approach might open up some new scenarios with a minimum of future churn. Thoughts?

@tsimbalar tsimbalar mentioned this pull request Mar 25, 2019
@nblumhardt
Copy link
Member

Hi @skomis-mm @tsimbalar - just a quick update, I've spent more time now with Serilog.Filters.Expressions and think we can do better in a number of ways:

  • The codebase suffers a lot from being extracted from/mashed up from other projects (it's unnecessarily complex and hard to maintain)
  • There are scenarios other than filtering in which expressions are useful (namespaces/names need changes)
  • The internal type system of Serilog.Filters.Expressions isn't a good fit for integration with other parts of the Serilog ecosystem (it's based on eagerly-converted POCO values, from a limited set of supported types, rather than working with LogEventPropertyValue and arbitrary user-provided types)

I've taken another shot at it in https://github.com/nblumhardt/serilog-expressions - it's still WIP and needs documentation, tests, and some explanations, but I hope it's a better foundation to build on.

The new lib includes LoggingFilterSwitch, so if we feel it's a better overall design, perhaps we should consider pointing this PR at it when done?

Also in there:

            .WriteTo.Console(new OutputTemplate(
                "[{@t:HH:mm:ss} {@l:u3} ({SourceContext})] {@m} (first item is {Items[0]})\n{@x}"))

Note {Items[0]} - the goal is to allow arbitrary expressions in "holes", which turns out to be rather tricky :-) --- the short @t etc. names are taken from Serilog.Formatting.Compact, I know there's a bit of tension between this usage and the @ operator in message templates but couldn't come up with a better option that wasn't just yet-another-new-syntax.

Final note ... @x is an Exception in this case rather than a string, so we could add supporting functions like message(@x) etc. to operate on it. Lots to think about.

May be a few more weeks chipping away before it's really presentable, but if you have ideas/suggestions please do jump in :-)

@tsimbalar
Copy link
Member

@nblumhardt Thanks for the heads up 👍
That looks interesting 👓

@skomis-mm
Copy link
Contributor Author

skomis-mm commented Sep 15, 2020

@nblumhardt @tsimbalar Great, looks promising!
Sorry for the no answer on the last comment, I just forgot this one since I had no strong opinion on how the syntax should look like. Will take a look on the new lib 👀

@skomis-mm
Copy link
Contributor Author

skomis-mm commented Nov 17, 2020

Added Serilog.Expressions support (from netcoreapp3.1 onwards) to Serilog.Filters.Expressions support for older frameworks.

@nblumhardt
Copy link
Member

I think once this is in we should merge #238 👍

@skomis-mm
Copy link
Contributor Author

@nblumhardt I was thinking that too. The only thing confuses me #231 . Should we fix that too?

@nblumhardt
Copy link
Member

I haven't really managed to wrap my head around #231 yet @skomis-mm - guess it might have to wait this time around :-)

@nblumhardt nblumhardt merged commit 07d7a8d into serilog:dev Nov 23, 2020
@nblumhardt nblumhardt mentioned this pull request Nov 23, 2020
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.

3 participants