Skip to content

Replace grpclog with dlog #341

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Replace grpclog with dlog #341

wants to merge 2 commits into from

Conversation

@bufdev
Copy link
Contributor

@bufdev bufdev commented Sep 15, 2015

This is re: grpc-ecosystem/grpc-gateway#54

So the idea is, both grpc-go and grpc-gateway use dlog to abstract logging libraries away. There is no effective change to grpc-go other than grpclog is deleted. To use glog, instead of doing:

import _ "google.golang.org/grpc/grpclog/glogger"

One would do:

import _ "go.pedge.io/dlog/glog"

For reference, go.pedge.io/dlog has TLS/SSL:

$ curl -v https://go.pedge.io/dlog
* Hostname was NOT found in DNS cache
*   Trying 54.149.141.139...
* Connected to go.pedge.io (54.149.141.139) port 443 (#0)
* TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate: go.pedge.io
* Server certificate: StartCom Class 1 Primary Intermediate Server CA
* Server certificate: StartCom Certification Authority
> GET /dlog HTTP/1.1
> User-Agent: curl/7.37.1
> Host: go.pedge.io
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: text/html; charset=utf-8
< Date: Tue, 15 Sep 2015 20:50:54 GMT
* Server nginx/1.6.2 is not blacklisted
< Server: nginx/1.6.2
< Content-Length: 473
< Connection: keep-alive
<
<!DOCTYPE html>
<html>
  <head>
    <meta name="go-import" content="go.pedge.io/dlog git https://github.com/peter-edge/go-dlog">
    <meta name="go-source" content="go.pedge.io/dlog https://github.com/peter-edge/go-dlog https://github.com/peter-edge/go-dlog/tree/master{/dir} https://github.com/peter-edge/go-dlog/tree/master{/dir}/{file}#L{line}">
    <meta http-equiv="refresh" content="0; url=https://github.com/peter-edge/go-dlog">
  </head>
  <body>
  </body>
</html>
* Connection #0 to host go.pedge.io left intact

@iamqizhao
Copy link
Contributor

Thanks for contributing. I got some concerns on this change:
i) Installation of gRPC-Go must NOT have inherent dependency on a non-standard or non-google-backed package (bradfitz/http2 is the only exception and will be eliminated in the future).
ii) your way to import glog will introduce a lot of trouble when importing grpc-go into google internal code base.

@iamqizhao
Copy link
Contributor

Moving this into grpc repo is the way to go if you are not against it.

@bufdev
Copy link
Contributor Author

bufdev commented Sep 15, 2015

On ii) - I can rename it to glogger again, so there's no confusion. In general though, that glog package would only ever be imported once, probably in a main package, it shouldn't be referenced in general i.e.:

import _ "go.edge.io/dlog/glog" // should be the only time you have to import

On moving it into grpc - I'm generally fine with this, I just wanted to use dlog in the future for other libraries (mostly my own), so making it grpc-specific is no fun for that. But with that said, totally get the "no external dependencies", and since for my own stuff I just use protolog...eh why not, let's just move it in.

I'll move dlog to grpc/grpclog, and add all the package comments/import etc, how does that sound? I'll probably just do a new PR so there's no wasted commits. If you give me the go (pun intended?), I'll write up this PR now.

@dsymonds
Copy link
Contributor

I can't say I'm a fan of the trend this is extending. Logging has been made over-complicated. Arguably grpc-go shouldn't even need to use glog. There's too many places where grpc-go logs, where it should instead report via some better channel.

@bufdev
Copy link
Contributor Author

bufdev commented Sep 15, 2015

I would say there's an argument to be made that grpc does too much logging in some places, but I think that's another concept that should be tackled - what this PR (and the equivalent one over on grpc-gateway) is trying to cover is that every library is choosing their own logger, and it makes end-user applications hard to coalesce all logging to a preferred system. Just my two cents.

@bufdev bufdev mentioned this pull request Sep 15, 2015
Closed
@bufdev
Copy link
Contributor Author

bufdev commented Sep 15, 2015

@iamqizhao I just put the other PR together for your review since it only took a few minutes

@bufdev
Copy link
Contributor Author

bufdev commented Sep 15, 2015

@dsymonds I'm pretty sure you saw this since you're a collaborator but just in case, this was our original discussion re: grpclog: #190

The main thing is it's hard to capture grpc's logs in some systems (well I guess that's empirical evidence, but ya, the systems I've worked on :) ) because not everyone uses glog. So you end up either ignoring grpc's logs (the usual solution), which isn't great, whether grpc is logging too much or not, or you have to redirect glog into your own logging system.

@dsymonds
Copy link
Contributor

I'm not really trying to hold up this particular pull request, just expressing my dissatisfaction. Maybe channeling some of Rob Pike too. Too many things just log random information instead of thinking harder about why one would care about a particular situation arising, and thus all this great infrastructure around logging gets built to cope with that.

@bufdev
Copy link
Contributor Author

bufdev commented Sep 15, 2015

I totally agree actually - I think when adding a long-term log message, it should be something actionable or meaningful. And on that line, totally happy to help grpc-go with this, because it is definitely a bit noisy.

This was sort of the motivation (plugging my own package) for go.pedge.io/protolog - it allows you to do generic logging, but the main use is to make a proto message for everything you log (log "events"), and you get somewhat of an artificial barrier to adding a new log message - emotionally, you think more about "do I really want to log this/make a new log message for this" before adding a log line. Small example (not using protolog much yet, but we're getting there): https://github.com/pachyderm/pachyderm/tree/master/src/pps/server

But ya, end of plug :)

@iamqizhao
Copy link
Contributor

I always think the library developers are sorta biased when they add the logging to their lib. The logs they added probably only benefit themselves and not even readable to users. Therefore, I am very willing to listen to the feedback from our users on logging and make the corresponding changes. The current logging information were from our code reviewers and users besides myself. I agree that they are not in a very decent form at the current stage due to the long dev period without a refinement and lack of level logging support in the standard log package. We can definitely do a refinement and make it better in near future.

@bufdev
Copy link
Contributor Author

bufdev commented Sep 15, 2015

For sure :)

philips pushed a commit to philips/grpc-go that referenced this pull request Sep 17, 2015
README: document the no-deps design constraints
c904713 
See this issue for context:
grpc#341 (comment)
philips pushed a commit to philips/grpc-go that referenced this pull request Sep 17, 2015
README: document the no-deps design constraints
3262c64 
See this issue for context:
grpc#341 (comment)
@philips philips mentioned this pull request Sep 17, 2015
Closed
@iamqizhao iamqizhao closed this Sep 17, 2015
@yugui yugui mentioned this pull request Nov 4, 2015
Closed
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bufdev @iamqizhao @dsymonds