Add throttler #246

black-adder · 2018-04-02T20:41:11Z

Throttler is a rate limiter that frequently fetches credits for each operation and rate limits said operation if it has insufficient credits. This PR only adds the throttler, the actual rate limiting will be done in a subsequent PR.

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

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

black-adder · 2018-04-02T20:49:13Z

src/throttler/remote_thottler.js

+    if (!this._credits.has(operation)) {
+      this._credits.set(operation, 0);
+      // If seen for the first time, async fetch credits
+      this._refreshCredits();


In golang, when we see the operation for the first time, we have the option of allowing service owners to synchronously fetch credits for the first call; this is useful for cases like yab. In node however, I spoke with @tiffon and he said it's not advisable (TBF, it's not advisable in golang either but I think necessary).

I think this is an ok compromise.

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

black-adder · 2018-04-02T21:49:46Z

src/_flow/jaeger-thrift.js

@@ -24,6 +24,9 @@ declare type LogData = {
 declare type Process = {
  serviceName: string,
  tags: Array<Tag>,
+  // N.B. uuid uniquely identifies this instance of the client. It is only used by the client and is not
+  // propagated as part of the real jaeger thrift process.
+  uuid?: string,


I made this optional to not break all the things. Should not be a breaking change.

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

black-adder · 2018-04-03T15:49:29Z

package.json

    "coveralls": "npm run cover && cat ./coverage/lcov.info | coveralls",
    "flow": "flow",
-    "format": "prettier --write '**/*.{js,json,md}'",
+    "format": "prettier --write '**/*.{js,json,md}' '!src/jaeger-idl/**'",


added a rule to skip prettifying a git submodule

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

coveralls · 2018-04-03T17:00:31Z

Coverage increased (+0.08%) to 96.911% when pulling 00df710 on add_throttler into aa4bfb8 on master.

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

tiffon

Looks great. Some questions and comments.

tiffon · 2018-04-04T06:21:14Z

test/throttler/remote_throttler.js

+import ConfigServer from '../lib/config_server';
+import LocalMetricFactory from '../lib/metrics/local/metric_factory.js';
+import LocalBackend from '../lib/metrics/local/backend.js';
+import RemoteThrottler from '../../src/throttler/remote_thottler';


Typo in filename, missing the "r" in throttler

that's a mighty sharp eye you have there

tiffon · 2018-04-04T06:28:00Z

src/samplers/remote_sampler.js

-        this._logger.error(`Error in fetching sampling strategy: ${err}.`);
-        this._metrics.samplerQueryFailure.increment(1);
-      });
+    let success: Function = body => {


Using an arrow function here can be avoided if the method is defined with an arrow function. Then you can just use the method as the callback.

_parseSamplingServerResponse = (body: string) => { // ... }

tiffon · 2018-04-04T06:35:30Z

src/throttler/remote_thottler.js

+
+  isAllowed(operation: string): boolean {
+    if (operation in this._credits) {
+      // TODO if credits is 0, is this false? Do I need to do an explicit check on undefined?


If this._credits.a === 0 then 'a' in this._credits is still true.

If null comes up as a value and you need to refresh if the value is null then you can check for

this._credits[operation] != `null`

Note the != instead of !== means the check will fail for both undefined and null

Or is that the point of initialDelayMs? Seems like it won't prevent the above scenario, though.

the credits can't ever be set to null so the current check will suffice

tiffon · 2018-04-04T06:39:56Z

src/throttler/remote_thottler.js

+    // N.B. The -1 assignment is necessary, otherwise we need to type credits as ?number which
+    // becomes a bit of a headache in this function because flow will throw errors on credits
+    // being null even if I explicitly check it's not before continuing.
+    let credits: number = operation in this._credits ? this._credits[operation] : 0;


This can be a const. And, this is vulnerable to the scenario where operation is in _credits but not a number, so then becomes NaN on line 114. An alternative is

const credits = this._credits[operation] || 0;

tiffon · 2018-04-04T06:42:02Z

src/throttler/remote_thottler.js

+    if (credits < MINIMUM_CREDITS) {
+      return false;
+    }
+    this._credits[operation] = credits - MINIMUM_CREDITS;


Looks like the minimum credit is doubling as the unit value? Maybe can be renamed to CREDIT_UNIT or UNIT_CREDIT or something?

tiffon · 2018-04-04T06:49:49Z

src/throttler/remote_thottler.js

+    }
+    this._credits[operation] = 0;
+    // If seen for the first time, async fetch credits
+    this._refreshCredits();


You might consider debouncing this some moderate amount of time so the wouldn't result in N calls to /credits (a possibility on initialization):

throttler.isAllowed('op-0'); throttler.isAllowed('op-1'); // ... throttler.isAllowed('op-N');

tiffon · 2018-04-04T06:54:44Z

src/throttler/remote_thottler.js

+
+    operations.forEach(operation => {
+      url = url + `&operation=${encodeURIComponent(operation)}`;
+    });


String concatenation is pretty slow in JS. An alternate is

const ops = operations.map(encodeURIComponent).join('&operation=') const url = `/credits?service=${serviceName}&uuid=${uuid}&operation=${ops}`

This led me astray: https://stackoverflow.com/questions/13859543/fast-way-to-concatenate-strings-in-nodejs-javascript

Is that link incorrect? I changed over to using join

Seems to vary based on the strings, themselves. But, they're all fairly close in most scenarios.

tiffon · 2018-04-04T06:59:35Z

src/throttler/remote_thottler.js

+
+  _afterInitialDelay(): void {
+    this._refreshCredits();
+    this._refreshIntervalHandle = setInterval(this._refreshCredits.bind(this), this._refreshIntervalMs);


You might consider setting this._initialDelayTimeoutHandle to null as it is no longer valid.

tiffon · 2018-04-04T07:00:46Z

src/throttler/remote_thottler.js

+  }
+
+  _isAllowed(operation: string): boolean {
+    // N.B. The -1 assignment is necessary, otherwise we need to type credits as ?number which


What -1 assignment?

sorry old comment

tiffon · 2018-04-04T07:02:12Z

src/throttler/remote_thottler.js

+    this._host = options.host || DEFAULT_THROTTLER_HOST;
+    this._port = options.port || DEFAULT_THROTTLER_PORT;
+
+    this._credits = Object.create(null);


What's the advantage of Object.create(null) vs {} in this case?

I copied over legacy code that did this. Since we're using the keys of the object, won't Object.keys({}) return things like _proto which is something we don't want right? I'll check

It should be fine:

The object of which the enumerable's own properties are to be returned.

MDN Object.keys

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

black-adder · 2018-04-04T21:46:56Z

addressed some comments, looking into debouncing and other comments

tiffon · 2018-04-05T17:59:05Z

src/throttler/remote_throttler.js

+      return;
+    }
+    const keys = Object.keys(this._credits);
+    if (keys.length == 0) {


I recommend using === instead of == for all cases except comparisons with null or undefined.

Ref: Loose equality using ==

tiffon · 2018-04-05T17:59:48Z

src/throttler/remote_throttler.js

+    let success: Function = body => {
+      this._parseCreditResponse(body);
+    };
+    let error: Function = err => {


I'd recommend any let that doesn't change be switched to a const.

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

tiffon · 2018-04-06T15:06:28Z

test/throttler/remote_throttler.js

+    throttler = new RemoteThrottler(serviceName);
+    throttler.setProcess({ uuid: null });
+    throttler._refreshCredits();
+  });


Should there be an assert, here?

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

tiffon

Some suggestions, mostly with regard to tests.

tiffon · 2018-04-10T15:16:14Z

src/metrics/metrics.js

@@ -120,5 +122,13 @@ export default class Metrics {
    });

    this.baggageTruncate = this._factory.createCounter('baggage-trucate');
+
+    this.throttlerUpdateSuccess = this._factory.createCounter('throttler-update', {


I don't know the params for this factory, but should this string be something like throttler-update-success and the next one throttler-update-failure?

tiffon · 2018-04-10T15:18:04Z

src/samplers/remote_sampler.js

-        this._logger.error(`Error in fetching sampling strategy: ${err}.`);
-        this._metrics.samplerQueryFailure.increment(1);
-      });
+    let success: Function = body => {


I recommend using const on values that don't change.

tiffon · 2018-04-10T15:18:54Z

src/throttler/default_throttler.js

+export default class DefaultThrottler {
+  _throttleAll: boolean;
+
+  constructor(throttleAll: ?boolean) {


I think this should be throttleAll?: boolean

tiffon · 2018-04-10T15:21:55Z

src/throttler/remote_throttler.js

+    if (operation in this._credits) {
+      return this._isAllowed(operation);
+    }
+    // Credits for the operation will be asynchronously fetched


Just curious, is relying on the interval consistent with the Go and Java throttlers?

tiffon · 2018-04-10T15:23:32Z

src/throttler/remote_throttler.js

+    }
+  }
+
+  close(callback: ?Function): void {


I believe this should be callback?: Function, or callback?: () => void

tiffon · 2018-04-10T15:25:12Z

test/lib/config_server.js

    this._strategies = Object.create(null);
+    this._credits = Object.create(null);


I think all of these Object.create(null) should just be {}...

bah, these are all over the code base. I'll add a subsequent PR to change all these at once

tiffon · 2018-04-10T15:50:12Z

test/throttler/remote_throttler.js

+    throttler.setProcess({ uuid: uuid });
+    throttler._credits[operation] = 0;
+    server.addCredits(serviceName, [{ operation: operation, credits: 5 }]);
+    throttler._onCreditsUpdate = _throttler => {


Looks like you can avoid overwriting the private(-ish) property by using the options.onCreditUpdate. This would avoid relying on the private property and breaking the public API would break the test.

let creditUpdatedHook; beforeEach(() => { creditUpdatedHook = sinon.spy(); throttler = new RemoteThrottler(serviceName, { // ... onCreditUpdate: (...args) => creditUpdatedHook(...args), }); }); it('....', () => { assertTrue(creditUpdatedHook.called); }); it('...', done => { creditUpdatedHook = () => { // ... done(); } });

great refactoring, will do

tiffon · 2018-04-10T15:50:48Z

test/throttler/remote_throttler.js

+    throttler._refreshCredits();
+  });
+
+  it('emit failure metric on failing to query for credits', done => {


For the failure scenarios, you might consider also ensuring the credit update callback does not fire.

what would be the best way to do this? the callback happens asynchronously, do I overwrite the done function to assert that the callback is not called? Might increase how long our tests take substantially.

I think I generally use setTimeout(..., 0), so something like:

it('emit failure metric when server returns an invalid response', done => { throttler.setProcess({ uuid: uuid }); throttler._credits[operation] = 0; metrics.throttlerUpdateFailure.increment = function() { assert.equal(logger._errorMsgs.length, 1, `errors=${logger._errorMsgs}`); setTimeout(() => { // assert done(); }, 0) }; throttler._refreshCredits(); });

stupid setTimeout issue on travis strikes again. I think asserting that the logs were added are good enough for these tests.

That is really unfortunate. Sort of mild in this case, but in general it's undermining our test-coverage...

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

tiffon · 2018-04-10T20:22:19Z

test/throttler/remote_throttler.js

+    throttler = new RemoteThrottler(serviceName);
+    throttler.setProcess({ uuid: uuid });
+    throttler._refreshCredits();
+    assert.equal(Object.keys(throttler._credits).length, 0);


Does this assert verify the fetch doesn't happen?

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

tiffon · 2018-04-12T15:14:31Z

src/_flow/jaeger-thrift.js

@@ -24,6 +24,9 @@ declare type LogData = {
 declare type Process = {
  serviceName: string,
  tags: Array<Tag>,
+  // N.B. uuid uniquely identifies this instance of the client. It is only used by the client and is not


Is the UUID used by the client and the agent?

tiffon · 2018-04-12T15:24:27Z

test/throttler/remote_throttler.js

+    throttler._refreshCredits();
+  });
+
+  it('emit failure metric on failing to query for credits', done => {


That is really unfortunate. Sort of mild in this case, but in general it's undermining our test-coverage...

tiffon

Looks great, two nits

tiffon · 2018-04-12T15:27:44Z

src/throttler/remote_throttler.js

+    if (operation in this._credits) {
+      return this._isAllowed(operation);
+    }
+    // Credits for the operation will be asynchronously fetched


tiffon · 2018-04-12T15:30:06Z

src/throttler/remote_throttler.js

+
+  close(callback?: Function): void {
+    clearTimeout(this._initialDelayTimeoutHandle);
+    clearInterval(this._refreshIntervalHandle);


There is an edge case of a credits fetch finishing after close is called.

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

closes jaegertracing#242 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

Add throttler

0295804

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

ghost assigned black-adder Apr 2, 2018

ghost added the review label Apr 2, 2018

update comment

e2ec1d6

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

black-adder commented Apr 2, 2018

View reviewed changes

fix flow

51cfd26

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

black-adder commented Apr 2, 2018

View reviewed changes

black-adder added 3 commits April 2, 2018 18:23

this

4d91c0f

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

add logging for async tests

b910ef8

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

attempt 5

cd6388b

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

black-adder commented Apr 3, 2018

View reviewed changes

black-adder added 3 commits April 3, 2018 11:54

increase timeout... for travis

f4a9c23

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

increase timeout v2... for travis

6e9d370

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

remove test

094bd23

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

black-adder added 4 commits April 3, 2018 13:41

dont use map since its not available in node 0.10

b22cfe4

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

try the test again?

325df4b

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

longer timeout

64db44d

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

remove bad test

38c33f6

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

tiffon reviewed Apr 4, 2018

View reviewed changes

address comments

681a67b

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

tiffon reviewed Apr 5, 2018

View reviewed changes

address comments

7a144b5

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

tiffon reviewed Apr 6, 2018

View reviewed changes

black-adder added 2 commits April 6, 2018 14:53

update comments

87c6e0f

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

sigh

0b1620f

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

tiffon reviewed Apr 10, 2018

View reviewed changes

address comments

86a03ed

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

tiffon reviewed Apr 10, 2018

View reviewed changes

black-adder added 3 commits April 10, 2018 17:01

please joe

c3fa736

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

et tu travis?

b6196cd

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

setTimeout never works on travis

e48c4c7

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

tiffon reviewed Apr 12, 2018

View reviewed changes

tiffon approved these changes Apr 12, 2018

View reviewed changes

black-adder and others added 2 commits April 12, 2018 12:00

update comment

ee7ec9d

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

Merge branch 'master' into add_throttler

00df710

black-adder merged commit 1608b0d into master Apr 12, 2018

ghost removed the review label Apr 12, 2018

black-adder deleted the add_throttler branch April 12, 2018 16:26

Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021

chore: remove script posttest (jaegertracing#246)

ea28887

closes jaegertracing#242 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

		this._strategies = Object.create(null);
		this._credits = Object.create(null);

Add throttler #246

Add throttler #246

Conversation

black-adder commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 3, 2018 • edited

tiffon 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

black-adder Apr 4, 2018 • edited

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

black-adder commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon 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

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 3, 2018 •

edited

black-adder Apr 4, 2018 •

edited