Add scope support - Githubissues

nreco / logging

Generic file logger for .NET Core (FileLoggerProvider) with minimal dependencies

MIT License

284 stars 56 forks source link

I have implemented scope support using a combination of code from the following: https://www.meziantou.net/asp-net-core-json-logger.htm https://www.codeproject.com/Articles/1556475/How-to-Write-a-Custom-Logging-Provider-in-ASP-NET

In addition, my new additions follow the 3 scope semantics outlined in this blog: https://nblumhardt.com/2016/11/ilogger-beginscope/

I have also included full xml comments and doc file. I also took the liberty of adding SourceLink configuration to allow downstream implementers to step through this project with a debugger if needed.

There are no breaking changes. All old tests still pass with no modifications. I have added new tests to demonstrate the use of scopes. By default, the standard logging output is unchanged. Users may write scope information to output by setting a custom callback for FileLoggerOptions.FormatLogEntry

I've reviewed your changes and here are my notes:

what is the purpose of "Microsoft.SourceLink.GitHub" package reference in 'NReco.Logging.File.csproj'? It is definitely excessive dependency.
namespace 'Nreco.Logging.File.Microsoft.Extensions.Logging' is strange and actually it doesn't follow naming convertions recommended by MS. As for me, all these new implementations should be in 'NReco.Logging.File' namespace.
interfaces ISupportExternalScope and IExternalScopeProvider seems like a bit overengineering. Since in fact scopes are still not supported by default, I would like to have a simple extension point in NReco.Logging.File (maybe with delegate(s) ).
now in 'LogMessage' class constructor List
and Dictionary<string,object> instances are created on each log message (even if scopes are not used at all!). This is unacceptable waste of resources for the logging provider which actually can handle thousands of messages per second. If it is not possible to add scopes without that, I prefer not to support scopes in "NReco.Logging.File" at all.
finally, your commits break the history - and now changeset shows that almost all code was removed and added again (maybe because of changes in formatting): https://github.com/nreco/logging/pull/20/files

--

I appreciate your wiliness to contribute a new capability, but as a package maintainer I cannot accept this PR. If you need to have scopes I think it would be better to use your own build of the logger.
TIPConsulting commented 3 years ago
- I already gave a link to the SourceLink documentation. Heres another: https://www.hanselman.com/blog/exploring-net-cores-sourcelink-stepping-into-the-source-code-of-nuget-packages-you-dont-own
- Nreco.Logging.File.Microsoft.Extensions.Logging is strange, Ill give you that. Those items come straight from the MS logging project on github. They do not already exist in this project because it's logging dependency is 4 major versions behind
- Adding scope to the log message object appears to be standard practice. I think your performance concerns are wildly misplaced - writing to disk is several orders of magnitude more expensive than those empty collections with a level 0 GC.
- Not much I can do about that
Finally: Noted. Next time I won't bother
VitaliyMF commented 3 years ago

Thank you for all your efforts -- after all, you've implemented scopes for yourself b/c you need them.

Open source is a good: you always can use your own fork with modifications that are needed for your project.
- © Githubissues.
- Githubissues is a development platform for aggregating issues.

nreco / logging

Add scope support #20