Implement Throttler #253

black-adder · 2018-01-17T18:01:54Z

Adding a throttler that will allow clients to rate limit the amount of debug spans that they can generate.

Tests
Agree on APIs

Signed-off-by: Won Jun Jang wjang@uber.com

codecov · 2018-01-17T18:07:31Z

Codecov Report

Merging #253 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master     #253    +/-   ##
========================================
+ Coverage   84.95%   85.96%    +1%     
========================================
  Files          51       54     +3     
  Lines        2705     2899   +194     
========================================
+ Hits         2298     2492   +194     
- Misses        287      288     +1     
+ Partials      120      119     -1

Impacted Files	Coverage Δ
tracer.go	`88.57% <100%> (+0.57%)`	⬆️
internal/throttler/remote/throttler.go	`100% <100%> (ø)`
log/logger.go	`100% <100%> (ø)`	⬆️
tracer_options.go	`74.54% <100%> (+1.46%)`	⬆️
internal/throttler/throttler.go	`100% <100%> (ø)`
internal/throttler/remote/options.go	`100% <100%> (ø)`
config/options.go	`100% <0%> (ø)`	⬆️
... and 1 more

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 f6dd779...e8cb677. Read the comment docs.

black-adder

An alternative approach would be to lazily set the UUID in the throttler as done here: https://github.com/jaegertracing/jaeger-client-go/blob/master/transport_udp.go#L85

However, this would mean the API would have to change to IsThrottled(operation, uuid) which could potentially save us a lot of headache. However, this isn't compatible with the Sampler API although I could duck type the sampler to remove the circular dependency issues and then we could just initialize everything in the tracer on initialization. I'll play around with this some more

black-adder · 2018-01-17T18:03:24Z

internal/throttler/sampler/throttled_sampler.go

+	"github.com/uber/jaeger-client-go/internal/throttler"
+)
+
+// N.B. this class is private as it's a proof of concept of how we would throttle normal


As mentioned in the comment, this is just a proof of concept of how we'd throttle normal sampler traffic. (I'll remove before landing).

black-adder · 2018-01-17T18:05:33Z

internal/throttler/sampler/throttled_sampler.go

+	s.throttler.SetUUID(uuid)
+}
+
+func (s *throttledSampler) IsSampled(id jaeger.TraceID, operation string) (bool, []jaeger.Tag) {


Some brainstorming is required before we can actually do this. If sampled == true and isThrottled() == true, we need to emit a hint somewhere (meta-span?) so that XYS is aware that a sampled span was dropped.

black-adder · 2018-01-17T18:11:10Z

internal/throttler/sampler/throttled_sampler.go

+	}
+}
+
+func (s *throttledSampler) SetUUID(uuid string) {


If we want to use the existing Sampler API, we would have to update it with this function since the sampler will be wrapped by the throttler in the config directory but the UUID setting occurs in the tracer which means it's too late to initialize this wrapped sampler in the tracer (and it'd cause cycling import errors).

duck-typing sounds fine to me. However, I would combine this UUID with service name + process tags (i.e. have an internal Process struct) and have the same SetProcess duck-typed interface shared by reporters and samplers.

black-adder · 2018-01-17T18:56:20Z

Ok, I got it where we can use the throttler as a sampler without needing to update the sampler API. Only downside is that we're possibly increasing the surface area of our root Jaeger API. I actually think it makes sense to keep the throttledSampler API private.

isaachier

Like the implementation overall. Mostly nits.

isaachier · 2018-01-17T21:29:36Z

internal/throttler/remote/throttler.go

+	t.uuid.Store(uuid)
+}
+
+// N.B. This function should be called with the Write Lock


I think you should use a different name if it changes the credits map, something like deductCredits makes more sense.

should/must - https://www.ietf.org/rfc/rfc2119.txt

isaachier · 2018-01-17T21:30:27Z

internal/throttler/remote/throttler.go

