Fix matching scheme #6

junr03 · 2017-02-01T01:33:00Z

@lyft/network-team

The following request:

RateLimitRequest:
  domain: example
  descriptor: ("key", "value"),("subkey", "subvalue")

Would match the following config:

domain: example
descriptors:
  - key: key
    value: value
    rate_limit:
      -  requests_per_unit: 300
         unit: second

Arguably the request should not match that config, it should only match a config entry like this:

domain: example
descriptors:
  - key: key
    value: value
    descriptors:
      - key: subkey      
        rate_limit:
          -  requests_per_unit: 300
             unit: second

So in general the ratelimit service should match the most specific rule with the same depth level as the request.

ccaraman · 2017-02-01T18:54:06Z

README.md

+
+#### Example 4
+
+The Ratelimit service matches requests to configuration entries with the same depth level. For instance, the following request:


It might be nice to define what you mean by depth level.

ccaraman · 2017-02-01T18:57:14Z

src/config/config_impl.go

@@ -265,7 +265,11 @@ func (this *rateLimitConfigImpl) GetLimit(

 		if nextDescriptor != nil && nextDescriptor.limit != nil {
 			logger.Debugf("found rate limit: %s", finalKey)


should this logger statement be moved into the if below?

I dont think so. In debug we want to know about all the ratelimits found, the next logger message would let us know if the ratelimit was not chosen.

ccaraman · 2017-02-01T19:06:04Z

test/config/config_test.go

+
+	rl = rlConfig.GetLimit(
+		nil, "test-domain",
+		&pb.RateLimitDescriptor{[]*pb.RateLimitDescriptor_Entry{{"key5", "value5"}, {"subkey5", "subvalue"}}})


What are you trying to test here?

The config did not get pushed in the branch. Just updated. I am testing that RL will not return a limit for two tuples, even though there was a descriptor with less nesting in the config that matched.

ccaraman · 2017-02-01T19:35:53Z

Once #2 has been merged, please update this pr to use consistent language when referring to depth vs descriptor list level example :https://github.com/lyft/ratelimit/blob/e6eb371b25f2214027f352c80a7ea56d86793cb7/README.md#descriptor-list-definition

ccaraman · 2017-02-01T21:04:41Z

README.md

-*attempt the most specific match possible*. This means both the most nested matching descriptor entry, as well as
-the most specific at any descriptor list level. Thus, key/value is always attempted as a match before just key.
+*attempt the most specific match possible*. This means
+the most specific descriptor at the same level as your request. Keep in mind that equally specific descriptors are matched on a first match basis. Thus, key/value is always attempted as a match before just key.


Can you rewrite the second sentence Keep in mind... what determines what is first? Order specified in the config?

I take that sentence back.

mattklein123 · 2017-02-03T17:30:33Z

README.md


 ## Loading Configuration

 The Ratelimit service uses a library written by Lyft called [goruntime](https://github.com/lyft/goruntime) to do configuration loading. Goruntime monitors
+>>>>>>> master


merge conflict

ccaraman · 2017-02-03T23:53:11Z

README.md

@@ -178,13 +179,14 @@ RateLimitRequest:
  descriptor: ("to_number", "2061111111")
 ```

-And the service with rate limit against *all* matching rules and return an aggregate result.
+And the service with rate limit against *all* matching rules and return an aggregate result; a logical OR of all


will instead of with?

ccaraman · 2017-02-03T23:58:34Z

README.md

+#### Example 4
+
+The Ratelimit service matches requests to configuration entries with the same level, i.e
+same number of tuples in the request's descriptor as nested levels of descriptors


nit: should this same level description go up higher? I don't know...

You could argue for or against. Each example explains something about matching, and how this works so it takes all four examples to understand.

ccaraman · 2017-02-03T23:58:59Z

README.md

+  descriptor: ("key", "value"),("subkey", "subvalue")
+```
+
+Would **not** match the following configuration, even though the first descriptor in


run together sentence.

Jose Nino added 3 commits January 30, 2017 13:19

Add README

7f103ab

Merge branch 'master' into docs

e6eb371

Fix matching scheme

4c7ea53

ccaraman reviewed Feb 1, 2017

View reviewed changes

Jose Nino added 2 commits February 1, 2017 11:15

address comments

51c94ab

fix

53b34a6

ccaraman reviewed Feb 1, 2017

View reviewed changes

Merge branch 'master' into fix-matching

7631bc1

junr03 changed the base branch from docs to master February 2, 2017 22:38

Update from comments

90fd9fb

mattklein123 reviewed Feb 3, 2017

View reviewed changes

remove merge conflict

0d1f996

ccaraman reviewed Feb 3, 2017

View reviewed changes

Update from comments

fe0e5da

mattklein123 approved these changes Feb 4, 2017

View reviewed changes

junr03 merged commit 3c92419 into master Feb 6, 2017

junr03 deleted the fix-matching branch February 6, 2017 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix matching scheme #6

Fix matching scheme #6

junr03 commented Feb 1, 2017

ccaraman Feb 1, 2017

junr03 Feb 1, 2017

ccaraman Feb 1, 2017

junr03 Feb 1, 2017

ccaraman Feb 1, 2017

junr03 Feb 1, 2017

ccaraman commented Feb 1, 2017

ccaraman Feb 1, 2017

junr03 Feb 2, 2017

mattklein123 Feb 3, 2017

junr03 Feb 3, 2017

ccaraman Feb 3, 2017

junr03 Feb 4, 2017

ccaraman Feb 3, 2017

junr03 Feb 4, 2017 •

edited

Loading

ccaraman Feb 3, 2017

junr03 Feb 4, 2017


		#### Example 4

		The Ratelimit service matches requests to configuration entries with the same depth level. For instance, the following request:

		@@ -265,7 +265,11 @@ func (this *rateLimitConfigImpl) GetLimit(

		if nextDescriptor != nil && nextDescriptor.limit != nil {
		logger.Debugf("found rate limit: %s", finalKey)

Fix matching scheme #6

Fix matching scheme #6

Conversation

junr03 commented Feb 1, 2017

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

ccaraman commented Feb 1, 2017

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

junr03 Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 Feb 4, 2017 •

edited

Loading