Kong / kong

🦍 The Cloud-Native API Gateway and AI Gateway.
https://konghq.com/install/#kong-community
Apache License 2.0
38.85k stars 4.77k forks source link

Metric counter #227

Closed jeevanrd closed 9 years ago

jeevanrd commented 9 years ago

Our use case is to keep track of number of records in an api call and want to do rate limiting on top of this not at the api level. Currently kong supports tracking at api call. correct me if I am wrong. And let me know is this something already available?? EX: http://example.com/products?limit=10 returns 10 products. The kong currently counts as one api call Instead of that i want to increment by 10. Otherwise we would like to send a PR for this change.

jeevanrd commented 9 years ago

@thefosk @sinzone @thibaultCha

I have started to create a custom plugin to enforce constraints on data usage.

How to configure in api?? Assumption: The backend api always send the record count in the custom response heaader so that we can capture and increment the counters accordingly. If the api doesn't send this header, we will default it to 1

While configuring the plugin, configure the response header name on which the no of records will be sent

Ex: curl -X POST http://localhost:8001/plugins_configurations/ \ --data "name=datausage" \ --data "api_id=API_ID" \ --data "value.metric_counter_variable=RECORD_COUNT" \ --data "value.limit=hour:100"

About implementation?? I have created a new column family whose schema is similar to that of ratelimiting_metrics and increment the counter by reading the count from response

Pull request: https://github.com/Mashape/kong/pull/247

Please let me know your views??

sonicaghi commented 9 years ago

another thing to consider in rate limiting other than custom objects (the solution you just wrote) it's about having a limit with no timing value. So can be 10 products or 10 calls "overall" and not by timeframe.

So in the rate limiting plugin we can make the value.limit optional or keep mandatory but with the option to put "null" in case the API provider wants an overall limit. cc @thibaultCha @thefosk

thibaultcha commented 9 years ago

This is a really valuable feature :+1: I gave my feedback on the PR: it should probably be part of the ratelimiting plugin.

@sinzone This is also a very valuable feature. We could either accept a value with no limit, but to avoid confusion we could accept something like overall:X, so the user knows he is enforcing a permanent limit.

orliesaurus commented 9 years ago

:+1:

sonicaghi commented 9 years ago

+1 for overall:X @jeevan1986 can you add it to the PR?

subnetmarco commented 9 years ago

I am noticing one thing regarding the Rate-Limiting, which is that there are really lots of ways of doing it:

How about having a "Rate-Limiting" section in our Plugins page, with all the different variants, as opposed to having one huge plugin which imho becomes harder to configure. People could also be able to combine even more than one rate-limiting together: for example "per-request" and "per-querystring param". Or maybe only rate-limit some users "per-request", while keeping the others on a "per-param" setting.

thibaultcha commented 9 years ago

per querystring parameter per body parameter

Then a user can just change them? The only valid way I see it is like @jeevan1986 implemented it: with a header from the server.

The issue with having multiple retaliating plugins is what we will end up with a ton of duplicated code.

thibaultcha commented 9 years ago

Oh I see with parameters. But still, we would end up with a lot of duplicated code where doing that in the current plugin would only take a few lines... And the same column family.

subnetmarco commented 9 years ago

The issue with having multiple retaliating plugins is what we will end up with a ton of duplicated code.

I agree with this, but at the same time I kinda like having "micro-plugins" instead of huge plugins. Maybe there is a clever way of doing this without replicating too much code.

I am just brainstorming now, but what is preventing us from having multiple schemas in the same plugin, and then just document each schema in a different plugin webpage in our plugins gallery. The code could effectively be shared more easily since it would be part of the same project/folder, but at the same time we have the advantage of micro-plugins.

Otherwise if not possible the other option would be a classic shared library.

thibaultcha commented 9 years ago

If one was using a ratelimiting plugin and suddenly wants to add a data parameter like this to increase the counter differently, then he or she is stuck, and have to configure a whole new plugin for it.

