Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a metric for hosts not being found in resolver #5414

Merged
merged 9 commits into from
Dec 22, 2023
1 change: 1 addition & 0 deletions cmd/server/cadence/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ func (s *server) startService() common.Daemon {
params.MembershipResolver, err = membership.NewResolver(
peerProvider,
params.Logger,
params.MetricsClient,
)
if err != nil {
log.Fatalf("error creating membership monitor: %v", err)
Expand Down
11 changes: 9 additions & 2 deletions common/membership/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ package membership
import (
"errors"
"fmt"
"github.com/uber/cadence/common/metrics"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imports are messed up.

"sync/atomic"

"github.com/uber/cadence/common"
Expand Down Expand Up @@ -80,7 +81,8 @@ type (

// MultiringResolver uses ring-per-service for membership information
type MultiringResolver struct {
status int32
metrics metrics.Client
status int32

provider PeerProvider
rings map[string]*ring
Expand All @@ -92,20 +94,23 @@ var _ Resolver = (*MultiringResolver)(nil)
func NewResolver(
provider PeerProvider,
logger log.Logger,
metrics metrics.Client,
) (*MultiringResolver, error) {
return NewMultiringResolver(service.List, provider, logger.WithTags(tag.ComponentServiceResolver)), nil
return NewMultiringResolver(service.List, provider, logger.WithTags(tag.ComponentServiceResolver), metrics), nil
}

// NewMultiringResolver creates hashrings for all services
func NewMultiringResolver(
services []string,
provider PeerProvider,
logger log.Logger,
metrics metrics.Client,
) *MultiringResolver {
rpo := &MultiringResolver{
status: common.DaemonStatusInitialized,
provider: provider,
rings: make(map[string]*ring),
metrics: metrics,
}

for _, s := range services {
Expand Down Expand Up @@ -193,6 +198,7 @@ func (rpo *MultiringResolver) Unsubscribe(service string, name string) error {
func (rpo *MultiringResolver) Members(service string) ([]HostInfo, error) {
ring, err := rpo.getRing(service)
if err != nil {
rpo.metrics.Scope(metrics.ResolverHostNotFoundScope).IncCounter(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to move to LookupByAddress() which calls Members?
Then it would be clear - every time LookupByAddress returns empty HostInfo we emit the metric.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you're right, I was being a bit hurried and this doesn't make sense

return nil, err
}
return ring.Members(), nil
Expand All @@ -208,6 +214,7 @@ func (rpo *MultiringResolver) LookupByAddress(service, address string) (HostInfo
return m, nil
}
}
rpo.metrics.Scope(metrics.ResolverHostNotFoundScope).IncCounter(1)
return HostInfo{}, errors.New("host not found")
}

Expand Down
2 changes: 2 additions & 0 deletions common/membership/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
package membership

import (
"github.com/uber/cadence/common/metrics"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imports are messed up

"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -136,5 +137,6 @@ func newTestResolver(t *testing.T) (*MultiringResolver, *MockPeerProvider) {
testServices,
pp,
log.NewNoop(),
metrics.NewNoopMetricsClient(),
), pp
}
4 changes: 4 additions & 0 deletions common/metrics/defs.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ const (
PersistenceUpdateDynamicConfigScope
// PersistenceShardRequestCountScope tracks number of persistence calls made to each shard
PersistenceShardRequestCountScope

// ResolverHostNotFoundScope is a simple low level error indicating a lookup failed in the membership resolver
ResolverHostNotFoundScope
// HistoryClientStartWorkflowExecutionScope tracks RPC calls to history service
HistoryClientStartWorkflowExecutionScope
// HistoryClientDescribeHistoryHostScope tracks RPC calls to history service
Expand Down Expand Up @@ -1353,6 +1356,7 @@ var ScopeDefs = map[ServiceIdx]map[int]scopeDefinition{
PersistenceFetchDynamicConfigScope: {operation: "FetchDynamicConfig"},
PersistenceUpdateDynamicConfigScope: {operation: "UpdateDynamicConfig"},
PersistenceShardRequestCountScope: {operation: "ShardIdPersistenceRequest"},
ResolverHostNotFoundScope: {operation: "ResolverHostNotFound"},

ClusterMetadataArchivalConfigScope: {operation: "ArchivalConfig"},

Expand Down