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

change grpclog to use structured logging #322

Closed
wants to merge 1 commit into from

Conversation

aybabtme
Copy link

I took a stab at changing all the log sites to use structured logging, with a pluggable interface. I didn't want to pull existing implementations (like logrus or go-kit/log) to avoid adding dependencies. There are many places, like in tests, where grpclog was used and I think a better use would be to pass around a testing.TB handle, but I didn't do that change to avoid mixing concerns in 1 PR.

Relates to #289.

I signed the CLA.

@dsymonds
Copy link
Contributor

dsymonds commented Sep 2, 2015

I'm not keen on this at all. It makes the code significantly harder to read. I understand why one would want this, but I don't think it's worth the cost.

@iamqizhao
Copy link
Contributor

I have the same feeling about the readability and usability. In addition, the PR adds an additional mutex to logging, which is unpleasant.

@aybabtme
Copy link
Author

aybabtme commented Sep 2, 2015

Alrighty then! No worries.

@iamqizhao the stdlib logger has a mutex the same way: https://golang.org/src/log/log.go?s=2250:2542#L38

@aybabtme aybabtme closed this Sep 2, 2015
@aybabtme
Copy link
Author

aybabtme commented Sep 2, 2015

Having structured logging somehow would be really nice. Is there another way you folks would see the concept being used by this project?

@iamqizhao
Copy link
Contributor

Yes, I was wrong (that mutex is NOT "Addtional" one). :-) I somehow thought you use stdlib logger in your implementation, which is not true.

Actually your idea is good but the API is not sound. Probably we can have some better idea such as doing a bit reformating so that it would be easy to convert it to the desirable format in post-processing.

@aybabtme
Copy link
Author

aybabtme commented Sep 2, 2015

@iamqizhao I'm not totally following what you mean with:

so that it would be easy to convert it to the desirable format in post-processing

Mind to elaborate? Are there APIs for structured logging that you'd prefer?

I could perhaps, in another PR, change the many uses of grpclog in tests to use t.Errorf and t.Fatalf, which would reduce the noise we see here. Unless there was a specific reason to use grpclog.Fatal in tests?

@aybabtme aybabtme deleted the structured-log branch February 4, 2016 23:11
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants