Initialize dynamic-sampler #1

ChristopherBiscardi · 2018-02-21T18:53:12Z

Implement two samplers, PerKey and Avg, with the same interface. Check
README for how to create new samplers and other docs.

Public API is to instantiate a new sampler and then use getSampleRate(key).

const sampler = new SomeSampler()
sampler.getSampleRate("some-key")

Implement two samplers, PerKey and Avg, with the same interface. Check README for how to create new samplers and other docs. Public API is to instantiate a new sampler and then use `getSampleRate(key)`. ```javascript const sampler = new SomeSampler() sampler.getSampleRate("some-key") ```

ChristopherBiscardi · 2018-02-21T18:54:17Z

index.test.js

@@ -0,0 +1,52 @@
+jest.mock("nanotimer");


this line isn't required but makes it nice for the next person who might be unfamiliar with mocks and why nanotimer now has a .tick() function but only in tests.

+1 this line in general. better to be explicit for future readers.

… and we're publishing multiple formats

eanakashima

Yay, excited about this! A few questions in line but nothing that seems like a blocker. Haven't had a chance to pull it down to test out, but the API all makes sense to me.

eanakashima · 2018-02-22T18:01:27Z

README.md

+    }
+  }
+}
+```


Love all the code examples in the README. 😍

Not a blocker for shipping this at all, but at some point pulling in more of the verbiage from the dynsampler-go readme — esp the "what is sampling" bits from the top — could be handy. Want me to open issues for readme nice-to-haves like that? (The other one I'm thinking of is the answer to "can I use this in a browser?" 😳)

yes please! Open issues and I'll knock em down.

Yay, will do.

eanakashima · 2018-02-22T18:23:52Z

README.md

+    });
+    this.savedSampleRates = newRates;
+  }
+}


I thought the example invocation in the source comment was helpful — maybe worth having that in this example too?

const sampler = new PerKey({clearFrequencySec: 100, perKeyThroughputSec: 2})

Helps document the config options too.

eanakashima · 2018-02-22T18:58:56Z

index.test.js

+
+  test("gets a sample rate", () => {
+    const sampler = new PerKey();
+    new Array(1500).fill(1).forEach(() => sampler.getSampleRate("my-key"));


Any reason behind 1500 here? Just curious.

just needed a value that I can predict the sample rate of

eanakashima · 2018-02-22T21:58:43Z

package.json

+  "repository": "https://github.com/honeycombio/dynamic-sampler.js",
+  "author":
+    "Christopher Biscardi <[email protected]> (@chrisbiscardi)",
+  "license": "MIT",


Sorry to resurrect our chat bikeshed on this topic, but thoughts on using Apache 2.0 for consistency with our other repos, for now?

sure.

For later discussion: This is the default boilerplate for yarn init and npm init though so we should write down the reasoning for "why apache 2.0" somewhere since this is the community default for JS.

eanakashima · 2018-02-22T23:41:27Z

index.test.js

+    // No traffic means this is the goal sample rate
+    expect(a).toEqual(10);
+    sampler.timer.tick();
+    expect(sampler.getSampleRate("my-key")).toEqual(1);


Not a blocker, but it takes me a fair amount of mental context threading to see how the log math above leads to this set of expectations... maybe a few more comments or something a little more verbose would be helpful? (The dynsampler-ruby equivalent feels more parseable somehow.)

~~I'm curious if it's the table-based approach in the ruby tests that makes it more parseable~~

Cross that, the ruby test looks like we're reaching into the internals to set the rate and it's not actually triggering an updateMap so wouldn't have the additional ticks. For comparison, this test is only using the public API with a mock for timers so we can manually trigger the updateMap() through the timer callback.

Curious as to your opinion on the difference in the tests. This test feels like it approximates more real-world usage

create a sampler

throw traffic at it

the timer callback triggers updateMaps

get the sample rate

Whereas the ruby test seems like:

Reach into internals to set internal properties

Check that those internal properties match what we'd expect

~~ugh, when did Shift+Enter become "commit comment". Too much time in Slack.~~

Do you think adding the "stage" comments above is enough? or should I try to do something else.

Ah yeah, fair point, agree that it's a bit nicer to focus on testing the public API. 👍for adding the stage comments for now and I'll noodle more on what my dream tests would look like.

toshok

general bikeshed comment:

dynsampler-js
AvgSampleRate + PerKeyThroughput
ideally dynsampler for the package name, but a lot more leeway for package idioms there.

toshok · 2018-02-21T20:54:13Z

index.test.js

@@ -0,0 +1,52 @@
+jest.mock("nanotimer");


+1 this line in general. better to be explicit for future readers.

toshok · 2018-02-25T04:20:50Z

index.js

+export class PerKey extends Sampler {
+  constructor(opts = {}) {
+    super(opts);
+    this.perKeyThroughputSec = opts.perKeyThroughputSec || 5;


both go and ruby default to 10 here.

toshok · 2018-02-25T04:22:17Z

index.js

+  updateMaps() {
+    debug("PerKey.updateMaps()", this.id && this.id);
+    if (this.currentCounts.size == 0) {
+      //no traffic in the last 30s. clear the result Map


suuuuuper minor nit. s/30s/clearFrequencySec/

toshok · 2018-02-25T04:46:34Z

README.md

+
+    const newRates = new Map();
+    this.currentCounts.forEach((val, key) => {
+      newRates.set(key, Math.max(1, val / actualPerKeyRate));


both ruby and golang coerce the value to an int. :shakes fist: at JS. any reason not to do that here?

Nope, going to go with Math.floor to match golang.

toshok · 2018-02-25T04:48:31Z

index.js

+
+    const newRates = new Map();
+    this.currentCounts.forEach((val, key) => {
+      newRates.set(key, Math.max(1, val / actualPerKeyRate));


same here wrt coercing to an int

toshok · 2018-02-25T04:58:39Z

index.js

+    });
+    const goalCount = sumEvents / this.goalSampleRate;
+    const goalRatio = goalCount / logSum;
+


both the go/ruby implementations sort the keys here and iterate in sorted order. my head hurts looking the ruby implementation. I expect it's a problem to drop the sort, but ... /shrug?

Since we're using Maps I'm pretty sure we're good here, unless we need a specific kind of sort?

The forEach() method executes a provided function once per each key/value pair in the Map object, in insertion order.

yeah that's the question I had - if we give the dynsamplers the same input, do we expect the same results? both other implementations go beyond the "sort them in the same order for this implementation to fix rounding errors" and use a sort that should be the same across implementations.

eh, we can deal with that when it's an issue for someone (e.g. "I'm using two dynsampler implementations and things are different between them"). And the answer even then might be WONTFIX.

toshok · 2018-02-25T04:59:57Z

index.js

+    } else {
+      // TODO: how well supported is this syntax
+      // (basically no IE native support. what about babel/etc?)
+      return super.getSampleRate(key);


I hope babel supports it. I've been writing code for a diff that uses it 😬

I believe it does. My concern was centered more at what buble does and whether we should care about that before microbundle gets it's babel support: developit/microbundle#39.

eanakashima

Ah, +1 "dynsampler-js" as the project name, can't believe I didn't look at that 🤦‍♂️. There have been recent efforts to make our herd of open source projects a little better at following naming conventions, so seems like a good chance to establish a convention here too.

eanakashima · 2018-02-25T06:40:21Z

README.md

+    }
+  }
+}
+```