It's not a big deal, in the schema add:

data_usage_location = {"response_header", "querystring", "payload"}
data_usage_name = "..."

And now we know when and how to increment the counter. It's stil la very lightweight plugin.

sonicaghi commented 9 years ago

i see this plugin becoming bigger and bigger over time. better to have a section called Rate Limiting and we put 3 or 4 plugins in there.

thibaultcha commented 9 years ago

? It is 50 lines of code.

plugins|master ⇒ sloc ratelimiting

---------- Result ------------

            Physical :  67
              Source :  47
             Comment :  7
 Single-line comment :  7
       Block comment :  0
               Mixed :  2
               Empty :  15

Number of files read :  3

------------------------------
ahmadnassri commented 9 years ago

with Luarocks you can setup a base / shared core as dependency and still separate out into multiple Kong plugins, thus giving users freedom to select "plugins" specific to their needs.

sonicaghi commented 9 years ago

for now it's small but for example a full rate limiting has 10 more features than the one you have now. Not talking about LOC or KB. but business prospective is better to segment to better position the value of each plugin.

thibaultcha commented 9 years ago

I am telling you you will end up with more problems, but once again, ignore my warning.

sonicaghi commented 9 years ago

I prefer micro-plugins vs larger plugins.

subnetmarco commented 9 years ago

I think he's referring to the configuration required when installing each plugin, not the size of the files:

From an experience perspective it's the difference between:

curl -d "value.param=counter&value.limit=10&value.period=second" ..

as opposed to:

curl -d "value.data_usage_location=querystring&value.data_usage_name=counter&value.limit=10&value.period=second" ..

By the way on a side note I just realized the period property is so ugly, would be nice to just be able to say value.limit=10s or value.limit=10d.

thibaultcha commented 9 years ago

Welcome to the future, latest ratelimiting PR has period:value notation.

subnetmarco commented 9 years ago

I am talking about a shorter notation, as you can see above.

thibaultcha commented 9 years ago

Is 10m minute or month?

subnetmarco commented 9 years ago

I saw some frameworks using either mn for minute, or mm for month.

thibaultcha commented 9 years ago

Also either your example assumes there is one plugin for each type (body, response header, querystring), which seems pretty bad, either it is biased.

It should be:

From an experience perspective it's the difference between:

curl -d "value.param=counter&value.param_location=header&value.limit=10s"

as opposed to:

curl -d "value.param=counter&value.param_location=header&value.limit=10s"

So as you can see, when using the same parameter names (hehe) and with the proper parameters in the first request, we can see they're just as easy to configure.

subnetmarco commented 9 years ago

In my examples one of the use-cases (asked by @russler) is to support a rate-limiting based on the length of a JSON array in the body. That would require a new location category for example, and the locations can grow. My idea was to have something like this, so we can easily add more categories while keeping it very simple to understand what features Kong support at first glance:

screen shot 2015-05-19 at 5 30 23 pm

This has two benefits:

Negative effect:

Another option (with same benefits):

jeevanrd commented 9 years ago

:+1:

sonicaghi commented 9 years ago

It all depends on how much duplicate code we'll have. If it's going to be substantial then I prefer the cleaner:

data_usage_location = {"response_header", "querystring", "payload"} data_usage_name = "..."

subnetmarco commented 9 years ago

We already have a code duplication problem in the authentication plugins for example, we need to setup a pattern for these cases since it's unavoidable to have code duplication now or in the future.

sonicaghi commented 9 years ago

but having in it in the past doesn't mean we should do it in the future.

subnetmarco commented 9 years ago

As the numbers of plugins increases, and especially as we allow third-party plugins to easily be created, we will have a code duplication unless we setup a pattern, one of which could be a shared library between plugins that need to use the same code.

sonicaghi commented 9 years ago

def need some common rules before it becomes a Jungle.

thibaultcha commented 9 years ago

Implemented in #493.