fix(resolverWrapper): only run logging logic in verbose mode #8622
Conversation
The default verbosity level is 0, so the first part of this logic always evaluated to false, meaning that this would never be reached unless the user sets `GRPC_GO_LOG_VERBOSITY_LEVEL=-1`. This should be disabled by default. This also swaps the and for an or, so that if channelz is off, the logic does not run either, which wasn't the case before. This method has been using lots of CPU for some of our usecases, as serializing lots of addresseses repeatedly can be costly.
|
|
| // state received from resolver implementations. | ||
| func (ccr *ccResolverWrapper) addChannelzTraceEvent(s resolver.State) { | ||
| if !logger.V(0) && !channelz.IsOn() { | ||
| if !logger.V(2) || !channelz.IsOn() { |
There was a problem hiding this comment.
We don't want the verbosity check to affect whether or not channelz traces are being added. The channelz.Infof adds a log and a trace.
I would be OK with removing the verbosity check completely and only checking if channelz is turned on.
There was a problem hiding this comment.
Ah, I just read the PR description and noticed that there is a change from && to ||. Even with that change, I still think that totally getting rid of the verbosity check would be better, since it would retain existing behavior instead of changing it.
|
I also just noticed that you are concerned about CPU usage of this function. I think this was raised previously where we suggested that we could get rid of the |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
2 participants
The default verbosity level is 0, so the first part of this logic always evaluated to false, meaning that this would never be reached unless the user sets
GRPC_GO_LOG_VERBOSITY_LEVEL=-1. This should be disabled by default.This also swaps the and for an or, so that if channelz is off, the logic does not run either, which wasn't the case before.
This method has been using lots of CPU for some of our usecases, as serializing lots of addresseses repeatedly can be costly.
RELEASE NOTES: