Copilot commented on code in PR #3685:
URL: https://github.com/apache/dubbo-go/pull/3685#discussion_r3819140970
##########
registry/servicediscovery/service_instances_changed_listener_impl.go:
##########
@@ -277,8 +281,23 @@ func GetMetadataInfo(app string, instance
registry.ServiceInstance, revision str
initCache(app)
})
cacheKey := metadataCacheKey(app, registryId, revision)
- if metadataInfo, ok := metaCache.Get(cacheKey); ok {
- return metadataInfo.(*info.MetadataInfo), nil
+ if cached, ok := metaCache.Get(cacheKey); ok {
+ logger.Debugf("[Metadata][Cache] hit app=%s registry=%s
revision=%s host=%s",
+ app, registryId, revision, instance.GetHost())
+ event :=
metricsMetadata.NewMetadataMetricTimeEvent(metricsMetadata.MetadataCache)
+ event.Succ = true
+ event.Attachment[metricsMetadata.TagProviderApp] = app
+ metrics.Publish(event)
+ metadataInfo := cached.(*info.MetadataInfo)
+ if metadataInfo == nil {
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceCache,
metricsMetadata.StorageTypeCache, metricsMetadata.ResultFailure,
+ perrors.New("typed nil metadata in cache"))
+ return nil, nil
+ }
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceCache,
metricsMetadata.StorageTypeCache, metricsMetadata.ResultSuccess, nil)
+ return metadataInfo, nil
}
Review Comment:
`cached.(*info.MetadataInfo)` can still panic if the cache returns a nil
interface value or an unexpected type (CacheManager stores `any` and can load
`Item.Value` from disk). Consider using a checked type assertion (`mi, ok :=
cached.(*info.MetadataInfo)`) and handling `cached == nil`/`!ok` by deleting
the entry + treating it as a miss (and publishing a failure event), rather than
assuming the assertion always succeeds.
##########
registry/servicediscovery/service_instances_changed_listener_impl.go:
##########
@@ -315,31 +343,67 @@ func GetMetadataInfo(app string, instance
registry.ServiceInstance, revision str
if reportErr != nil {
// Wrap rpcErr so callers can use errors.Is/As
on the primary failure;
// reportErr is annotated as context since it
triggered the fallback.
- return nil, perrors.Wrapf(err,
+ wrappedErr := perrors.Wrapf(err,
"both paths failed, reportErr: %v",
reportErr)
+ publishMetadataFetchEvent(app, registryId,
revision, instance.GetHost(),
+ metricsMetadata.SourceRpc,
metricsMetadata.StorageTypeRemote, metricsMetadata.ResultFailure, wrappedErr)
+ return nil, wrappedErr
}
// reportErr was nil — the report returned nil metadata
and RPC also failed.
- return nil, perrors.Wrapf(err,
+ wrappedErr := perrors.Wrapf(err,
"RPC fallback failed after report returned nil
metadata")
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceRpc,
metricsMetadata.StorageTypeRemote, metricsMetadata.ResultFailure, wrappedErr)
+ return nil, wrappedErr
}
if metadataInfo == nil {
- return nil, perrors.Errorf("got nil metadata from RPC
app=%s registry=%s revision=%s",
+ rpcErr := perrors.Errorf("got nil metadata from RPC
app=%s registry=%s revision=%s",
app, registryId, revision)
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceRpc,
metricsMetadata.StorageTypeRemote, metricsMetadata.ResultFailure, rpcErr)
+ return nil, rpcErr
}
metaCache.Set(cacheKey, metadataInfo)
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceFallback,
metricsMetadata.StorageTypeRemote, metricsMetadata.ResultSuccess, nil)
return metadataInfo, nil
}
// Non-remote storage type ("local" or absent): fetch metadata via RPC
directly.
metadataInfo, err = metadata.GetMetadataFromRpc(revision, instance)
if err != nil {
- return nil, perrors.Wrapf(err,
+ wrappedErr := perrors.Wrapf(err,
"failed app=%s registry=%s revision=%s", app,
registryId, revision)
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceRpc,
metricsMetadata.StorageTypeLocal, metricsMetadata.ResultFailure, wrappedErr)
+ return nil, wrappedErr
}
if metadataInfo == nil {
- return nil, perrors.Errorf("got nil metadata from RPC app=%s
registry=%s revision=%s",
+ rpcErr := perrors.Errorf("got nil metadata from RPC app=%s
registry=%s revision=%s",
app, registryId, revision)
+ publishMetadataFetchEvent(app, registryId, revision,
instance.GetHost(),
+ metricsMetadata.SourceRpc,
metricsMetadata.StorageTypeLocal, metricsMetadata.ResultFailure, rpcErr)
+ return nil, rpcErr
}
metaCache.Set(cacheKey, metadataInfo)
+ publishMetadataFetchEvent(app, registryId, revision, instance.GetHost(),
+ metricsMetadata.SourceRpc, metricsMetadata.StorageTypeLocal,
metricsMetadata.ResultSuccess, nil)
return metadataInfo, nil
}
+
+func publishMetadataFetchEvent(app, registryId, revision, host, source,
storageType, result string, err error) {
+ event :=
metricsMetadata.NewMetadataMetricTimeEvent(metricsMetadata.MetadataFetch)
+ event.Attachment[metricsMetadata.TagProviderApp] = app
+ event.Attachment[metricsMetadata.TagSource] = source
+ event.Attachment[metricsMetadata.TagStorageType] = storageType
+ event.Attachment[metricsMetadata.TagResult] = result
+ metrics.Publish(event)
Review Comment:
Events created via `NewMetadataMetricTimeEvent` typically set `Succ` and
`End` before publishing (e.g. metadata/report_instance.go and
service_discovery_registry.go). `publishMetadataFetchEvent` publishes without
setting `End`/`Succ`, which makes `CostMs()` invalid and is inconsistent with
existing metric events. Consider setting `event.Succ` based on `result` and
`event.End = time.Now()` (and likewise for the MetadataCache events above).
##########
metrics/metadata/collector.go:
##########
@@ -88,6 +92,26 @@ func (c *MetadataMetricCollector)
handleSubscribeService(event *MetadataMetricEv
c.R.Rt(metrics.NewMetricId(subscribeServiceRt, level),
&metrics.RtOpts{}).Observe(event.CostMs())
}
+func (c *MetadataMetricCollector) handleMetadataCache(event
*MetadataMetricEvent) {
+ labels := metrics.GetApplicationLevel().Tags()
+ labels[TagProviderApp] = event.Attachment[TagProviderApp]
+ c.R.Counter(metrics.NewMetricIdByLabels(metadataCacheNum, labels)).Inc()
+ if event.Succ {
+ c.R.Counter(metrics.NewMetricIdByLabels(metadataCacheHit,
labels)).Inc()
+ } else {
+ c.R.Counter(metrics.NewMetricIdByLabels(metadataCacheMiss,
labels)).Inc()
+ }
+}
+
+func (c *MetadataMetricCollector) handleMetadataFetch(event
*MetadataMetricEvent) {
+ labels := metrics.GetApplicationLevel().Tags()
+ labels[TagProviderApp] = event.Attachment[TagProviderApp]
+ labels[TagSource] = event.Attachment[TagSource]
+ labels[TagStorageType] = event.Attachment[TagStorageType]
+ labels[TagResult] = event.Attachment[TagResult]
+ c.R.Counter(metrics.NewMetricIdByLabels(metadataFetchNum, labels)).Inc()
+}
Review Comment:
The new `handleMetadataCache` / `handleMetadataFetch` behavior (label
composition and hit/miss/result counters) is untested. Since this package
already has tests, please add unit tests that publish
MetadataCache/MetadataFetch events and assert the expected counters/labels are
emitted.
##########
registry/servicediscovery/service_instances_changed_listener_impl.go:
##########
@@ -296,6 +315,13 @@ func GetMetadataInfo(app string, instance
registry.ServiceInstance, revision str
}
}
+ logger.Infof("[Metadata][Cache] miss app=%s registry=%s revision=%s
host=%s storageType=%s",
+ app, registryId, revision, instance.GetHost(),
metadataStorageType)
+ event :=
metricsMetadata.NewMetadataMetricTimeEvent(metricsMetadata.MetadataCache)
+ event.Succ = false
+ event.Attachment[metricsMetadata.TagProviderApp] = app
+ metrics.Publish(event)
Review Comment:
`logger.Infof("[Metadata][Cache] miss ...")` runs on every cache miss and
includes host/revision, which can produce very high log volume on startup or
large instance sets. Consider downgrading to Debug, sampling/ratelimiting, or
logging miss summary at a higher level to avoid flooding production logs.
--
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]
