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

Candidate for log level filtering #349

Closed
wants to merge 1 commit into from
Closed

Candidate for log level filtering #349

wants to merge 1 commit into from

Conversation

nelz9999
Copy link
Contributor

I saw the previous work on #250 and #269, but I thought I saw an opportunity for a simpler implementation, so I took a stab at it.

I figured that since the end user needed to call into a level-specific function, that would an appropriate time to actually do the filtering, by then just sending back an instance of the NopLogger so that no action is taken past that point.

I got a little cute with having only the one noopLogger, but possibly written several times if you choose multiple of the xxxFilter() at config time, but I don't think that should present much overhead.

Let me know if there's further work you'd like to see here.

@nelz9999
Copy link
Contributor Author

(Eesh... Yah, I've got "noop" in my head from college courses... I guess the Go way is "nop", huh?)

@jstordeur
Copy link
Contributor

jstordeur commented Aug 30, 2016

Hello,

I'm also interested in having log leveling filtering.
Here are two comments I have after reading your proposal.

Levels vs Logger
If I interpret your proposal correctly the filtering would be invoked like this:
l := levels.New(logger, InfoFilter())

Then you would pass that Levels object in your function instead of a Logger and perform log calls like this:
l.Error().Log("foo", "bar") // not filtered
l.Info().Log("foo", "bar") // filtered

I don't know if forcing the Levels object instead of a Logger is a problem, I would think it could be because some functions in go-kit are expecting a Logger.
My understanding is that the current usage for levels is more the other way around, you carry around a Logger object and create the Levels object just before logging:
levels.New(logger).Error().Log("foo", "bar")

But maybe this is fine, I don't have enough experience to say for sure.

level threshold

When it comes to log filtering, I would expect to be able to have ordering across the different levels so that if I filter warning, then the info and debug logs are also filtered.

@nelz9999
Copy link
Contributor Author

nelz9999 commented Aug 31, 2016

@jstordeur, for your first point: I didn't realized that's how people might be using the Levels objects - by distributing the responsibility for creating the Levels object to the loci of the actual logging calls. Yes, my proposal would not work well for that pattern, because I assumed people would define the filter levels at startup/init time, and pass the Levels object around their code. (And unless I'm missing something, it seems that passing around the Levels object would be how you'd have to do it if you took advantage of the configuration renaming any of the levels, or the field name for the levels.)

And for your second point: Yes, I also am more familiar with having a ranked/ordered log hierarchy, but the other proposals seemed to get stuck at injecting that concept, and I figured I could just rely on a tiny bit more verbosity at setup/init time to minimize the changes to the API.

@ChrisHines
Copy link
Member

FWIW, we had a conversation about leveled logging in Slack recently that approached the problem from a different angle. I think it has some potential, but haven't had time to work through the ideas. I don't want to hi-jack this PR with the details, but here is a link to the start of the conversation: https://gophers.slack.com/archives/go-kit/p1471960936001282.

@nelz9999
Copy link
Contributor Author

@ChrisHines Thanks for that... Was the kick in the pants I needed to finally join the Slack! 😁

@peterbourgon
Copy link
Member

peterbourgon commented Sep 6, 2016

Please take a look at #357 and let me know what you think — and accept my apologies for usurping your work here! :)

@peterbourgon
Copy link
Member

Ping? :)

@nelz9999
Copy link
Contributor Author

Oh, sorry. I'll retract this in favor of #357.

@nelz9999 nelz9999 closed this Sep 29, 2016
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.

4 participants