getsentry
Attached new logger instance to client
New logger instance attached to client creation. Tests are tweaked to accommodate new changes but no new tests are written for error checks. I am not sure whether we want to do it or not.
FYI: I have still kept Global logger to be used directly, however, the mutation would still exist with this.
@YashishDua thank you for the PR!
FYI: I have still kept Global logger to be used directly, however, the mutation would still exist with this.
:+1: indeed it would be a smoother transition if we can keep that global logger around for a while, deprecate it and remove it as part of cleaning up the exported API.
@YashishDua the changes in the current state break the example code: https://travis-ci.com/getsentry/sentry-go/jobs/288114402
There are API changes, we need to be careful and diligent with those.
@YashishDua sorry I didn't have capacity to review this in detail yet. Getting rid of the global logger is more involved than I anticipated. I may need a few more days to get back to this.
Hi @YashishDua! I've thought about this a few times over the last weeks and now had another look.
I ended up reading (again) https://dave.cheney.net/2017/01/23/the-package-level-logger-anti-pattern and logging-related discussions in other projects like Kubernetes and posts in golang-nuts.
Your work and this PR makes it clear that several parts of the public API would require change. I'm now reluctant to think *log.Logger is the right dependency to impose on users of the SDK.
People should be free to use whatever logging framework they want (and possibly already use in their main package), be it log, go.uber.org/zap, github.com/golang/glog, k8s.io/klog, github.com/sirupsen/logrus, ..., the list is really long ⇒ awesome-go#logging.
I do not want to disappoint you, but I would like to consider the API change more carefully. I hope we can agree that there is a cost for users when we make breaking changes, therefore, let's take a step back and trace a path forward.
Right now consumers of sentry-go can enable/disable (debug) logging as they please, and they can configure where the output is sent. It is not ideal, and has problems as I described in #148. As far as I know nobody has stepped up to complain about that yet, except for myself who filed the issue.
Sorry for the long message, just wanted to put down my thoughts and hear what your ideas are. Can we discuss the requirements and design first before we invest more time on the solution? I apologize for not having discussed it in more detail before in #148. #148 was a mental note, something I had planned to work on, but had not investigated the consequences in detail. Your work made things clear, thank you! I appreciate your help.
@YashishDua I have an idea to move this forward.
Looking back at #148, I realized we can split the problem into two parts. The first one is #148 itself, which concerns only Client and the problem of calling NewClient.
For all the rest I discussed in my previous comment, the deeper changes required to deprecate/remove the global Logger, I've opened #182.
I believe we can fix #148 in this PR without changes to the public API.
This pull request has gone three weeks without activity. In another week, I will close it.
But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!
"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}