core: added changes to make service config more explicit by logging required info #11917
Conversation
shivaspeaks
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
| InternalConfigSelector prevConfig = configSelector.get(); | ||
| configSelector.set(config); | ||
| configSelector.set(newConfig); | ||
| channelLogger.log(ChannelLogLevel.INFO, |
There was a problem hiding this comment.
This could be right but this can also lead to spamming as config update can happen per RPC.
Perhaps we should discuss/ rethink on the requirement for this logging. Or maybe it will be better if we log only when default config is applied and expected retry config is not applied when retry is enabled and service config is present. And we also need to figure out how it will help the user, about why the given config was not applied! Logging that will be much better.
There was a problem hiding this comment.
gRPC does not log at a default-enabled log level (java.util.logging INFO or higher), unless SEVERE and there's no other way to handle it. However, here you're using the channel logger, which uses FINER java.util.logging level.
In general, I don't think typical logging is appropriate for this issue. (We can have some debug logging, but it'd be mostly for us; users would be unlikely to see it.) I think it'd be best to expose the current service config to channelz, although when I mentioned it to other TLs in the past it didn't gain traction. This "channel logger" does dump things in channelz when configured to do so, and then you can use grpcdebug to see the logs. Not the easiest, but could work. Importantly, we wouldn't want to log when the config is unchanged.
But overall, I think this issue needs a design before an implementation.
There was a problem hiding this comment.
Perhaps we should discuss/ rethink on the requirement for this logging.
There was no such requirement of logging specifically.
core: added changes to make service config more explicit by logging required details of transported JSON
Fixes: #5758