Skip to content

[common/log] Unify logger package #6779

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

Merged
merged 4 commits into from
Apr 2, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bench/lib/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"github.com/uber-go/tally"
"go.uber.org/zap"

"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log"
)

const (
Expand Down Expand Up @@ -59,7 +59,7 @@ func NewRuntimeContext(cfg *Config) (*RuntimeContext, error) {
return nil, err
}

metricsScope := cfg.Metrics.NewScope(loggerimpl.NewLogger(logger), cfg.Bench.Name)
metricsScope := cfg.Metrics.NewScope(log.NewLogger(logger), cfg.Bench.Name)

if cfg.Cadence.ServiceName == "" {
cfg.Cadence.ServiceName = defaultCadenceServiceName
Expand Down
4 changes: 2 additions & 2 deletions canary/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import (
"go.uber.org/zap"
"google.golang.org/grpc/credentials"

"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log"
)

type canaryRunner struct {
Expand All @@ -56,7 +56,7 @@ func NewCanaryRunner(cfg *Config) (Runnable, error) {
return nil, fmt.Errorf("failed to create logger: %v", err)
}

metricsScope := cfg.Metrics.NewScope(loggerimpl.NewLogger(logger), "cadence-canary")
metricsScope := cfg.Metrics.NewScope(log.NewLogger(logger), "cadence-canary")

if cfg.Cadence.ServiceName == "" {
cfg.Cadence.ServiceName = CadenceServiceName
Expand Down
3 changes: 1 addition & 2 deletions cmd/server/cadence/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ import (
"github.com/uber/cadence/common/elasticsearch"
"github.com/uber/cadence/common/isolationgroup/isolationgroupapi"
cadencelog "github.com/uber/cadence/common/log"
"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log/tag"
"github.com/uber/cadence/common/membership"
"github.com/uber/cadence/common/messaging/kafka"
Expand Down Expand Up @@ -129,7 +128,7 @@ func (s *server) startService() common.Daemon {
if err != nil {
log.Fatal("failed to create the zap logger, err: ", err.Error())
}
params.Logger = loggerimpl.NewLogger(zapLogger).WithTags(tag.Service(params.Name))
params.Logger = cadencelog.NewLogger(zapLogger).WithTags(tag.Service(params.Name))

params.PersistenceConfig = s.cfg.Persistence

Expand Down
22 changes: 13 additions & 9 deletions cmd/server/cadence/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
package cadence

import (
"log"
"os"
"testing"
"time"
Expand All @@ -37,7 +36,9 @@ import (
"github.com/uber/cadence/common"
"github.com/uber/cadence/common/config"
"github.com/uber/cadence/common/dynamicconfig"
"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log"
"github.com/uber/cadence/common/log/tag"
"github.com/uber/cadence/common/log/testlogger"
"github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql"
"github.com/uber/cadence/common/resource"
"github.com/uber/cadence/common/service"
Expand All @@ -51,6 +52,8 @@ import (
type ServerSuite struct {
*require.Assertions
suite.Suite

logger log.Logger
}

func TestServerSuite(t *testing.T) {
Expand All @@ -60,6 +63,7 @@ func TestServerSuite(t *testing.T) {

func (s *ServerSuite) SetupTest() {
s.Assertions = require.New(s.T())
s.logger = testlogger.New(s.T())
}

/*
Expand All @@ -77,7 +81,7 @@ func (s *ServerSuite) TestServerStartup() {
var cfg config.Config
err := config.Load(env, configDir, zone, &cfg)
if err != nil {
log.Fatal("Config file corrupted.", err)
s.logger.Fatal("Config file corrupted.", tag.Error(err))
}

if os.Getenv("CASSANDRA_SEEDS") == "cassandra" {
Expand All @@ -97,11 +101,11 @@ func (s *ServerSuite) TestServerStartup() {
cfg.DynamicConfig.FileBased.Filepath = constructPathIfNeed(rootDir, cfg.DynamicConfig.FileBased.Filepath)

if err := cfg.ValidateAndFillDefaults(); err != nil {
log.Fatalf("config validation failed: %v", err)
s.logger.Fatal("config validation failed", tag.Error(err))
}
// cassandra schema version validation
if err := cassandra.VerifyCompatibleVersion(cfg.Persistence, gocql.All); err != nil {
log.Fatal("cassandra schema version compatibility check failed: ", err)
s.logger.Fatal("cassandra schema version compatibility check failed", tag.Error(err))
}

var daemons []common.Daemon
Expand All @@ -128,11 +132,11 @@ func TestSettingGettingZonalIsolationGroupsFromIG(t *testing.T) {
"zone-1", "zone-2",
}, nil)

dc := dynamicconfig.NewCollection(client, loggerimpl.NewNopLogger())
dc := dynamicconfig.NewCollection(client, log.NewNoop())

assert.NotPanics(t, func() {
fn := getFromDynamicConfig(resource.Params{
Logger: loggerimpl.NewNopLogger(),
Logger: log.NewNoop(),
}, dc)
out := fn()
assert.Equal(t, []string{"zone-1", "zone-2"}, out)
Expand All @@ -143,11 +147,11 @@ func TestSettingGettingZonalIsolationGroupsFromIGError(t *testing.T) {
ctrl := gomock.NewController(t)
client := dynamicconfig.NewMockClient(ctrl)
client.EXPECT().GetListValue(dynamicconfig.AllIsolationGroups, gomock.Any()).Return(nil, assert.AnError)
dc := dynamicconfig.NewCollection(client, loggerimpl.NewNopLogger())
dc := dynamicconfig.NewCollection(client, log.NewNoop())

assert.NotPanics(t, func() {
getFromDynamicConfig(resource.Params{
Logger: loggerimpl.NewNopLogger(),
Logger: log.NewNoop(),
}, dc)()
})
}
16 changes: 8 additions & 8 deletions common/cluster/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/uber/cadence/common/config"
"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log"
"github.com/uber/cadence/common/log/testlogger"
"github.com/uber/cadence/common/metrics"
)
Expand Down Expand Up @@ -226,7 +226,7 @@ func TestResolvingClusterVersion(t *testing.T) {
initialFailoverVersionC2: clusterName2,
},
metrics: metrics.NewNoopMetricsClient().Scope(0),
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
}

for name, td := range tests {
Expand Down Expand Up @@ -290,7 +290,7 @@ func TestIsPartOfTheSameCluster(t *testing.T) {
initialFailoverVersionC1: clusterName1,
initialFailoverVersionC2: clusterName2,
},
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
metrics: metrics.NewNoopMetricsClient().Scope(0),
}

Expand Down Expand Up @@ -532,7 +532,7 @@ func TestIsPartOfTheSameClusterAPIFixing(t *testing.T) {
},
useNewFailoverVersionOverride: func(domain string) bool { return false },
metrics: metrics.NewNoopMetricsClient().Scope(0),
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
}

for i := range tests {
Expand Down Expand Up @@ -774,7 +774,7 @@ func TestClusterNameForFailoverVersion(t *testing.T) {
},
metrics: metrics.NewNoopMetricsClient().Scope(0),
useNewFailoverVersionOverride: func(domain string) bool { return false },
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
}

for i := range tests {
Expand Down Expand Up @@ -811,7 +811,7 @@ func TestServerResolution(t *testing.T) {
},
metrics: metrics.NewNoopMetricsClient().Scope(0),
useNewFailoverVersionOverride: func(domain string) bool { return domain == domainToMigrate },
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
}

err := quick.Check(func(currentFOVersion int64, migrateDomain bool) bool {
Expand Down Expand Up @@ -876,7 +876,7 @@ func TestNoChangesInUnmigratedState(t *testing.T) {
},
metrics: metrics.NewNoopMetricsClient().Scope(0),
useNewFailoverVersionOverride: func(domain string) bool { return false },
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
}

err := quick.CheckEqual(func(currVersion int64) int64 {
Expand Down Expand Up @@ -920,7 +920,7 @@ func TestFailoverVersionResolution(t *testing.T) {
},
metrics: metrics.NewNoopMetricsClient().Scope(0),
useNewFailoverVersionOverride: func(domain string) bool { return false },
log: loggerimpl.NewNopLogger(),
log: log.NewNoop(),
}

tests := map[string]struct {
Expand Down
8 changes: 4 additions & 4 deletions common/cluster/metadata_test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ package cluster

import (
"github.com/uber/cadence/common/config"
"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log"
commonMetrics "github.com/uber/cadence/common/metrics"
"github.com/uber/cadence/common/service"
)
Expand Down Expand Up @@ -96,7 +96,7 @@ var (
TestAllClusterInfo,
func(d string) bool { return false },
commonMetrics.NewNoopMetricsClient(),
loggerimpl.NewNopLogger(),
log.NewNoop(),
)

// TestPassiveClusterMetadata is metadata for a passive cluster
Expand All @@ -107,7 +107,7 @@ var (
TestAllClusterInfo,
func(d string) bool { return false },
commonMetrics.NewNoopMetricsClient(),
loggerimpl.NewNopLogger(),
log.NewNoop(),
)
)

Expand All @@ -125,6 +125,6 @@ func GetTestClusterMetadata(isPrimaryCluster bool) Metadata {
TestAllClusterInfo,
func(d string) bool { return false },
commonMetrics.NewNoopMetricsClient(),
loggerimpl.NewNopLogger(),
log.NewNoop(),
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,16 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

//go:generate mockgen -package=$GOPACKAGE -destination=limiterfactory_mock.go github.com/uber/cadence/common/quotas LimiterFactory

package quotas
Copy link
Member

Choose a reason for hiding this comment

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

Is it to avoid a circulair dependency we are moving this to the dynamicconfig package. It feels a bit strange, the ratelimiters does not have anything to do with dynamic config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they use dynamic config and log and introduce circular dependency :(

Copy link
Member

Choose a reason for hiding this comment

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

yeah, well everything uses dynamic config.. but I guess it's very tied to these loggers, so maybe it's ok


import (
"github.com/uber/cadence/common/dynamicconfig"
"github.com/uber/cadence/common/quotas"
)

// LimiterFactory is used to create a Limiter for a given domain
type LimiterFactory interface {
// GetLimiter returns a new Limiter for the given domain
GetLimiter(domain string) Limiter
}

// NewSimpleDynamicRateLimiterFactory creates a new LimiterFactory which creates
// a new DynamicRateLimiter for each domain, the RPS for the DynamicRateLimiter is given by the dynamic config
func NewSimpleDynamicRateLimiterFactory(rps dynamicconfig.IntPropertyFnWithDomainFilter) LimiterFactory {
func NewSimpleDynamicRateLimiterFactory(rps dynamicconfig.IntPropertyFnWithDomainFilter) quotas.LimiterFactory {
return dynamicRateLimiterFactory{
rps: rps,
}
Expand All @@ -47,6 +40,6 @@ type dynamicRateLimiterFactory struct {
}

// GetLimiter returns a new Limiter for the given domain
func (f dynamicRateLimiterFactory) GetLimiter(domain string) Limiter {
return NewDynamicRateLimiter(func() float64 { return float64(f.rps(domain)) })
func (f dynamicRateLimiterFactory) GetLimiter(domain string) quotas.Limiter {
return quotas.NewDynamicRateLimiter(func() float64 { return float64(f.rps(domain)) })
}
43 changes: 43 additions & 0 deletions common/dynamicconfig/quotas/dynamicratelimiterfactory_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// The MIT License (MIT)

// Copyright (c) 2017-2020 Uber Technologies Inc.

// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all
// copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

package quotas

import (
"testing"

"github.com/stretchr/testify/assert"
"golang.org/x/time/rate"
)

func TestNewSimpleDynamicRateLimiterFactory(t *testing.T) {
const _testDomain = "test-domain"

factory := NewSimpleDynamicRateLimiterFactory(func(domain string) int {
assert.Equal(t, _testDomain, domain)
return 100
})

limiter := factory.GetLimiter(_testDomain)

assert.Equal(t, rate.Limit(100), limiter.Limit())
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,18 @@

package quotas

import "github.com/uber/cadence/common/dynamicconfig"
import (
"github.com/uber/cadence/common/dynamicconfig"
"github.com/uber/cadence/common/quotas"
)

// LimiterFactory is used to create a Limiter for a given domain
// NewFallbackDynamicRateLimiterFactory is used to create a Limiter for a given domain
// the created Limiter will use the primary dynamic config if it is set
// otherwise it will use the secondary dynamic config
func NewFallbackDynamicRateLimiterFactory(
primary dynamicconfig.IntPropertyFnWithDomainFilter,
secondary dynamicconfig.IntPropertyFn,
) LimiterFactory {
) quotas.LimiterFactory {
return fallbackDynamicRateLimiterFactory{
primary: primary,
secondary: secondary,
Expand All @@ -44,8 +47,8 @@ type fallbackDynamicRateLimiterFactory struct {
}

// GetLimiter returns a new Limiter for the given domain
func (f fallbackDynamicRateLimiterFactory) GetLimiter(domain string) Limiter {
return NewDynamicRateLimiter(func() float64 {
func (f fallbackDynamicRateLimiterFactory) GetLimiter(domain string) quotas.Limiter {
return quotas.NewDynamicRateLimiter(func() float64 {
return limitWithFallback(
float64(f.primary(domain)),
float64(f.secondary()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
"github.com/uber/cadence/common/cache"
"github.com/uber/cadence/common/dynamicconfig"
"github.com/uber/cadence/common/isolationgroup/isolationgroupapi"
"github.com/uber/cadence/common/log/loggerimpl"
"github.com/uber/cadence/common/log"
"github.com/uber/cadence/common/log/testlogger"
"github.com/uber/cadence/common/metrics"
"github.com/uber/cadence/common/persistence"
Expand Down Expand Up @@ -326,7 +326,7 @@ func TestNewDefaultIsolationGroupStateWatcherWithConfigStoreClient(t *testing.T)
client := metrics.NewNoopMetricsClient()
ig := func() []string { return nil }
NewDefaultIsolationGroupStateWatcherWithConfigStoreClient(
loggerimpl.NewNopLogger(),
log.NewNoop(),
dc,
domainCache,
nil,
Expand Down
Loading