{"action":"created","issue":{"url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/issues/1948","repository_url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet","labels_url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/issues/1948/labels{/name}","comments_url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/issues/1948/comments","events_url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/issues/1948/events","html_url":"https://github.com/GoogleCloudPlatform/google-cloud-dotnet/pull/1948","id":301882669,"number":1948,"title":"Add convenience extension method on WebHost builder","user":{"login":"henkmollema","id":1521207,"avatar_url":"https://avatars2.githubusercontent.com/u/1521207?v=4","gravatar_id":"","url":"https://api.github.com/users/henkmollema","html_url":"https://github.com/henkmollema","followers_url":"https://api.github.com/users/henkmollema/followers","following_url":"https://api.github.com/users/henkmollema/following{/other_user}","gists_url":"https://api.github.com/users/henkmollema/gists{/gist_id}","starred_url":"https://api.github.com/users/henkmollema/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/henkmollema/subscriptions","organizations_url":"https://api.github.com/users/henkmollema/orgs","repos_url":"https://api.github.com/users/henkmollema/repos","events_url":"https://api.github.com/users/henkmollema/events{/privacy}","received_events_url":"https://api.github.com/users/henkmollema/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":18,"created_at":"2018-03-02T19:43:28Z","updated_at":"2018-03-26T18:17:09Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/pulls/1948","html_url":"https://github.com/GoogleCloudPlatform/google-cloud-dotnet/pull/1948","diff_url":"https://github.com/GoogleCloudPlatform/google-cloud-dotnet/pull/1948.diff","patch_url":"https://github.com/GoogleCloudPlatform/google-cloud-dotnet/pull/1948.patch"},"body":"An initial take on refactoring to use `IServiceProvider` rather than using the static `HttpContextAccessorWrapper`. \r\n\r\nThis introduces breaking changes the the public APIs of `ILoggerFactory.AddGoogle()` and `GoogleLoggerProvider.Create()`, effectively making it impossible to configure logging without an `IServiceProvider` instance at hand. \r\n\r\nMost users will either:\r\n- Call `AddGoogle` on an injected `ILoggerFactory` instance in `Startup` class. E.g.:\r\n```cs\r\npublic void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)\r\n{\r\n    loggerFactory.AddGoogle();\r\n}\r\n```\r\nThis usage can easily be fixed by passing `app.ApplicationServices` to `AddGoogle`.\r\n\r\n- Use `GoogleLoggerProvider.Create` in `ConfigureLogging` when configuring the web host builder. (ASP.NET Core 2.x) E.g.:\r\n```cs\r\nWebHost.CreateDefaultBuilder(args)\r\n    .ConfigureLogging((hostContext, builder) =>\r\n    {\r\n        builder.AddProvider(GoogleLoggerProvider.Create());\r\n    })\r\n\r\n```\r\n~This will impose problems as you're unable to pass the `IServiceProvider` instance at this point, as `ConfigureLogging` actually participates in composing the `IServiceCollection`.~ This could be changed to:\r\n```cs\r\nbuilder.Services.AddSingleton<ILoggerProvider>(sp =>\r\n    GoogleLoggerProvider.Create(sp, projectId));\r\n```\r\n\r\nBesides that, I propose to create a new extension method on the `IWebHostBuilder` interface which configures logging. As I suggested in https://github.com/GoogleCloudPlatform/google-cloud-dotnet/issues/1937#issuecomment-369857071 it could configure: \r\n- Google logger provider (using a startup filter with `ILoggerFactory`)\r\n- Google tracing and exception logging services\r\n - Google tracing and exception logging middleware\r\n\r\nAs a last note: the `IServiceProvider` instance could be optional (I've already implemented its usage as if it was optional) which keeps the breaking changes to a minimal. However, this probably would cause confusion as some features won't work due to the absence of an `IServiceProvider`.\r\n\r\nI'm curious about your thoughts about this and how to go forward with it.\r\n\r\n/cc @iantalarico @jskeet "},"comment":{"url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/issues/comments/376262034","html_url":"https://github.com/GoogleCloudPlatform/google-cloud-dotnet/pull/1948#issuecomment-376262034","issue_url":"https://api.github.com/repos/GoogleCloudPlatform/google-cloud-dotnet/issues/1948","id":376262034,"user":{"login":"henkmollema","id":1521207,"avatar_url":"https://avatars2.githubusercontent.com/u/1521207?v=4","gravatar_id":"","url":"https://api.github.com/users/henkmollema","html_url":"https://github.com/henkmollema","followers_url":"https://api.github.com/users/henkmollema/followers","following_url":"https://api.github.com/users/henkmollema/following{/other_user}","gists_url":"https://api.github.com/users/henkmollema/gists{/gist_id}","starred_url":"https://api.github.com/users/henkmollema/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/henkmollema/subscriptions","organizations_url":"https://api.github.com/users/henkmollema/orgs","repos_url":"https://api.github.com/users/henkmollema/repos","events_url":"https://api.github.com/users/henkmollema/events{/privacy}","received_events_url":"https://api.github.com/users/henkmollema/received_events","type":"User","site_admin":false},"created_at":"2018-03-26T18:17:08Z","updated_at":"2018-03-26T18:17:08Z","author_association":"NONE","body":"@iantalarico I've left two comments with some questions. Would you like to take a look?\r\n\r\nBeside that, I think this PR is mostly done. It has become a combination of two things now:\r\n- Refactor to use `IServiceProvider` rather than a static HttpContext accessor\r\n- Convenience extension method on WebHost builder\r\n\r\nIs it okay to keep them together in this PR, or do you want me to split them up? They're separate commits so it's no big deal to split."}}
{"actor":{"display_login":"henkmollema"}}