-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
channelz: log on channelz trace events and trace on channelz relevant logs. #3329
channelz: log on channelz trace events and trace on channelz relevant logs. #3329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach seems fine. Some comments inline.
@menghanl noticed that the depth
may pretty easily be a constant when we bring in module-level logging. If we do all our logging/channelz-ing through module-level wrappers, we can probably accomplish this fairly easily & naturally.
grpclog/loggerv2.go
Outdated
|
||
// DepthLogger is a LoggerV2 that can log at different call frame. | ||
type DepthLogger interface { | ||
LoggerV2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove LoggerV2 here; add to comment something like:
// ... If a logger implements DepthLogger, the below functions will be called with
// the appropriate stack depth set for trivial functions the logger may ignore.
//
// This API is EXPERIMENTAL.
grpclog/grpclog.go
Outdated
@@ -50,6 +50,15 @@ func Infoln(args ...interface{}) { | |||
logger.Infoln(args...) | |||
} | |||
|
|||
// InfoDepth logs to the INFO log at the specified depth. | |||
func InfoDepth(depth int, args ...interface{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move these to internal/grpclog
so they are not visible to users.
To avoid the circular dependency (grpclog
depends on internal/grpclog
and vice-versa) and also avoid the type assertion where this is used:
package internal/grpclog
:
- Copy
DepthLogger
andV2Logger
interfaces here (unfortunate but this is internal). Type aliases would be better, but those will make the godoc ofgrpclog
bad. - Add globals:
var DepthLogger DepthLogger
andvar Logger V2Logger
. - Add
func InfoDepth
/etc, usingDepthLogger
if non-nil, or callingLogger.Info
/etc otherwise.- Our packages call these functions instead of via the
grpc/grpclog
package.
- Our packages call these functions instead of via the
package grpclog
: imports internal/grpclog
- Remove global
logger
. - Set
internal/grpclog.Logger
andinternal/grpclog.DepthLogger
together, clearing theDepthLogger
if theV2Logger
does not implement it. - Use
internal/grpclog.Logger
instead of the globallogger
.
Change to log (using grpclog) on channelz trace events and generate channelz trace events on grpclogs. Only grpclogs in proximity of / relevant to a channelz channel were modified.
updates #2939