Yay, will do.

eanakashima · 2018-02-25T06:44:31Z

index.test.js

+    // No traffic means this is the goal sample rate
+    expect(a).toEqual(10);
+    sampler.timer.tick();
+    expect(sampler.getSampleRate("my-key")).toEqual(1);


Ah yeah, fair point, agree that it's a bit nicer to focus on testing the public API. 👍for adding the stage comments for now and I'll noodle more on what my dream tests would look like.

ChristopherBiscardi · 2018-03-01T22:47:27Z

@toshok Are there still changes here you want to see?

I'd like to merge it and move on to more work like setting up Travis, etc.

toshok · 2018-03-02T16:32:08Z

thanks for the ping. other than repo renaming, this LGTM

ChristopherBiscardi commented Feb 21, 2018

View reviewed changes

ChristopherBiscardi requested review from toshok, eanakashima and christineyen February 21, 2018 20:36

use named exports for everything because microbundle knows whats what…

caadb53

… and we're publishing multiple formats

eanakashima approved these changes Feb 22, 2018

View reviewed changes

toshok suggested changes Feb 25, 2018

View reviewed changes

eanakashima reviewed Feb 25, 2018

View reviewed changes

some pr feedback

9345abf

toshok approved these changes Mar 2, 2018

View reviewed changes

ChristopherBiscardi merged commit 8ed199f into master Mar 2, 2018

robbkidd deleted the first-push branch January 27, 2021 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize dynamic-sampler #1

Initialize dynamic-sampler #1

ChristopherBiscardi commented Feb 21, 2018

ChristopherBiscardi Feb 21, 2018

toshok Feb 21, 2018

eanakashima left a comment

eanakashima Feb 22, 2018

ChristopherBiscardi Feb 23, 2018

eanakashima Feb 25, 2018

eanakashima Feb 22, 2018

eanakashima Feb 22, 2018

ChristopherBiscardi Feb 23, 2018

eanakashima Feb 22, 2018

ChristopherBiscardi Feb 23, 2018

eanakashima Feb 22, 2018

ChristopherBiscardi Feb 23, 2018

ChristopherBiscardi Feb 23, 2018

eanakashima Feb 25, 2018

toshok left a comment

toshok Feb 21, 2018

toshok Feb 25, 2018

toshok Feb 25, 2018

toshok Feb 25, 2018

ChristopherBiscardi Feb 26, 2018

toshok Feb 25, 2018

toshok Feb 25, 2018

ChristopherBiscardi Feb 26, 2018 •

edited

Loading

toshok Feb 26, 2018

toshok Feb 25, 2018

ChristopherBiscardi Feb 26, 2018

eanakashima left a comment

eanakashima Feb 25, 2018

eanakashima Feb 25, 2018

ChristopherBiscardi commented Mar 1, 2018 •

edited

Loading

toshok commented Mar 2, 2018

Initialize dynamic-sampler #1

Initialize dynamic-sampler #1

Conversation

ChristopherBiscardi commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanakashima 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

toshok 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

ChristopherBiscardi Feb 26, 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

eanakashima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherBiscardi commented Mar 1, 2018 • edited Loading

toshok commented Mar 2, 2018

ChristopherBiscardi Feb 26, 2018 •

edited

Loading

ChristopherBiscardi commented Mar 1, 2018 •

edited

Loading