+func (t *Throttler) pollManager() {
+	defer t.stopped.Done()
+	ticker := time.NewTicker(t.refreshInterval)
+	defer ticker.Stop()


I believe stopping the ticker is unnecessary in a case like this.

why is that?

Because it should be garbage collected as a local variable with no references to its channel. Unless this results in a leaked goroutine, which is confusing. For the record, it seems you can leak goroutines but not channels.

isaachier · 2018-01-17T21:31:09Z

internal/throttler/remote/throttler.go

+
+func (t *Throttler) fetchCredits() {
+	t.mux.RLock()
+	// TODO This is probably inefficient, maybe create a static slice of operations?


This may not be as bad as you think. Used to do transformations like this all the time in C++ thanks to std::transform. Even when we hit memory issues in our C++ code, std::transform wasn't the main issue, despite allocating a temporary vector each time like this code does. IMO, allocation issues aren't really a problem for a small collection of temporaries (< 100 elements) as much as long living, tree-like recursive structures or huge blobs.

allocations are a problem when they are on the hot path of the business logic of the application. In the periodic background activities - not so much.

isaachier · 2018-01-17T21:42:06Z

internal/throttler/remote/throttler.go

+type Throttler struct {
+	options
+
+	mux           sync.RWMutex


For the record, I used to love the idea of reader-writer locks in all code, then learned that it might actually perform worse than a regular mutex if the contention isn't high enough. RW locks generally have a bit more overhead because the logic to avoid writer/reader starvation is more complex. Might be worth considering and/or profiling.

isaachier · 2018-01-17T21:42:33Z

internal/throttler/remote/throttler.go

+
+	mux           sync.RWMutex
+	service       string
+	uuid          *atomic.String


Why bother with an atomic string instead of using a regular constructor here?

isaachier · 2018-01-17T21:47:11Z

tracer_options.go

+	return func(tracer *Tracer) {
+		tracer.throttler = throttler
+	}
+}


Why are we doing two throttlers upfront? I thought we would use one throttler and just allow the users to define the type of throttling to do.

The options are:

Two throttlers options

One throttler option, and a enum option of: "DEBUG", "SAMPLING", "BOTH"

I don't mind either one since both will work but I just chose the former

I thought we haven't even agreed that throttling regular sampling is not going to mess up xys.

tis a proof of concept, I wanted to make sure we could throttle the sampler without being too invasive. I'll be removing this code.

Cool. I was thinking more like the option based approach if we need it.

isaachier · 2018-01-17T21:51:49Z

internal/throttler/remote/throttler.go

+	return false
+}
+
+func (t *Throttler) pollManager() {


Not sure if polling is truly necessary. On the other hand, I believe it should be required to spawn an asynchronous credit request when an operation runs out of credits (i.e. below 1.0). Otherwise, we run the risk of making requests when we don't need credits and not requesting when we do need credits.

if we do this on demand, and the demand happens to be very high, there will be a lot of these async requests, each only returning a small fraction of accumulated credits from the agent.

also, the agent uses these requests to update the client's updateTime. If we don't ping regularly, we could potentially have the agent give the client way more credits than its due.

Ya I was wondering if we could combine the two somehow. I guess let's try this and see if it works well.

yurishkuro · 2018-01-18T04:29:11Z

internal/throttler/remote/throttler.go

+	"sync"
+	"time"
+
+	"github.com/uber-go/atomic"


uber-go/atomic is not an existing dependency (it may be transitive of tchannel, which is not used in production), so let's not introduce it

yurishkuro · 2018-01-18T04:32:28Z

internal/throttler/remote/throttler.go

+	_, ok := t.credits[operation]
+	if !ok {
+		// If it is the first time this operation is being checked, synchronously fetch
+		// the credits.


synchronously fetch

can we make this configurable? Only yab needs it synchronously, normal services could just wait for refresh?

yurishkuro · 2018-01-18T04:33:53Z

internal/throttler/remote/throttler.go

+
+func (t *Throttler) fetchCredits() {
+	t.mux.RLock()
+	// TODO This is probably inefficient, maybe create a static slice of operations?


allocations are a problem when they are on the hot path of the business logic of the application. In the periodic background activities - not so much.

yurishkuro · 2018-01-18T04:36:15Z

internal/throttler/remote/throttler.go

+	}
+}
+
+func (t *Throttler) fetchCredits() {


refreshCredits

yurishkuro · 2018-01-18T04:36:30Z

internal/throttler/remote/throttler.go

+	}
+}
+
+func (t *Throttler) fetchCreditsHelper(operations []string) []creditResponse {


fetchCredits

yurishkuro · 2018-01-18T04:38:16Z

internal/throttler/remote/throttler.go

+	t.uuid.Store(uuid)
+}
+
+// N.B. This function should be called with the Write Lock


should/must - https://www.ietf.org/rfc/rfc2119.txt

yurishkuro · 2018-01-18T04:41:53Z

internal/throttler/remote/throttler.go

+	return false
+}
+
+func (t *Throttler) pollManager() {


if we do this on demand, and the demand happens to be very high, there will be a lot of these async requests, each only returning a small fraction of accumulated credits from the agent.

yurishkuro · 2018-01-18T04:43:43Z

internal/throttler/remote/throttler.go

+func (t *Throttler) pollManager() {
+	defer t.stopped.Done()
+	ticker := time.NewTicker(t.refreshInterval)
+	defer ticker.Stop()


why is that?

yurishkuro · 2018-01-18T04:52:21Z

internal/throttler/throttler.go

+	io.Closer
+
+	// IsThrottled determines whether the operation should be throttled.
+	IsThrottled(operation string) bool


Somehow I get a feeling that this is a double negative. Wouldn't "is allowed" be better? If you look at various rate limiter implementations, their API are typically expressed in the positive outcomes, "is allowed", "how long till it's allowed".

yurishkuro · 2018-01-18T04:53:13Z

internal/throttler/throttler.go

+// are always sampled, a throttler can be enabled per client to rate limit the amount
+// of debug spans a client can start.
+type Throttler interface {
+	io.Closer


it might be better to duck type this

may I ask why?

Good question. I don't really get when to do one or the other.

yurishkuro · 2018-01-18T04:53:39Z

internal/throttler/throttler.go

+	IsThrottled(operation string) bool
+
+	// SetUUID sets the UUID that identifies this client instance.
+	SetUUID(uuid string)


did you see my comment this morning about unifying this with SetProcess across the board?

black-adder · 2018-01-18T20:52:33Z

process.go

+// ProcessSetter sets a process. This can be used by any class that requires
+// the process to be set as part of initialization.
+// See internal/throttler/remote/throttler.go for an example.
+type ProcessSetter interface {


@yurishkuro is this what you had in mind?

yurishkuro · 2018-01-18T22:23:43Z

internal/throttler/remote/throttler.go

+	mux           sync.RWMutex
+	service       string
+	uuid          string
+	uuidSet       uint32 // 1 means uuid set, 0 means not set


Why not just use atomic.value for the string?

isaachier · 2018-01-31T19:30:51Z

LGTM, but you might wants to wait on @yurishkuro.

Signed-off-by: Won Jun Jang <wjang@uber.com>

vprithvi · 2018-02-12T21:02:40Z

internal/throttler/remote/options.go

+
+// SynchronousInitialization creates an Option that determines whether the throttler should synchronously
+// fetch credits from the agent when an operation is seen for the first time.
+func (options) SynchronousInitialization(b bool) Option {


Could you add some details on when it's a good idea to set this option to the docstring?

vprithvi · 2018-02-12T21:05:09Z

internal/throttler/remote/options_test.go

+	assert.Equal(t, "localhost:5778", options.hostPort)
+	assert.Equal(t, time.Second*5, options.refreshInterval)
+	assert.NotNil(t, options.metrics)
+	assert.NotNil(t, options.logger)


Why don't we test the default SynchronousInitialization?

vprithvi · 2018-02-12T21:11:58Z

process.go

+// Process holds process specific metadata that's relevant to this client.
+type Process struct {
+	Service string
+	UUID    string


Why is this called UUID?
We only seem to be using it as an arbitrary identifier.

At one point we actually did use a UUID. Not sure what happened.

I don't think we should continue calling this field UUID if it's not going to be holding a UUID.

It will be a UUID, I just have to combine this with https://github.com/jaegertracing/jaeger-client-go/pull/248/files

vprithvi · 2018-02-12T21:22:37Z

internal/throttler/remote/throttler.go

+	}
+}
+
+func (t *Throttler) fetchCredits(operations []string) []creditResponse {


Why not return a ([]creditResponse, error) ?

Signed-off-by: Won Jun Jang <wjang@uber.com>

black-adder commented Jan 17, 2018

View reviewed changes

black-adder changed the title ~~[WIP] Implement Throttler~~ Implement Throttler Jan 17, 2018

isaachier reviewed Jan 17, 2018

View reviewed changes

yurishkuro reviewed Jan 18, 2018

View reviewed changes

black-adder commented Jan 18, 2018

View reviewed changes

yurishkuro reviewed Jan 18, 2018

View reviewed changes

black-adder added 8 commits February 12, 2018 15:48

Add throttler

9da8c6a

Signed-off-by: Won Jun Jang <wjang@uber.com>

no need for duck typing

54c5e6c

Signed-off-by: Won Jun Jang <wjang@uber.com>

remove unecessary function

33a6089

Signed-off-by: Won Jun Jang <wjang@uber.com>

address comments

341b4c1

Signed-off-by: Won Jun Jang <wjang@uber.com>

remove unused import

34d4979

Signed-off-by: Won Jun Jang <wjang@uber.com>

nits?

de1a1eb

Signed-off-by: Won Jun Jang <wjang@uber.com>

update tests

611b52d

Signed-off-by: Won Jun Jang <wjang@uber.com>

cover all the things

c088647

Signed-off-by: Won Jun Jang <wjang@uber.com>

black-adder force-pushed the throttler_prototype branch from f0565f2 to c088647 Compare February 12, 2018 20:48

vprithvi reviewed Feb 12, 2018

View reviewed changes

black-adder added 2 commits February 13, 2018 11:04

address

ab92b3c

Signed-off-by: Won Jun Jang <wjang@uber.com>

100% coverage

e8cb677

Signed-off-by: Won Jun Jang <wjang@uber.com>

vprithvi approved these changes Feb 14, 2018

View reviewed changes

black-adder merged commit d7f08d5 into master Feb 14, 2018

black-adder deleted the throttler_prototype branch February 14, 2018 15:23

isaachier pushed a commit to isaachier/jaeger-client-go that referenced this pull request Mar 26, 2018

Add Throttler and ProcessSetter (jaegertracing#253)

92bfd82

isaachier pushed a commit to isaachier/jaeger-client-go that referenced this pull request Mar 26, 2018

Add Throttler and ProcessSetter (jaegertracing#253)

2eb121b

isaachier pushed a commit to isaachier/jaeger-client-go that referenced this pull request Apr 16, 2018

Add Throttler and ProcessSetter (jaegertracing#253)

fa47ae3

Implement Throttler #253

Implement Throttler #253

Conversation

black-adder commented Jan 17, 2018 • edited

codecov bot commented Jan 17, 2018 • edited

Codecov Report

black-adder 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

black-adder commented Jan 17, 2018

isaachier 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

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

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

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

isaachier commented Jan 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

black-adder commented Jan 17, 2018 •

edited

codecov bot commented Jan 17, 2018 •

edited