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

channelz: log on channelz trace events and trace on channelz relevant logs. #3329

Merged
merged 16 commits into from
Feb 14, 2020
Merged

channelz: log on channelz trace events and trace on channelz relevant logs. #3329

merged 16 commits into from
Feb 14, 2020

Conversation

GarrettGutierrez1
Copy link
Contributor

@GarrettGutierrez1 GarrettGutierrez1 commented Jan 21, 2020

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

@dfawley dfawley self-requested a review January 23, 2020 21:14
@dfawley dfawley self-assigned this Jan 23, 2020
@GarrettGutierrez1 GarrettGutierrez1 added no release notes Type: Feature New features or improvements in behavior labels Jan 28, 2020
internal/channelz/logging.go Outdated Show resolved Hide resolved
internal/channelz/logging.go Show resolved Hide resolved
internal/channelz/logging.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
internal/channelz/funcs.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned GarrettGutierrez1 and unassigned dfawley Jan 29, 2020
@dfawley dfawley assigned GarrettGutierrez1 and unassigned dfawley Feb 3, 2020
Copy link
Member

@dfawley dfawley left a 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.


// DepthLogger is a LoggerV2 that can log at different call frame.
type DepthLogger interface {
LoggerV2
Copy link
Member

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.

@@ -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{}) {
Copy link
Member

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 and V2Logger interfaces here (unfortunate but this is internal). Type aliases would be better, but those will make the godoc of grpclog bad.
  • Add globals: var DepthLogger DepthLogger and var Logger V2Logger.
  • Add func InfoDepth/etc, using DepthLogger if non-nil, or calling Logger.Info/etc otherwise.
    • Our packages call these functions instead of via the grpc/grpclog package.

package grpclog: imports internal/grpclog

  • Remove global logger.
  • Set internal/grpclog.Logger and internal/grpclog.DepthLogger together, clearing the DepthLogger if the V2Logger does not implement it.
  • Use internal/grpclog.Logger instead of the global logger.

@dfawley dfawley assigned GarrettGutierrez1 and unassigned dfawley Feb 10, 2020
clientconn.go Outdated Show resolved Hide resolved
grpclog/loggerv2.go Outdated Show resolved Hide resolved
internal/channelz/funcs.go Outdated Show resolved Hide resolved
internal/grpclog/grpclog.go Show resolved Hide resolved
internal/grpclog/grpclog.go Show resolved Hide resolved
@GarrettGutierrez1 GarrettGutierrez1 changed the title Log on channelz trace events and trace on channelz relevant logs. channelz: log on channelz trace events and trace on channelz relevant logs. Feb 14, 2020
@GarrettGutierrez1 GarrettGutierrez1 merged commit fff75ae into grpc:master Feb 14, 2020
@GarrettGutierrez1 GarrettGutierrez1 deleted the channelz-to-logs branch February 18, 2020 20:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants