{"action":"created","issue":{"url":"https://api.github.com/repos/uber-go/fx/issues/290","repository_url":"https://api.github.com/repos/uber-go/fx","labels_url":"https://api.github.com/repos/uber-go/fx/issues/290/labels{/name}","comments_url":"https://api.github.com/repos/uber-go/fx/issues/290/comments","events_url":"https://api.github.com/repos/uber-go/fx/issues/290/events","html_url":"https://github.com/uber-go/fx/pull/290","id":208626098,"number":290,"title":"Upgrade to zap 1.0 release candidate","user":{"login":"akshayjshah","id":972790,"avatar_url":"https://avatars.githubusercontent.com/u/972790?v=3","gravatar_id":"","url":"https://api.github.com/users/akshayjshah","html_url":"https://github.com/akshayjshah","followers_url":"https://api.github.com/users/akshayjshah/followers","following_url":"https://api.github.com/users/akshayjshah/following{/other_user}","gists_url":"https://api.github.com/users/akshayjshah/gists{/gist_id}","starred_url":"https://api.github.com/users/akshayjshah/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/akshayjshah/subscriptions","organizations_url":"https://api.github.com/users/akshayjshah/orgs","repos_url":"https://api.github.com/users/akshayjshah/repos","events_url":"https://api.github.com/users/akshayjshah/events{/privacy}","received_events_url":"https://api.github.com/users/akshayjshah/received_events","type":"User","site_admin":false},"labels":[{"id":521352238,"url":"https://api.github.com/repos/uber-go/fx/labels/refactor","name":"refactor","color":"fbca04","default":false}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":15,"created_at":"2017-02-18T08:55:43Z","updated_at":"2017-02-24T01:10:35Z","closed_at":"2017-02-24T00:55:50Z","pull_request":{"url":"https://api.github.com/repos/uber-go/fx/pulls/290","html_url":"https://github.com/uber-go/fx/pull/290","diff_url":"https://github.com/uber-go/fx/pull/290.diff","patch_url":"https://github.com/uber-go/fx/pull/290.patch"},"body":"This PR upgrades UberFX to use the release candidate of zap 1.0. Along the way, it migrates FX to use the base zap logger (what you were calling the typed logger). It includes a few potentially controversial changes, which I'll address head-on here. I'm happy to chat about any of this in person on Monday.\r\n\r\n(Clearly, this can't get merged until after your code freeze. That's cool, I'd just like to get review done before I'm constantly rebasing.)\r\n\r\n## No ulog.Log interface\r\n\r\nInterfaces are good for a few things:\r\n\r\n- depending on a subset of a type's methods\r\n- allowing callers to pass third-party types without depending on those external packages\r\n- abstracting away implementation, which lets you swap implementations later\r\n\r\nHowever, the former `ulog.Log` interface doesn't accomplish any of these goals. It would have included the full `zap.SugaredLogger` API, and it doesn't prevent UberFX (and ulog) from taking a concrete dependency on zap. Most importantly, because we need users to be able to recover the strongly-typed `*zap.Logger`, it references tons of concrete zap types (`*zap.Logger`, which then references `Field` and `*CheckedEntry`). It's hard for me to imagine a reimplementation of that API outside zap.\r\n\r\nThat might still be okay, but interfaces are also harder to extend - adding even a single method is a breaking change. Given all that, it seems wiser to me to use `*zap.Logger` directly and eliminate a bunch of copypasta. Since we own both libraries, I don't see this as a significant problem. - users can and should declare narrow interfaces for just the portions of the logger that they're using.\r\n\r\n## No Sentry integration\r\n\r\nThis is purely a time issue. If you can't wait for the Filebeat plugin we've discussed, I'm happy to resurrect direct Sentry integration in a follow-on PR. The latest zap should allow us to do that without so many map copies and with zero impact on non-error log performance.\r\n\r\n## No loggers in contexts\r\n\r\nI was a little confused by the existing code. It looks to me like we're largely working with a single global logger, which starts off with some default configuration and is reconfigured on service startup. We're supplying that logger to users by putting it into the request context and providing `ulog.Logger(ctx context.Context) ulog.Logger` as a getter. Where possible, we add tracing information to the logger before putting it into the context.\r\n\r\nIn all that, I'm not sure whether the logger *needs* to be in the context at all. We can keep `ulog.Logger`, and add just add the context's tracing information to the logger when it's retrieved. This is quite inexpensive, and it's much simpler.\r\n\r\n## No log builder\r\n\r\nI don't think that this is required any more either; we can just use zap's config shapes instead. They already come with sensible default values, and we can modify the service scaffold to comply with our internal logging schemas.\r\n\r\n## Documentation\r\n\r\nI didn't touch the ulog README much - I'll leave that for someone on your team."},"comment":{"url":"https://api.github.com/repos/uber-go/fx/issues/comments/282173444","html_url":"https://github.com/uber-go/fx/pull/290#issuecomment-282173444","issue_url":"https://api.github.com/repos/uber-go/fx/issues/290","id":282173444,"user":{"login":"glibsm","id":199077,"avatar_url":"https://avatars.githubusercontent.com/u/199077?v=3","gravatar_id":"","url":"https://api.github.com/users/glibsm","html_url":"https://github.com/glibsm","followers_url":"https://api.github.com/users/glibsm/followers","following_url":"https://api.github.com/users/glibsm/following{/other_user}","gists_url":"https://api.github.com/users/glibsm/gists{/gist_id}","starred_url":"https://api.github.com/users/glibsm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/glibsm/subscriptions","organizations_url":"https://api.github.com/users/glibsm/orgs","repos_url":"https://api.github.com/users/glibsm/repos","events_url":"https://api.github.com/users/glibsm/events{/privacy}","received_events_url":"https://api.github.com/users/glibsm/received_events","type":"User","site_admin":false},"created_at":"2017-02-24T01:10:35Z","updated_at":"2017-02-24T01:10:35Z","body":"leerooooooooooyyyyyyyyy"}}
{"actor":{"display_login":"glibsm"}}