Add rate limiter option to Cassandra writer (WIP) #1147

isaachier · 2018-10-26T20:00:11Z

Which problem is this PR solving?

Resolves Add a rate limiter to jaeger-ingester #1100.

Short description of the changes

Add rate limiter to Cassandra writer so we do not overload Cassandra with write requests.

vprithvi · 2018-10-26T20:23:28Z

Few questions:

What are the advantages of scoping this to Cassandra as opposed to creating a Writer decorator that can be used for all writers?
What are the disadvantages of using https://github.com/uber-go/ratelimit in lieu of writing our own implementation?

isaachier · 2018-10-26T21:15:25Z

What are the advantages of scoping this to Cassandra as opposed to creating a Writer decorator that can be used for all writers?

I had considered that as well. I think it makes sense. The only issue is that config tends to be database specific, so the main change will probably still be in the Cassandra config (unless you'd like me to add to all database implementations immediately).

What are the disadvantages of using https://github.com/uber-go/ratelimit in lieu of writing our own implementation?

I'd love to use uber-go/ratelimit, but they only support integer rates, not floating point rates (not sure why). If I can get a PR through to them, that would be the best solution. Then again, it is probably a breaking change for their API.

vprithvi · 2018-10-26T21:38:04Z

I'd love to use uber-go/ratelimit, but they only support integer rates, not floating point rates

What makes floating point rates indispensable for us?

yurishkuro · 2018-10-27T04:38:13Z

I don't envision us having rate per ingester of less than one msg per second, so integer rates seem fine

isaachier · 2018-10-27T23:01:10Z

OK cool. I can remove the rate limiter too.

isaachier · 2018-10-27T23:59:17Z

Actually, ratelimit is broken. Here is the error:

vendor/github.com/uber-go/ratelimit/ratelimit.go:27:2: use of internal package github.com/jaegertracing/jaeger/vendor/go.uber.org/ratelimit/internal/clock not allowed

Looks like the package is not really maintained based on the history and lack of versions.

codecov · 2018-10-28T02:30:52Z

Codecov Report

Merging #1147 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1147   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         159     160    +1     
  Lines        7145    7162   +17     
======================================
+ Hits         7145    7162   +17

Impacted Files	Coverage Δ
storage/spanstore/ratelimit/rate_limit.go	`100% <100%> (ø)`
plugin/storage/cassandra/factory.go	`100% <100%> (ø)`	⬆️
plugin/storage/cassandra/options.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b18bd9...2f11922. Read the comment docs.

isaachier · 2018-10-29T12:42:25Z

Can someone please rerun test? ES unit test failed.

black-adder · 2018-10-29T14:16:47Z

ratelimit is broken

I find that hard to believe given that it's being used by multiple other teams at Uber. Looks like the issue is more a vendoring issue.

isaachier · 2018-10-29T14:24:40Z

ratelimit is broken

I find that hard to believe given that it's being used by multiple other teams at Uber. Looks like the issue is more a vendoring issue.

Interesting thought. I'm surprised it works. Seems to use internal package types in public interface. I'll try again and let you know.

black-adder · 2018-10-29T14:37:22Z

storage/spanstore/decorator/rate_limit.go

+// rate limiter before each write.
+func (r *rateLimitedWriter) WriteSpan(s *model.Span) error {
+	const cost = 1
+	for ok, waitTime := r.l.CheckCredit(cost); !ok; ok, waitTime = r.l.CheckCredit(cost) {


Won't

for ok, waitTime := r.l.CheckCredit(cost); !ok { time.Sleep(waitTime)

work? Not sure why the second call is needed

Well let's imagine Java/C++/wtvr other language:

for (bool ok = r.l.CheckCredit(cost); !ok; ok = r.l.CheckCredit(cost)) { // ... }

First statement is a declaration/initialization statement. Last statement repeats each time the loop is repeated. Not 100% sure, but I imagine Go works the same way.

Ya, this explains it: https://tour.golang.org/flowcontrol/1.

black-adder · 2018-10-29T14:51:44Z

plugin/storage/cassandra/options.go

@@ -48,6 +48,8 @@ const (

 	// common storage settings
 	suffixSpanStoreWriteCacheTTL = ".span-store-write-cache-ttl"
+	suffixWritesPerSecond        = ".writer-rate-limit.operations-per-second"


I'm ok with this for now but I think these configs should be storage type agnostic (ie ES can also use the rate limiter but we shouldn't have to pollute ES storage configs with .writer-rate-limit.operations-per-second and .writer-rate-limit.max-burst)

I wish the configs generally were less storage specific. I'm not sure if we share any config at the moment.

vprithvi · 2018-10-29T15:34:16Z

plugin/storage/cassandra/options.go

+// rateLimitConfig defines common arguments to a rate limiter.
+type rateLimitConfig struct {
+	operationsPerSecond float64
+	maxBurst            float64


Is this config required?

Meant to delete this. Thanks for the reminder.

plugin/storage/cassandra/factory.go

vprithvi · 2018-10-29T19:26:28Z

storage/spanstore/decorator/rate_limit_test.go

+	require.Nil(t, decoratedWriter)
+}
+
+func TestRateLimitedWriterUnlimitedWritesPerSecond(t *testing.T) {


I feel that this test doesn't test anything additional over TestRateLimitedWriter. Could this be made better?
(If not, it may make sense to implement a no-op Limiter inside our decorator so that we can test it better)

Ya. I didn't want to use time alone seeing how flakey that can be. Will try mocking soon.

Not a huge fan of type testing, but tried it out in latest revision.

yurishkuro · 2018-10-30T16:09:31Z

plugin/storage/cassandra/factory.go

@@ -28,6 +28,7 @@ import (
 	"github.com/jaegertracing/jaeger/storage"
 	"github.com/jaegertracing/jaeger/storage/dependencystore"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
+	"github.com/jaegertracing/jaeger/storage/spanstore/decorator"


why the "decorator" package? Metrics is also a decorator, it's in its own package. I suggest spanstore/ratelimited instead

yurishkuro · 2018-10-30T16:10:22Z

plugin/storage/cassandra/factory.go

+
+	return decorator.NewRateLimitedWriter(
+		writer,
+		f.Options.writesPerSecond,


if f.Options.writesPerSecond == 0, do we need to wrap?

Lol see what @vprithvi suggested earlier. We are wrapping with noop rate limiter in that case.

if we have an if statement, I'd rather have it here and not add two more layers of indirection. Then in your rate limited writer, return an error if rate < 1.

Oh man. I want to wait on @vprithvi's response because this is literally what I was wondering above.

Good idea. I wasn't sure if a passthrough would hurt performance due to the indirection.

#1147 (comment)

I have a preference towards having this logic be in the rate limiter because it means that it doesn't need to be in every storage implementation. However, it's not a strong preference.

That's a separate concern though, the config and application of rate limiter is currently in the cassandra factory, but we could easily pull it up to the higher level (meta) factory (I just don't think we need to do it right now). This keeps the rate limiter simpler - you can only instantiate it with a positive rate.

I'm wondering when we care about performance of indirection, etc. But I'll go back to the original if statement here.

yurishkuro · 2018-10-30T16:14:01Z

storage/spanstore/decorator/rate_limit.go

+
+type rateLimitedWriter struct {
+	w spanstore.Writer
+	l ratelimit.Limiter


Please use full field names. One letter names are ok for local variables when there is context.

isaachier · 2018-10-30T19:53:48Z

ES integration test again!

jpkrohling · 2018-10-31T10:37:43Z

Job restarted

plugin/storage/cassandra/factory_test.go

yurishkuro · 2018-10-31T16:19:43Z

plugin/storage/cassandra/options.go

@@ -111,6 +113,9 @@ func (opt *Options) AddFlags(flagSet *flag.FlagSet) {
 	flagSet.Duration(opt.primary.namespace+suffixSpanStoreWriteCacheTTL,
 		opt.SpanStoreWriteCacheTTL,
 		"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.

That's better.

yurishkuro · 2018-10-31T16:22:03Z

plugin/storage/cassandra/options.go

@@ -57,6 +58,7 @@ type Options struct {
 	primary                *namespaceConfig
 	others                 map[string]*namespaceConfig
 	SpanStoreWriteCacheTTL time.Duration
+	writesPerSecond        int


Shouldn't this be public? And qualified with SpanStore prefix.

IDK why SpanStoreWriteCacheTTL is public either. There is no serialization here. I think it might be a workaround to pass options to another package that need to read SpanStoreWriteCacheTTL, but doesn't make much sense given that passing the int is just as effective.

how are we using this internally? SpanStoreWriteCacheTTL is externally settable option.

Both of these are initialized by command-line flags, so neither should be public. I did a code search and could not find a reference to SpanStoreWriteCacheTTL (not even in our internal Uber repo). Seems to be vestigial from some point where we needed access from our internal repo into this part of the OSS.

We don't use cli flags internally. If we need to set these two fields how would we do it without them being public? Did you try your branch without our internal main?

$ git grep -l SpanStoreWriteCacheTTL plugin/storage/cassandra/factory.go plugin/storage/cassandra/options.go

yurishkuro · 2018-10-31T16:23:10Z

plugin/storage/cassandra/options_test.go

@@ -82,4 +83,6 @@ 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)


move to L73

yurishkuro · 2018-10-31T16:24:33Z

storage/spanstore/ratelimit/rate_limit.go

+	"github.com/jaegertracing/jaeger/storage/spanstore"
+)
+
+var errInvalidRate = errors.New("rate must be a non-zero positive integer")


positive already implies non-zero

yurishkuro

couldn't we do the metrics in a separate PR? This is a scope creep.

yurishkuro · 2018-10-31T19:19:40Z

storage/spanstore/ratelimit/rate_limit.go

 	if rate <= 0 {
 		return nil, errInvalidRate
 	}

+	m := metricsFactory.Namespace("rate-limited-writer", nil)


is using dashes in metric names consistent with the rest of the code?

I copied the format from the storage/spanstore/metrics/writer_metrics.go file, but I cannot guarantee that file is correct.

yurishkuro · 2018-10-31T19:23:37Z

storage/spanstore/ratelimit/rate_limit.go

 // 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 {
-	r.limiter.Take()
+	const threshold = 100 * time.Microsecond


this looks like a complete hack / arbitrary threshold. We should already have a write latency metric from the main writer. You can add another timer here that would measure full latency (wait + actual write) and emit that.

As for the counter, unless uber-go limiter returns a value indicating whether there was any sleep, I don't think we should be trying to reverse-engineer it with some arbitrary thredsholds.

I happen to agree wholeheartedly. Team suggested I add a metric with number of delayed spans. The current rate limiter API makes this essentially impossible. IMO the real issue is that the rate limiter API has limited use in its current form. I believe it would have made more sense to return the duration slept as opposed to the timestamp of the wake time.

isaachier · 2018-10-31T20:03:30Z

I just added metrics here while testing. I understand the concern regarding scope creep, and will move it to another PR soon.

Signed-off-by: Isaac Hier <[email protected]>

yurishkuro · 2019-04-06T14:34:45Z

I think this is solved differently by #1353

isaachier requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners October 26, 2018 20:00

isaachier force-pushed the writer-rate-limit branch from 8dcca83 to 879b240 Compare October 26, 2018 20:01

black-adder reviewed Oct 29, 2018

View reviewed changes

vprithvi reviewed Oct 29, 2018

View reviewed changes

isaachier changed the title ~~Add rate limiter option to Cassandra writer (WIP)~~ Add rate limiter option to Cassandra writer Oct 29, 2018

vprithvi reviewed Oct 29, 2018

View reviewed changes

plugin/storage/cassandra/factory.go Show resolved Hide resolved

vprithvi reviewed Oct 29, 2018

View reviewed changes

vprithvi approved these changes Oct 30, 2018

View reviewed changes

isaachier changed the title ~~Add rate limiter option to Cassandra writer~~ Add rate limiter option to Cassandra writer (WIP) Oct 30, 2018

yurishkuro reviewed Oct 30, 2018

View reviewed changes

isaachier requested a review from tiffon as a code owner October 30, 2018 19:25

yurishkuro reviewed Oct 31, 2018

View reviewed changes

isaachier requested a review from objectiser as a code owner October 31, 2018 18:39

yurishkuro reviewed Oct 31, 2018

View reviewed changes

isaachier force-pushed the writer-rate-limit branch from ade3fde to 24aea8b Compare November 22, 2018 00:25

isaachier force-pushed the writer-rate-limit branch from 24aea8b to 891eef8 Compare December 5, 2018 19:07

Isaac Hier added 3 commits December 7, 2018 08:18

Implement rate limiting for storage writers

7ea7931

Signed-off-by: Isaac Hier <[email protected]>

Use newer protobuf (v1.1.1)

8db0f63

Signed-off-by: Isaac Hier <[email protected]>

Do not specify version for gogo/protobuf

2f11922

Signed-off-by: Isaac Hier <[email protected]>

isaachier force-pushed the writer-rate-limit branch from 0b60e6f to 2f11922 Compare December 7, 2018 13:19

yurishkuro closed this Apr 6, 2019

Add rate limiter option to Cassandra writer (WIP) #1147

Add rate limiter option to Cassandra writer (WIP) #1147

Conversation

isaachier commented Oct 26, 2018

Which problem is this PR solving?

Short description of the changes

vprithvi commented Oct 26, 2018 • edited Loading

isaachier commented Oct 26, 2018

vprithvi commented Oct 26, 2018

yurishkuro commented Oct 27, 2018

isaachier commented Oct 27, 2018

isaachier commented Oct 27, 2018

codecov bot commented Oct 28, 2018 • edited Loading

Codecov Report

isaachier commented Oct 29, 2018 • edited Loading

black-adder commented Oct 29, 2018

isaachier commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vprithvi Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Oct 30, 2018

jpkrohling commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Oct 31, 2018

yurishkuro commented Apr 6, 2019

vprithvi commented Oct 26, 2018 •

edited

Loading

codecov bot commented Oct 28, 2018 •

edited

Loading

isaachier commented Oct 29, 2018 •

edited

Loading

vprithvi Oct 29, 2018 •

edited

Loading

isaachier Oct 30, 2018 •

edited

Loading