MetadataMgr: Move back to controllerhost #47
Conversation
| @@ -173,7 +173,7 @@ func NewController(cfg configure.CommonAppConfig, sVice *common.Service, metadat | |||
|
|
|||
| context.dstLock = lockMgr | |||
| context.m3Client = metrics.NewClient(instance.Service.GetMetricsReporter(), metrics.Controller) | |||
| context.mm = common.NewMetadataMgr(metadataClient, context.m3Client, context.log) | |||
| context.mm = NewMetadataMgr(metadataClient, context.m3Client, context.log) | |||
There was a problem hiding this comment.
instead of metadataClient, you should probably use "instance.mClient" .. after initializing instance.mClient (above) to use "common/metadata" metadataMgr .. that emits metrics. this would ensure that all calls to metadata (via the metadataClient) would emit necessary metrics.
import mm "github.com/uber/cherami-server/common/metadata"
...
instance.mClient = mm.NewMetadataMetricsMgr(metadataClient, context.m3Client, logger)
| m.m3.IncCounter(metrics.MetadataReadDestinationScope, metrics.MetadataFailures) | ||
| if _, ok := err.(*shared.EntityNotExistsError); !ok { | ||
| m.m3.IncCounter(metrics.MetadataReadDestinationScope, metrics.MetadataFailures) | ||
| } |
There was a problem hiding this comment.
this is something i had missed .. thanks for taking care of it.
| @@ -173,7 +174,8 @@ func NewController(cfg configure.CommonAppConfig, sVice *common.Service, metadat | |||
|
|
|||
| context.dstLock = lockMgr | |||
| context.m3Client = metrics.NewClient(instance.Service.GetMetricsReporter(), metrics.Controller) | |||
| context.mm = common.NewMetadataMgr(metadataClient, context.m3Client, context.log) | |||
| mClient := metadata.NewMetadataMetricsMgr(metadataClient, context.m3Client, context.log) | |||
There was a problem hiding this comment.
you should probably update the global (Mcp.mClient) to use the instance that emits metrics .. so that other sub-components (within controllerhost) that calls into metadata would automatically emit metrics. i see that retention manager instantiate another instance of metadata-metrics within its code -- we should probably remove that.
There was a problem hiding this comment.
good idea, done !
|
Changes Unknown when pulling abdd1bc on md-mgr into ** on master**. |
This patch moves the MetadataMgr interface & impl within the controller (where it originally belonged). Now the controller implementation is just a decorator around the common metadata client (that emits metrics).