Skip to content
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

Merged
merged 5 commits into from
Oct 20, 2018

Conversation

loren-osborn
Copy link

@loren-osborn loren-osborn commented Oct 18, 2018

This is just a rebased update to PR #652

Please see discussion and history there.

@loren-osborn
Copy link
Author

Added tests from #643 as @stevvooe suggested in #652.

@loren-osborn
Copy link
Author

Pulled in a few improvements from #810.

@loren-osborn
Copy link
Author

@stevvooe, I saw your comment: containerd/containerd#2028

@Random-Liu I am a maintainer there...

I think the problem with #652 is that it is not backwards compatible for those using the interface to define a logger type. I'll LGTM it and see if that is a concern.

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 StdLogger and FieldLogger are defined, they only make sense as a way to abstract logrus internal types. We could make implementing user-defined loggers dead simple, by adding a few interfaces and a bit of plumbing.

@loren-osborn
Copy link
Author

loren-osborn commented Oct 18, 2018

@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?

@gonix
Copy link
Contributor

gonix commented Oct 18, 2018

Looks good to me.

@Xiol
Copy link

Xiol commented Oct 18, 2018

Looks good to me too. Can't tell you how excited I am for this - can finally get rid of my trace level wrappers.

Copy link
Collaborator

@dgsb dgsb left a 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 ?

@dgsb
Copy link
Collaborator

dgsb commented Oct 19, 2018

@loren-osborn thank you for your contritbution, this looks mostly good to me, I've just requested a slight change in a test function.
We may also want to warn all hooks implementers of this new level before tagging this new version.
Even if it is a non-breaking api change it may have impact on how the hooks are implemented.

@loren-osborn
Copy link
Author

loren-osborn commented Oct 19, 2018

@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 Ext1FieldLogger, and I'd prefer not to add an additional Ext2FieldLogger for these new methods, so perhaps, after merging this into master, you could hold off on a release a few days until my next PR is ready?

@dgsb
Copy link
Collaborator

dgsb commented Oct 19, 2018

@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.

@loren-osborn
Copy link
Author

loren-osborn commented Oct 19, 2018

@dgsb, I expect to make that a separate PR, so feel free to merge this whenever convenient.

(You’ll probably also want to close #629, #643, #652, #663, and #810. It looks like you’ve closed several of these already.)

@loren-osborn
Copy link
Author

loren-osborn commented Oct 19, 2018

I should probably amend the last commit message to

PR#844: Added Trace to TestLogLevelEnabled() (requested by @dgsb)

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.

@Dieterbe
Copy link

awesome. thank you so much. i've been wanting a trace level and now i have it :D 🎈

@loren-osborn
Copy link
Author

@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?

@loren-osborn loren-osborn restored the pull/652_AddTraceLevelLogging branch October 22, 2018 16:36
loren-osborn pushed a commit to loren-osborn/logrus that referenced this pull request Oct 22, 2018
@dgsb
Copy link
Collaborator

dgsb commented Oct 23, 2018

@loren-osborn we don't need an api for that, we already have the variable AllLevels which gathers all existing levels.
Anyhow, hooks implementors needs to be warned when we add a new level, especially ones with a high volume.

@loren-osborn
Copy link
Author

Something like AllLevels is exactly what I had in mind. I’m sorry I hadn’t noticed it there before.

gguridi pushed a commit to hybridtheory/logrus-nxlog-hook that referenced this pull request Jan 22, 2019
gguridi added a commit to hybridtheory/logrus-nxlog-hook that referenced this pull request Jan 22, 2019
@tmshn tmshn mentioned this pull request Sep 1, 2019
cgxxv pushed a commit to cgxxv/logrus that referenced this pull request Mar 25, 2022
johntdyer added a commit to johntdyer/slackrus that referenced this pull request Sep 12, 2022
Address addition of trace level logging sirupsen/logrus#844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants