{"action":"created","issue":{"url":"https://api.github.com/repos/aspnet/Logging/issues/400","repository_url":"https://api.github.com/repos/aspnet/Logging","labels_url":"https://api.github.com/repos/aspnet/Logging/issues/400/labels{/name}","comments_url":"https://api.github.com/repos/aspnet/Logging/issues/400/comments","events_url":"https://api.github.com/repos/aspnet/Logging/issues/400/events","html_url":"https://github.com/aspnet/Logging/pull/400","id":143764614,"number":400,"title":"Split loggers and log providers/observers (ILogger has too many responsibilities)","user":{"login":"cwe1ss","id":4581460,"avatar_url":"https://avatars.githubusercontent.com/u/4581460?v=3","gravatar_id":"","url":"https://api.github.com/users/cwe1ss","html_url":"https://github.com/cwe1ss","followers_url":"https://api.github.com/users/cwe1ss/followers","following_url":"https://api.github.com/users/cwe1ss/following{/other_user}","gists_url":"https://api.github.com/users/cwe1ss/gists{/gist_id}","starred_url":"https://api.github.com/users/cwe1ss/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cwe1ss/subscriptions","organizations_url":"https://api.github.com/users/cwe1ss/orgs","repos_url":"https://api.github.com/users/cwe1ss/repos","events_url":"https://api.github.com/users/cwe1ss/events{/privacy}","received_events_url":"https://api.github.com/users/cwe1ss/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/aspnet/Logging/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-03-27T00:45:39Z","updated_at":"2016-03-27T00:45:45Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/aspnet/Logging/pulls/400","html_url":"https://github.com/aspnet/Logging/pull/400","diff_url":"https://github.com/aspnet/Logging/pull/400.diff","patch_url":"https://github.com/aspnet/Logging/pull/400.patch"},"body":"I think it is problematic that the `ILogger` interface is used for creating log entries **and** for writing them to some output. For that reason, I suggest that providers (e.g. Console, EventLog, ...) should not inherit `ILogger`. Instead, we should move towards an Observable-like system.\r\n\r\nI will try my best to explain the benefits and possible future improvements, so please take a seat for the long story! :)\r\n\r\nThis is the current dependency structure:\r\n* `LoggerFactory` creates and manages `Logger` instances - one per categoryName. They are responsible for **creating** log entries.\r\n* `LoggerFactory` has one `ILoggerProvider` per provider\r\n* Every `Logger` instance stores a list of `ILogger` instances - one per provider. They are responsible for **writing** log entries to outputs.\r\n* Whenever a new provider is added, `LoggerFactory` adds the `ILogger` instance from the provider to every `Logger`-instance.\r\n* Each `Logger` communicates directly with each provider-`ILogger`.\r\n\r\nI think that this solution has a few disadvantages:\r\n* Providers are forced to have `ILogger` instances per logger category. Requiring different `Logger` instances is a concept of log entry creation and is useful there. Why should there be many instances of e.g. an EventLog-provider?\r\n* This also makes it harder for providers to batch log entries. If you want to send e.g. 50 log entries at once to some output system, you would either have to batch 50 per category or you would have to merge them somehow first.\r\n* the direct relationship between `Logger` and the provider-`ILogger`s makes it harder to apply global patterns like filtering because you always have to broadcast everything to every `Logger` in the LoggerFactory. \r\n* Providers are forced to implement the same contract. If we would e.g. have global filtering, `IsEnabled` would only be interesting for the creation-side. Providers would not need the same method because they would not even receive the entry.\r\n* Mixing the concepts makes this really hard to understand. Just look at the mix of `ILogger` and `Logger` here.\r\n\r\nIn my opinion, the logging system should work more like the observable pattern. `ILogger` instances should be responsible for creating log entries and they should send them to some central coordinator. This coordinator knows about subscribed providers and forwards the entries to each of them. \r\n\r\n#### what I would like to see\r\nIdeally, I would completely split log creation and forwarding. This would begin with how providers are set up. I think it is weird, that you can add providers to a `LoggerFactory` because the factory is used in many locations. Why should you be able to do something like this:\r\n```csharp\r\npublic SomeClass {\r\n  public SomeClass(ILoggerFactory loggerFactory) {\r\n    loggerFactory.AddConsole();\r\n  }\r\n}\r\n```\r\nI think it should only be possible to call `CreateLogger` here. I think this would come closest to what the name \"factory\" implies.\r\n\r\nIdeally, providers would be observers - e.g. `ILogObserver`. There would be a corresponding `LogObservable` which acts as the described central coordinator. Only this class would need the `AddObserver` (aka `Subscribe`, aka `AddProvider`) method. So when you would create the application, you would configure `LogObservable` instead of `LoggerFactory`. This would look like this:\r\n```csharp\r\nvar logObservable = new LogObservable()\r\n  .AddEventLog()\r\n  .AddConsole();\r\n\r\nvar loggerFactory = new LoggerFactory(logObservable);\r\n// both will be added to DI as singleton\r\n\r\n// There would be no AddProvider here\r\nvar logger = loggerFactory.CreateLogger<Program>();\r\n```\r\nIdeally, the `LogObservable` and `LogObserver` would actually implement `IObservable` and `IObserver` to allow us to use Rx components. However I'm not sure if this would play well with other required methods like `BeginScope`.\r\n\r\n#### baby steps\r\nSince this would be a quite big change, I'm proposing a compromise and a first step with this pull request:\r\n* the meaning of `ILoggerProvider` will be changed (instead of renamig it to LogObserver). This interface will represent everything that is required for providers to **write** entries to some output. It will no longer have `CreateLogger`, instead it will have methods like `Log` and `BeginScope`. This will be a breaking change, however it only affects people who built their own providers.\r\n* Providers will no longer have to inherit `ILogger` because everything they need is on `ILoggerProvider`.\r\n* Providers will get the `categoryName` as part of their methods. This means, there may only be one instance of every provider. If the provider needs to have different instances, it can still do so based on this parameter.\r\n* The `LoggerFactory` will still be responsible for adding providers to make sure there is no breaking change for client applications.\r\n* The `LoggerFactory` and `Logger` instances will no longer maintain a direct relationship with providers. Instead, they will just forward everything to the new `LogObservable` class. This results in very easy to understand code in `Logger` and `LoggerFactory`\r\n* The new `LogObservable` contains most of the code from `Logger` and `LoggerFactory`. It allows us to have one central location that talks to the providers.\r\n\r\nThis code contains a POC of how this would work. Please note the following:\r\n* the existing providers still contain *LoggerProvider and *Logger classes. The *Logger class would not be necessary but I kept them to keep changes to a minimum.\r\n* the providers still contain code for filtering. This could be moved to the LogObservable class.\r\n* some unit tests fail - I just made sure everything compiles for now.\r\n\r\nso don't merge it yet :-) :-)\r\n\r\nI think that this solution would also make global filtering #398 easier. I think it would remove the need to create a separate LoggerFactory. I will try to come up with a good solution.\r\n\r\nI'm looking forward to your feedback!"},"comment":{"url":"https://api.github.com/repos/aspnet/Logging/issues/comments/201964509","html_url":"https://github.com/aspnet/Logging/pull/400#issuecomment-201964509","issue_url":"https://api.github.com/repos/aspnet/Logging/issues/400","id":201964509,"user":{"login":"dnfclas","id":10201666,"avatar_url":"https://avatars.githubusercontent.com/u/10201666?v=3","gravatar_id":"","url":"https://api.github.com/users/dnfclas","html_url":"https://github.com/dnfclas","followers_url":"https://api.github.com/users/dnfclas/followers","following_url":"https://api.github.com/users/dnfclas/following{/other_user}","gists_url":"https://api.github.com/users/dnfclas/gists{/gist_id}","starred_url":"https://api.github.com/users/dnfclas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dnfclas/subscriptions","organizations_url":"https://api.github.com/users/dnfclas/orgs","repos_url":"https://api.github.com/users/dnfclas/repos","events_url":"https://api.github.com/users/dnfclas/events{/privacy}","received_events_url":"https://api.github.com/users/dnfclas/received_events","type":"User","site_admin":false},"created_at":"2016-03-27T00:45:45Z","updated_at":"2016-03-27T00:45:45Z","body":"Hi __@cwe1ss__, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!\r\n    <span>You've already signed the contribution license agreement. Thanks!</span>\r\n        <p>The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.</p>\r\n\r\nTTYL, DNFBOT;\r\n"}}