Named rule support; v1.4.0 by mikeybtn · Pull Request #7 · button/divvy

mikeybtn · 2019-05-08T15:31:12Z

No description provided.

mikeybtn · 2019-05-08T21:18:06Z

Pushed 'ready for review' too soon, looks like something got lost on the way to external - stand by..

mikeybtn · 2019-05-08T21:22:25Z

..ready to go!

danielmcgrath · 2019-05-08T21:23:55Z

src/server.js

      const result = status.isAllowed ? 'accepted' : 'rejected';
      const matchType = Server.getMatchType(rule);
-      this.instrumenter.countHit(result, matchType);
+      const ruleName = rule && rule.name ? rule.name : '';


s/name/label

danielmcgrath · 2019-05-08T21:25:44Z

src/instrumenter.js

    this.statsd.increment(`hit.${status}`);
    this.statsd.increment(`hit.${status}.${type}`);
-    this.hitCounter.labels(status, type).inc();
+    this.hitCounter.labels(status, type, name || '').inc();


You're handling name || '' both here and at the call-site. Doesn't hurt anything, though.

Separately, rename the name param here to label?

Separately, rename the name param here to label?

good catch, fixed!

danielmcgrath · 2019-05-08T21:28:32Z

README.md


 The following optional fields are also supported:

+* `label`: A short, slug-like name for the rule. If set, Prometheus metrics for hits matching the rule will be labeled with `rule_label` as this value.


I think it would be helpful to call out explicitly that this value is ignored for statsd metrics. We can consider adding support for additional statsd counters on a per-label basis down the road?

i · 2019-05-09T17:21:53Z

tests/config-test.js

          creditLimit: 1,
          resetSeconds: 60,
          actorField: '',
+          label: null,


most configs won't explicitly set label to null.
maybe specify one as undefined and another with no key for label for good measure?

i · 2019-05-09T17:31:05Z

src/config.js

   * @param {string} comment      Optional diagnostic name for this rule.
   */
-  addRule(operation, creditLimit, resetSeconds, actorField, comment) {
+  addRule(operation, creditLimit, resetSeconds, actorField, label, comment) {


since label is an optional argument, why position it second to last?
there's a bunch of places in tests/config-test.js that will need to be updated to reflect this positional change.

alternatively, since the cardinality of this function is getting big, what are your thoughts on having it accept an arguments object instead of positional arguments?

since label is an optional argument, why position it second to last?

Since both label and comment are optional, am guessing I somewhat arbitrarily decided labels will be more common than comments.

alternatively, since the cardinality of this function is getting big, what are your thoughts on having it accept an arguments object instead of positional arguments?

I like it, that'd definitely make it a bunch more readable! May sheepishly defer this suggestion as am running into limited cycles to iterate further. OK with you to land as is?

Adds test coverage for `countHit` with a label in `server-test`.

mikeybtn

PTAL!

mikeybtn · 2019-05-14T14:02:15Z

README.md


 The following optional fields are also supported:

+* `label`: A short, slug-like name for the rule. If set, Prometheus metrics for hits matching the rule will be labeled with `rule_label` as this value.


mikeybtn · 2019-05-14T14:05:58Z

src/config.js

   * @param {string} comment      Optional diagnostic name for this rule.
   */
-  addRule(operation, creditLimit, resetSeconds, actorField, comment) {
+  addRule(operation, creditLimit, resetSeconds, actorField, label, comment) {


since label is an optional argument, why position it second to last?

Since both label and comment are optional, am guessing I somewhat arbitrarily decided labels will be more common than comments.

alternatively, since the cardinality of this function is getting big, what are your thoughts on having it accept an arguments object instead of positional arguments?

I like it, that'd definitely make it a bunch more readable! May sheepishly defer this suggestion as am running into limited cycles to iterate further. OK with you to land as is?

mikeybtn · 2019-05-14T14:07:26Z

src/instrumenter.js

    this.statsd.increment(`hit.${status}`);
    this.statsd.increment(`hit.${status}.${type}`);
-    this.hitCounter.labels(status, type).inc();
+    this.hitCounter.labels(status, type, name || '').inc();


Separately, rename the name param here to label?

good catch, fixed!

i · 2019-05-14T15:11:41Z

src/server.js

      const result = status.isAllowed ? 'accepted' : 'rejected';
      const matchType = Server.getMatchType(rule);
-      this.instrumenter.countHit(result, matchType);
+      const ruleLabel = rule && rule.label ? rule.label : '';


nit: what's the order of operations here? can you clarify with parens?

const ruleLabel = (rule && rule.label) ? rule.label : ''; const ruleLabel = rule && (rule.label ? rule.label : '');

assuming it's the first since, but not clear from reading

Can do. Ternary is pretty much lowest precedence operator in JS so its not uncommon to see 'naked' expressions like this ahead of it. (In fact your suggestion would be rejected by rules/no-extra-parens but we don't enable it.)

d'oh! no need to include for my sake if it's an antipattern.

i think its fine, happy to follow the "if someone asked, its probably worth doing/clarifying" school of thought here :)

i

LGTM. Defer to your heart with regards to leaving those parens in or out.

mikeybtn requested review from danielmcgrath and i May 8, 2019 21:17

mikeybtn marked this pull request as ready for review May 8, 2019 21:17

mikeybtn added 2 commits May 8, 2019 17:21

Feature: Labeled (named) rules.

ba1cb63

v1.4.0

3a81e07

mikeybtn force-pushed the mikey/named-rules branch from adedc56 to 3a81e07 Compare May 8, 2019 21:22

danielmcgrath reviewed May 8, 2019

View reviewed changes

i reviewed May 9, 2019

View reviewed changes

PR feedback

29b93f8

Adds test coverage for `countHit` with a label in `server-test`.

mikeybtn force-pushed the mikey/named-rules branch from 5a32a28 to 29b93f8 Compare May 14, 2019 14:31

mikeybtn commented May 14, 2019

View reviewed changes

i reviewed May 14, 2019

View reviewed changes

PR feedback

b04a5ed

mikeybtn requested a review from i May 14, 2019 18:50

i approved these changes May 14, 2019

View reviewed changes

mikeybtn merged commit 201ed5e into master May 15, 2019

mikeybtn deleted the mikey/named-rules branch May 15, 2019 13:19


		The following optional fields are also supported:

		* `label`: A short, slug-like name for the rule. If set, Prometheus metrics for hits matching the rule will be labeled with `rule_label` as this value.

Conversation

mikeybtn commented May 8, 2019

Uh oh!

mikeybtn commented May 8, 2019

Uh oh!

mikeybtn commented May 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeybtn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

i May 9, 2019 •

edited

Loading