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
*: Limit calls to pretty.ToJSON
to more verbose debugging, warnings, and errors to improve performance
#7169
Conversation
…, and errors to improve performance.
LGTM! @zasweq can you also please have a pass through this PR? |
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.
LGTM.
@@ -194,5 +194,7 @@ func (ccr *ccResolverWrapper) addChannelzTraceEvent(s resolver.State) { | |||
} else if len(ccr.curState.Addresses) == 0 && len(s.Addresses) > 0 { | |||
updates = append(updates, "resolver returned new addresses") | |||
} | |||
channelz.Infof(logger, ccr.cc.channelz, "Resolver state updated: %s (%v)", pretty.ToJSON(s), strings.Join(updates, "; ")) | |||
if channelz.IsOn() || logger.V(2) { |
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.
What is the reason for the additional channelz.IsOn() bool conditional?
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.
It's similar to the reason for the logger.V(2)
check. We only want to call pretty.ToJSON
when we know we're going to use the result. channelz.Infof
can add a trace event, log, or both, so we want to call it when channelz data collection is on, we're logging at depth 2, or both.
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.
lgtm
Please don't merge this yet. Doug Fawley and I are still discussing this approach. |
I'm going to close this since I think we got the same win just from the Debugf calls. We can reopen or recreate in the future if needed. |
RELEASE NOTES: None