Copilot commented on code in PR #3346:
URL: https://github.com/apache/dubbo-go/pull/3346#discussion_r3329579792
##########
registry/servicediscovery/service_mapping_change_listener_impl.go:
##########
@@ -95,7 +95,7 @@ func (lstn *ServiceMappingChangedListenerImpl) OnEvent(e
observer.Event) error {
}
for _, service := range newServiceNames.Values() {
if !oldServiceNames.Contains(service) {
- logger.Infof("Service-application mapping changed for
service: %s, new applications: %q, old applications: %q.",
lstn.serviceUrl.ServiceKey(), oldServiceNames, newServiceNames)
+ logger.Infof("[Registry][ServiceDiscovery]
service-application mapping changed for service=%s, newApp=%q oldApp=%q",
lstn.serviceUrl.ServiceKey(), oldServiceNames, newServiceNames)
Review Comment:
The updated message labels the first set as `newApp`, but the argument
passed there is `oldServiceNames` (and `newServiceNames` is passed as
`oldApp`). This makes the diagnostic log report the old and new mappings
backwards when a mapping changes.
##########
registry/servicediscovery/service_instances_changed_listener_impl.go:
##########
@@ -50,7 +50,7 @@ func initCache(app string) {
fileName := constant.DefaultMetaFileName + app
cache, err := store.NewCacheManager(constant.DefaultMetaCacheName,
fileName, time.Minute*10, constant.DefaultEntrySize, true)
if err != nil {
- logger.Fatal("Failed to create cache [%s],the err is %v",
constant.DefaultMetaCacheName, err)
+ logger.Fatal("[Registry][ServiceDiscovery] failed to create
cache [%s],the err is %v", constant.DefaultMetaCacheName, err)
Review Comment:
This still calls the non-formatting `logger.Fatal` with `%` placeholders and
extra arguments, so the fatal log will not render the cache name or error. Use
the formatted variant here, just as the surrounding sweep does for other logger
calls with format arguments.
##########
registry/nacos/service_discovery.go:
##########
@@ -323,7 +323,7 @@ func (n *nacosServiceDiscovery) AddListener(listener
registry.ServiceInstancesCh
}
if e != nil {
- logger.Errorf("Dispatching event got
exception, service name: %s, err: %v", serviceName, err)
+ logger.Errorf("[Registry][Nacos]
dispatching event got exception, serviceName=%s err=%v", serviceName, err)
Review Comment:
When `OnEvent` returns an error, this log prints the callback parameter
`err` instead of the dispatch error stored in `e`, so dispatch failures are
reported as nil or as an unrelated subscribe error. Log `e` here so the
listener failure is visible.
##########
registry/nacos/service_discovery.go:
##########
@@ -268,7 +268,7 @@ func (n *nacosServiceDiscovery)
registerInstanceListener(listener registry.Servi
for _, t := range listener.GetServiceNames().Values() {
serviceName, ok := t.(string)
if !ok {
- logger.Errorf("service name error %s", t)
+ logger.Errorf("[Registry][Nacos] service name error,
name=%s", t)
Review Comment:
This branch is entered only when `t` is not a string, but the log formats it
with `%s`. That produces `%!s(...)` output for the value that failed the type
assertion, making the error log hard to diagnose; use `%v` for the arbitrary
value.
##########
remoting/getty/readwriter.go:
##########
@@ -82,18 +82,18 @@ func (p *RpcClientPackageHandler) Write(ss getty.Session,
pkg any) ([]byte, erro
if ok {
buf, err := (p.client.codec).EncodeResponse(res)
if err != nil {
- logger.Warnf("binary.Write(res{%#v}) = err{%#v}", req,
perrors.WithStack(err))
+ logger.Warnf("[Remoting][Getty] binary.Write(res=%#v) =
err=%#v", req, perrors.WithStack(err))
Review Comment:
If encoding the response fails, this log prints `req`, which is nil in the
response branch, instead of the response that failed to encode. That hides the
failing package details in the error path.
##########
registry/zookeeper/service_discovery.go:
##########
@@ -241,7 +241,7 @@ func (zksd *zookeeperServiceDiscovery) AddListener(listener
registry.ServiceInst
for _, t := range listener.GetServiceNames().Values() {
serviceName, ok := t.(string)
if !ok {
- logger.Errorf("service name error %s", t)
+ logger.Errorf("[Registry][Zookeeper] service name
error, name=%s", t)
Review Comment:
This branch is entered only when `t` is not a string, but the log formats it
with `%s`. That produces `%!s(...)` output for the value that failed the type
assertion, making the error log hard to diagnose; use `%v` for the arbitrary
value.
##########
server/options.go:
##########
@@ -104,8 +104,8 @@ func (srvOpts *ServerOptions) init(opts ...ServerOption)
error {
return perrors.Errorf("The adaptive service is
disabled, " +
"adaptive service verbose should be disabled
either.")
}
- logger.Infof("adaptive service verbose is enabled.")
- logger.Debugf("debug-level info could be shown.")
+ logger.Info("[Server] adaptive service verbose is enabled")
+ logger.Debug("[Server] debug-level info could be shown")
Review Comment:
The PR description says a new `tools/loggercheck` analyzer is added and
wired into `make lint`, but this checkout has no `tools/loggercheck` directory
and the Makefile contains no `loggercheck` target/reference. Without those
changes, the advertised regression check is not present.
##########
registry/zookeeper/service_discovery.go:
##########
@@ -258,7 +258,7 @@ func (zksd *zookeeperServiceDiscovery) AddListener(listener
registry.ServiceInst
for _, t := range listener.GetServiceNames().Values() {
serviceName, ok := t.(string)
if !ok {
- logger.Errorf("service name error %s", t)
+ logger.Errorf("[Registry][Zookeeper] service name
error, name=%s", t)
Review Comment:
This branch is entered only when `t` is not a string, but the log formats it
with `%s`. That produces `%!s(...)` output for the value that failed the type
assertion, making the error log hard to diagnose; use `%v` for the arbitrary
value.
##########
remoting/getty/readwriter.go:
##########
@@ -145,16 +145,16 @@ func (p *RpcServerPackageHandler) Write(ss getty.Session,
pkg any) ([]byte, erro
buf, err := (p.server.codec).EncodeRequest(req)
bufLength := buf.Len()
if bufLength > maxBufLength {
- logger.Errorf("Data length %d too large, max payload
%d", bufLength-impl.HEADER_LENGTH, srvConf.GettySessionParam.MaxMsgLen)
+ logger.Errorf("[Remoting][Getty] data length %d too
large, max payload=%d", bufLength-impl.HEADER_LENGTH,
srvConf.GettySessionParam.MaxMsgLen)
return nil, perrors.Errorf("Data length %d too large,
max payload %d", bufLength-impl.HEADER_LENGTH,
srvConf.GettySessionParam.MaxMsgLen)
}
if err != nil {
- logger.Warnf("binary.Write(req{%#v}) = err{%#v}", res,
perrors.WithStack(err))
+ logger.Warnf("[Remoting][Getty] binary.Write(req=%#v) =
err=%#v", res, perrors.WithStack(err))
Review Comment:
If encoding the request fails, this log prints `res`, which is nil in the
request branch, instead of the request that failed to encode. That hides the
failing package details in the error path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
