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

proposal: x/net: Accept client-provided logger for http2 debug logs in addition to GODEBUG=http2debug=1,2 #52734

Open
zachwalton opened this issue May 6, 2022 · 8 comments

Comments

@zachwalton
Copy link

@zachwalton zachwalton commented May 6, 2022

This allows me to get HTTP/2 debug logs: https://github.com/golang/net/blob/290c469a71a567d354e4abd335577aba44c4bde4/http2/http2.go#L41-L50

...But unlike the gRPC package, I can't specify my own logger. I also can't change the log level at runtime (I can toggle VerboseLogs since that's exported, but not logFrameWrites and logFrameReads).

For logging format consistency and runtime configuration of log levels it would be great to provide my own logger. I suppose this would also require a switch to level-based logging instead of e.g. log.Printf(); I don't know if the structure of logs is covered by the Go compatibility guarantee and if so this would need to be done in a way that doesn't break it (maybe all debug level with no formatting change in the default logger implementation?)

Thanks for considering this!

@gopherbot gopherbot added this to the Unreleased milestone May 6, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 11, 2022

CC @neild @bradfitz

Do you have a suggested API? Thanks.

@ianlancetaylor ianlancetaylor removed this from the Unreleased milestone May 11, 2022
@ianlancetaylor ianlancetaylor added this to the Proposal milestone May 11, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 11, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 18, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals May 18, 2022
@seankhliao seankhliao changed the title x/net: Accept client-provided logger for http2 debug logs in addition to GODEBUG=http2debug=1,2 proposal: x/net: Accept client-provided logger for http2 debug logs in addition to GODEBUG=http2debug=1,2 May 18, 2022
@zachwalton
Copy link
Author

@zachwalton zachwalton commented May 19, 2022

Do you have a suggested API? Thanks.

In the spirit of minimal change and consistency with the google.golang.org/grpc/grpclog package, I'd suggest it could be as simple as http2.SetLogger(l Logger) that updates a package global, where Logger is identical to grpclog.LoggerV2.

I would then personally be ok with all logs going to debug, but otherwise the implementer will need to determine the right log level for every existing log line. Then the default implementation can log in whatever format makes sense, probably the same as it does today if compatibility is important.

Alternatively (I can spec this out with more effort if interested), each struct in the package could have a Logger field with the same interface and optionally a (new) constructor to set the logger. However, without a corresponding change to grpclog to wire through the user-provided logger I think the utility of this would be limited, since as the package states very few people are using the http2 package directly.

@rsc
Copy link
Contributor

@rsc rsc commented May 25, 2022

Adding grpclog.LoggerV2 to any package in std is a non-starter. That's an enormous interface. What about *log.Logger instead (from the standard log package)? Or just an io.Writer?

We are also thinking about whether to revamp logging in std generally. Maybe we should hold this for that work. /cc @jba

@neild
Copy link
Contributor

@neild neild commented May 25, 2022

Getting useful log information and directing it to a desired location are reasonable desires, but I don't think slapping an additional public API on the http2 package's existing logs is the right thing to do. What about net/http and HTTP/1 logs? TLS? Are there other standard library packages that should have logging? What kinds of events should we be logging in all of these?

The http2 package has the ability to emit debug logs, but they're extremely ad hoc and there pretty much only for debugging the HTTP/2 implementation.

@zachwalton
Copy link
Author

@zachwalton zachwalton commented May 25, 2022

What about *log.Logger instead (from the standard log package)? Or just an io.Writer?

An interface would be better than *log.Logger IMO. io.Writer would be totally fine! Let me know if you want me to come up with a more complete proposal based on that.

What about net/http and HTTP/1 logs? TLS?

Well, as someone who works on those at the protocol level I'd say yes (if it's a cheap language addition) but I recognize that's relatively niche.

The http2 package has the ability to emit debug logs, but they're extremely ad hoc and there pretty much only for debugging the HTTP/2 implementation.

Just to provide a counter example, I got to this point because I was debugging random stalls over a high-throughput gRPC connection and it turned out the HTTP/2 debug logs held the key to the fix:

2022/05/06 18:45:48 http2: Framer 0xc0000f0c40: wrote WINDOW_UPDATE len=4 (conn) incr=4107

Seeing a flood of these told me I needed to increase initial connection window sizes on both ends; I think the only way I could have tackled this problem without logs was to start trying knobs until something panned out.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 1, 2022

We should probably put this on hold until we have a clearer story about what to do about logging generally, starting with package log.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 1, 2022

Placed on hold.
— rsc for the proposal review group

@rsc rsc moved this from Active to Hold in Proposals Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

5 participants