Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(proxy-mirror): support mirror requests sample_ratio #4965

Merged
merged 46 commits into from
Sep 10, 2021

Conversation

okaybase
Copy link
Member

@okaybase okaybase commented Sep 2, 2021

What this PR does / why we need it:

support mirror requests sample_ratio

use case:
In the simulation environment, user hope to use a small amount of real traffic to verify the new function, but the real traffic may be huge, which puts great pressure on the simulation service.

Fix #3753

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give some use cases about this feature?

@okaybase
Copy link
Member Author

okaybase commented Sep 2, 2021

Could you give some use cases about this feature?
okay~
In the simulation environment, we hope to use a small amount of real traffic to verify the new function, but the real traffic may be huge, which puts great pressure on the simulation service.

@spacewander
Copy link
Member

Could you give some use cases about this feature?
okay~
In the simulation environment, we hope to use a small amount of real traffic to verify the new function, but the real traffic may be huge, which puts great pressure on the simulation service.

Personally, I prefer to sample randomly than sample the first N. As,

  1. sample randomly is more accurate. It doesn't miss requests after the first N
  2. we don't need a place to store the count.

What about your opinion?
@membphis @tokers @tzssangglass

@okaybase
Copy link
Member Author

okaybase commented Sep 2, 2021

Could you give some use cases about this feature?
okay~
In the simulation environment, we hope to use a small amount of real traffic to verify the new function, but the real traffic may be huge, which puts great pressure on the simulation service.

Personally, I prefer to sample randomly than sample the first N. As,

  1. sample randomly is more accurate. It doesn't miss requests after the first N
  2. we don't need a place to store the count.

What about your opinion?
@membphis @tokers @tzssangglass

Thanks for your careful review and suggestions firstly~ 😄
Will use random sampling percentages to achieve this function.

@tzssangglass
Copy link
Member

tzssangglass commented Sep 2, 2021

random +1

@tokers
Copy link
Contributor

tokers commented Sep 2, 2021

Could you give some use cases about this feature?
okay~
In the simulation environment, we hope to use a small amount of real traffic to verify the new function, but the real traffic may be huge, which puts great pressure on the simulation service.

Personally, I prefer to sample randomly than sample the first N. As,

  1. sample randomly is more accurate. It doesn't miss requests after the first N
  2. we don't need a place to store the count.

What about your opinion?
@membphis @tokers @tzssangglass

I think a random sample is better than the current way. Also, if we support these rate-limiting ways, we have to change multiple plugins in the future if new rate-limiting algorithms are introduced, and it's prone to be inconsistent on the configurations items.

@okaybase okaybase changed the title feat(proxy-mirror): support mirror requests quota feat(proxy-mirror): support mirror percentage Sep 2, 2021
@okaybase
Copy link
Member Author

okaybase commented Sep 2, 2021

Will use random sampling percentages to achieve this function~ @spacewander @tokers @tzssangglass

@okaybase okaybase changed the title feat(proxy-mirror): support mirror percentage feat(proxy-mirror): support mirror requests percentage Sep 2, 2021
@okaybase okaybase requested a review from tokers September 3, 2021 01:51
apisix/plugins/proxy-mirror.lua Outdated Show resolved Hide resolved
apisix/plugins/proxy-mirror.lua Outdated Show resolved Hide resolved
Co-authored-by: tzssangglass <tzssangglass@gmail.com>
@okaybase okaybase requested a review from tokers September 7, 2021 01:23
apisix/plugins/proxy-mirror.lua Outdated Show resolved Hide resolved
t/plugin/proxy-mirror.t Outdated Show resolved Hide resolved
@spacewander
Copy link
Member

Try this patch?

diff --git t/plugin/proxy-mirror.t t/plugin/proxy-mirror.t
index 888e9a86..a6b7e3ca 100644
--- t/plugin/proxy-mirror.t
+++ t/plugin/proxy-mirror.t
@@ -38,7 +38,7 @@ add_block_preprocessor(sub {
                 local http = require("resty.http")

                 local httpc = http.new()
-                local url = "http://127.0.0.1:1984/stat_count?action=inc"
+                local url = "http://127.0.0.1:1980/stat_count?action=inc"
                 local res, err = httpc:request_uri(url, {method = "GET"})
                 if not res then
                     core.log.error(err)
@@ -588,46 +588,14 @@ passed



-=== TEST 17: set route(stat_count)
---- config
-       location /t {
-           content_by_lua_block {
-               local t = require("lib.test_admin").test
-               local code, body = t('/apisix/admin/routes/2',
-                    ngx.HTTP_PUT,
-                    [[{
-                        "upstream": {
-                            "nodes": {
-                                "127.0.0.1:1980": 1
-                            },
-                            "type": "roundrobin"
-                        },
-                        "uri": "/stat_count"
-                   }]]
-                   )
-
-               if code >= 300 then
-                   ngx.status = code
-               end
-               ngx.say(body)
-           }
-       }
---- request
-GET /t
---- error_code: 200
---- response_body
-passed
-
-
-
-=== TEST 18: send batch requests and get mirror stat count
+=== TEST 17: send batch requests and get mirror stat count
 --- config
        location /t {
            content_by_lua_block {
                 local t = require("lib.test_admin").test

                 -- reset count
-                local code, count = t('/stat_count?action=reset', ngx.HTTP_GET)
+                local code, _, count = t('/stat_count?action=reset', ngx.HTTP_GET)
                 if code >= 300 then
                    ngx.status = code
                 end
@@ -645,10 +613,12 @@ passed
                 end

                 -- get mirror stat count
-                code, count = t('/stat_count', ngx.HTTP_GET)
+                local code, _, count = t('/stat_count', ngx.HTTP_GET)
                 if code >= 300 then
                    ngx.status = code
+                   return
                 end
+                count = tonumber(count)

                 assert(count >= 75 and count <= 125, "mirror request count not in [75, 125]")
            }
@@ -657,4 +627,3 @@ passed
 GET /t
 --- error_log_like eval
 qr/(uri: \/hello\?sample_ratio=0\.5){75,125}/
---- timeout: 60

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@spacewander
Copy link
Member

Since the patch is so big, I decide to push it to this branch instead (also trigger the CI)

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
This reverts commit d5276c1.
According to the error.log, it seems that the mirror request may not
finish when the HTTP request is done. So just check it in the error log

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@spacewander
Copy link
Member

According to the error.log, it seems that the mirror request may not
finish when the HTTP request is done, if the CPU resource is low. So just roll back to check it in the error log

@spacewander spacewander merged commit 63cfcbd into apache:master Sep 10, 2021
@okaybase okaybase deleted the feat-n branch September 10, 2021 15:54
@okaybase
Copy link
Member Author

According to the error.log, it seems that the mirror request may not
finish when the HTTP request is done, if the CPU resource is low. So just roll back to check it in the error log

okay~ thanks a lot~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request help: about proxy-mirror plugin
5 participants