-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added TRACE level logging. #844
Added TRACE level logging. #844
Conversation
Pulled in a few improvements from #810. |
4a32d60
to
ef9d84e
Compare
@stevvooe, I saw your comment: containerd/containerd#2028
I understand your concern, but it rather makes me think we're using interfaces wrong. I think they should generally require the minimum functionality to accomplish what's required. All the logLevel methods are really convenience functions. Let me think on this a bit, but I think we can get a lot less repetitive code while making user-defined loggers much simpler and more adaptable. The way |
@stevvooe I have some good ideas regarding interfaces for useful user-defined logging, but I think I will wait on implementation until this PR is merged in, so I don't have to make some of the changes twice. @sirupsen, let me know if there are any additional blockers on reviewing and merging this PR, as it appears to have been kicking around in various forms for 2+ years now. @drampull, @xandrewrampulla, @Random-Liu, @BenTheElder, @gonix, @Xiol, @chackett, @pasztorpisti, @dnephin, @mkorolyov, @dgsb, @dmathieu: All of your contributed code and/or comments on this issue. Do you have anything to add, or would you like to see any additions to this PR? |
36ad384
to
b54cafe
Compare
Looks good to me. |
Looks good to me too. Can't tell you how excited I am for this - can finally get rid of my trace level wrappers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you complete TestLogLevelEnabled with this new level ?
@loren-osborn thank you for your contritbution, this looks mostly good to me, I've just requested a slight change in a test function. |
@dgsb, Thanks for the quick feedback. I know everybody is eager to have this in a new release, but I have one more change in mind (that I mentioned above in my comment to @stevvooe ) after this is merged that would further extend |
@loren-osborn, there is indeed no need to tag immediately a new version. I usually tag a new version at the beginning of the month. |
I should probably amend the last commit message to
before you merge this branch, but I won’t be at a computer for another 3 hours. I can do it then, or you’re welcome to amend the commit before then if you have time. Thanks. Update: Done. |
a09b778
to
ed3ffa0
Compare
awesome. thank you so much. i've been wanting a trace level and now i have it :D 🎈 |
@dgsb, Given all the legwork you're needing to do here, It's likely a good idea to add a way for a hook implementor to query the range of the log levels, so if it changes again, the hook developers don't need to make additional chanes? |
…velLogging Added TRACE level logging.
@loren-osborn we don't need an api for that, we already have the variable AllLevels which gathers all existing levels. |
Something like |
…n logrus v1.2.0 (sirupsen/logrus#844). (#3)
…velLogging Added TRACE level logging.
Address addition of trace level logging sirupsen/logrus#844
This is just a rebased update to PR #652
Please see discussion and history there.