From 24aea8b7adb1e3b8ec2c5e9348930fc153163af8 Mon Sep 17 00:00:00 2001 From: Isaac Hier Date: Thu, 1 Nov 2018 10:59:55 -0400 Subject: [PATCH] Remove metrics, minor improvements Signed-off-by: Isaac Hier --- plugin/storage/cassandra/factory.go | 2 +- plugin/storage/cassandra/factory_test.go | 5 +++- plugin/storage/cassandra/options.go | 2 +- plugin/storage/cassandra/options_test.go | 3 +- storage/spanstore/ratelimit/rate_limit.go | 30 ++++--------------- .../spanstore/ratelimit/rate_limit_test.go | 13 ++------ 6 files changed, 16 insertions(+), 39 deletions(-) diff --git a/plugin/storage/cassandra/factory.go b/plugin/storage/cassandra/factory.go index 44543b2c6fa..10edff9cb76 100644 --- a/plugin/storage/cassandra/factory.go +++ b/plugin/storage/cassandra/factory.go @@ -108,7 +108,7 @@ func (f *Factory) CreateSpanWriter() (spanstore.Writer, error) { return writer, nil } - return ratelimit.NewRateLimitedWriter(writer, f.Options.writesPerSecond, f.primaryMetricsFactory) + return ratelimit.NewRateLimitedWriter(writer, f.Options.writesPerSecond) } // CreateDependencyReader implements storage.Factory diff --git a/plugin/storage/cassandra/factory_test.go b/plugin/storage/cassandra/factory_test.go index 435304b6560..c45ea54ee48 100644 --- a/plugin/storage/cassandra/factory_test.go +++ b/plugin/storage/cassandra/factory_test.go @@ -27,6 +27,7 @@ import ( "github.com/jaegertracing/jaeger/pkg/config" "github.com/jaegertracing/jaeger/pkg/testutils" "github.com/jaegertracing/jaeger/storage" + "github.com/jaegertracing/jaeger/storage/spanstore/ratelimit" ) var _ storage.Factory = new(Factory) @@ -102,6 +103,8 @@ func TestCassandraFactoryWriterRateLimit(t *testing.T) { f.archiveConfig = nil assert.NoError(t, f.Initialize(metrics.NullFactory, logger)) - _, err := f.CreateSpanWriter() + writer, err := f.CreateSpanWriter() assert.NoError(t, err) + dummyRateLimitedWriter, _ := ratelimit.NewRateLimitedWriter(nil, 1) + assert.IsType(t, dummyRateLimitedWriter, writer) } diff --git a/plugin/storage/cassandra/options.go b/plugin/storage/cassandra/options.go index 7960af9d74b..e7e033e286f 100644 --- a/plugin/storage/cassandra/options.go +++ b/plugin/storage/cassandra/options.go @@ -115,7 +115,7 @@ func (opt *Options) AddFlags(flagSet *flag.FlagSet) { "The duration to wait before rewriting an existing service or operation name") flagSet.Int(opt.primary.namespace+suffixWritesPerSecond, opt.writesPerSecond, - "The number of writes per second using rate limiter") + "Optional upper limit on the number of span writes per second") } func addFlags(flagSet *flag.FlagSet, nsConfig *namespaceConfig) { diff --git a/plugin/storage/cassandra/options_test.go b/plugin/storage/cassandra/options_test.go index e21bfe87037..852a55d109c 100644 --- a/plugin/storage/cassandra/options_test.go +++ b/plugin/storage/cassandra/options_test.go @@ -70,6 +70,7 @@ func TestOptionsWithFlags(t *testing.T) { assert.Equal(t, "jaeger", primary.Keyspace) assert.Equal(t, []string{"1.1.1.1", "2.2.2.2"}, primary.Servers) assert.Equal(t, "ONE", primary.Consistency) + assert.Equal(t, 10, opts.writesPerSecond) aux := opts.Get("cas-aux") require.NotNil(t, aux) @@ -83,6 +84,4 @@ func TestOptionsWithFlags(t *testing.T) { assert.Equal(t, "", aux.Consistency, "aux storage does not inherit consistency from primary") assert.Equal(t, 3, aux.ProtoVersion) assert.Equal(t, 42*time.Second, aux.SocketKeepAlive) - - assert.Equal(t, 10, opts.writesPerSecond) } diff --git a/storage/spanstore/ratelimit/rate_limit.go b/storage/spanstore/ratelimit/rate_limit.go index 0566f5c2d4c..c8abde4f2be 100644 --- a/storage/spanstore/ratelimit/rate_limit.go +++ b/storage/spanstore/ratelimit/rate_limit.go @@ -16,9 +16,7 @@ package ratelimit import ( "errors" - "time" - "github.com/uber/jaeger-lib/metrics" rlImpl "go.uber.org/ratelimit" "github.com/jaegertracing/jaeger/model" @@ -28,42 +26,26 @@ import ( var errInvalidRate = errors.New("rate must be a positive integer") type rateLimitedWriter struct { - writer spanstore.Writer - limiter rlImpl.Limiter - sleepTime metrics.Timer - delayedWrites metrics.Counter + writer spanstore.Writer + limiter rlImpl.Limiter } // NewRateLimitedWriter decorates a Writer with a rate limiter in order to limit // the number of writes per second. -func NewRateLimitedWriter( - writer spanstore.Writer, - rate int, - metricsFactory metrics.Factory, -) (spanstore.Writer, error) { +func NewRateLimitedWriter(writer spanstore.Writer, rate int) (spanstore.Writer, error) { if rate <= 0 { return nil, errInvalidRate } - m := metricsFactory.Namespace("rate-limited-writer", nil) return &rateLimitedWriter{ - writer: writer, - limiter: rlImpl.New(rate), - sleepTime: m.Timer("sleep-time", nil), - delayedWrites: m.Counter("delayed-writes", nil), + writer: writer, + limiter: rlImpl.New(rate), }, nil } // WriteSpan wraps the write span method of the inner writer, but invokes the // rate limiter before each write. func (r *rateLimitedWriter) WriteSpan(s *model.Span) error { - const threshold = 100 * time.Microsecond - startTime := time.Now() - endTime := r.limiter.Take() - duration := endTime.Sub(startTime) - if duration >= threshold { - r.delayedWrites.Inc(1) - r.sleepTime.Record(duration) - } + r.limiter.Take() return r.writer.WriteSpan(s) } diff --git a/storage/spanstore/ratelimit/rate_limit_test.go b/storage/spanstore/ratelimit/rate_limit_test.go index 043d2d9dc33..aec8b37db0e 100644 --- a/storage/spanstore/ratelimit/rate_limit_test.go +++ b/storage/spanstore/ratelimit/rate_limit_test.go @@ -20,7 +20,6 @@ import ( "time" "github.com/stretchr/testify/require" - "github.com/uber/jaeger-lib/metrics" "github.com/jaegertracing/jaeger/model" ) @@ -44,8 +43,7 @@ func (m *mockRateLimiter) Take() time.Time { func TestRateLimitedWriter(t *testing.T) { writer := &mockWriter{} - metricsFactory := metrics.NewLocalFactory(0) - decoratedWriter, err := NewRateLimitedWriter(writer, 10, metricsFactory) + decoratedWriter, err := NewRateLimitedWriter(writer, 10) require.NoError(t, err) var rateLimiter mockRateLimiter decoratedWriter.(*rateLimitedWriter).limiter = &rateLimiter @@ -57,8 +55,7 @@ func TestRateLimitedWriter(t *testing.T) { func TestRateLimitedWriterInvalidWritesPerSecond(t *testing.T) { writer := &mockWriter{} - metricsFactory := metrics.NewLocalFactory(0) - decoratedWriter, err := NewRateLimitedWriter(writer, 0, metricsFactory) + decoratedWriter, err := NewRateLimitedWriter(writer, 0) require.Error(t, err) require.Nil(t, decoratedWriter) } @@ -68,8 +65,7 @@ func TestRateLimitedWriterWithWriteError(t *testing.T) { writer := &mockWriter{ expectedError: fakeError, } - metricsFactory := metrics.NewLocalFactory(0) - decoratedWriter, err := NewRateLimitedWriter(writer, 5, metricsFactory) + decoratedWriter, err := NewRateLimitedWriter(writer, 5) require.NoError(t, err) err = decoratedWriter.WriteSpan(nil) require.Error(t, err) @@ -77,7 +73,4 @@ func TestRateLimitedWriterWithWriteError(t *testing.T) { writer.expectedError = nil err = decoratedWriter.WriteSpan(nil) require.NoError(t, err) - counters, _ := metricsFactory.Snapshot() - require.Contains(t, counters, "rate-limited-writer.delayed-writes") - require.Equal(t, int64(1), counters["rate-limited-writer.delayed-writes"]) }