You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/11/22 10:59:23 UTC

[GitHub] [apisix] Firstsawyou opened a new pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Firstsawyou opened a new pull request #2817:
URL: https://github.com/apache/apisix/pull/2817


   close #2501
   
   ### What this PR does / why we need it:
   <!--- Why is this change required? What problem does it solve? -->
   <!--- If it fixes an open issue, please link to the issue here. -->
   
   ### Pre-submission checklist:
   
   * [x] Did you explain what problem does this PR solve? Or what new features have been added?
   * [x] Have you added corresponding test cases?
   * [ ] Have you modified the corresponding document?
   * [x] Is this PR backward compatible?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528345739



##########
File path: t/lib/server.lua
##########
@@ -365,4 +365,8 @@ function _M.log()
 end
 
 
+function _M.server_error()
+    error("500 Internal Server Error")
+end
+

Review comment:
       done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530809012



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--

Review comment:
       updated.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r529097971



##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,373 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin",
+                            "timeout": {
+                                "connect": 0.5,
+                                "send": 0.5,
+                                "read": 0.5
+                            }
+                        },
+                        "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-APISIX-Upstream-Status: 504 
+
+
+
+=== TEST 5: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-APISIX-Upstream-Status: 502
+
+
+
+=== TEST 7: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/server_error"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 8: hit routes(500 Internal Server Error)
+--- request
+GET /server_error
+--- error_code: 500
+--- response_body eval
+qr/>500 Internal Server Error/
+--- error_log
+X-APISIX-Upstream-Status: 500
+
+
+
+=== TEST 9: set route(id: 1), and bind the upstream_id
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "uri": "/hello",
+                        "upstream_id": "1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 10: set upstream(id: 1, retries = 2), has available upstream
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1980": 1
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 11: hit routes, status code is 200
+--- request
+GET /hello
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 12: set upstream(id: 1, retries = 2), all upstream nodes are unavailable
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1": 1
+
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 13: hit routes, retry between upstream failed(TODO: The `X-APISIX-Upstream-Status: 502` response header should be added, but the value obtained by $upstream_status is nil.)

Review comment:
       Better to solve the problem instead of adding such `TODO` note.
   We should check the expected result, not the actual one.

##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,373 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin",
+                            "timeout": {
+                                "connect": 0.5,
+                                "send": 0.5,
+                                "read": 0.5
+                            }
+                        },
+                        "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-APISIX-Upstream-Status: 504 
+
+
+
+=== TEST 5: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-APISIX-Upstream-Status: 502
+
+
+
+=== TEST 7: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/server_error"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 8: hit routes(500 Internal Server Error)
+--- request
+GET /server_error
+--- error_code: 500
+--- response_body eval
+qr/>500 Internal Server Error/
+--- error_log
+X-APISIX-Upstream-Status: 500
+
+
+
+=== TEST 9: set route(id: 1), and bind the upstream_id
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "uri": "/hello",
+                        "upstream_id": "1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 10: set upstream(id: 1, retries = 2), has available upstream
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1980": 1
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 11: hit routes, status code is 200
+--- request
+GET /hello
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 12: set upstream(id: 1, retries = 2), all upstream nodes are unavailable
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1": 1
+
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 13: hit routes, retry between upstream failed(TODO: The `X-APISIX-Upstream-Status: 502` response header should be added, but the value obtained by $upstream_status is nil.)
+--- request
+GET /hello
+--- error_code: 502
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status: 502/
+--- grep_error_log_out
+
+
+
+=== TEST 14: the status code returned from apisix
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                ngx.HTTP_PUT,
+                [[{
+                        "plugins": {
+                            "fault-injection": {
+                                "abort": {
+                                    "http_status": 500,
+                                    "body": "Fault Injection!\n"
+                                }
+                            }
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 15: hit routes, retries failed, status code is 502
+--- request
+GET /hello
+--- error_code: 500

Review comment:
       The title says it is 502?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528460268



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       This is still unresolved.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530039191



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")
+    if from then
+        core.response.set_header("X-APISIX-Upstream-Status", up_status)

Review comment:
       We should find a place to document this feature.

##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       We should only care about the last status, not any one happened in retry.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530777169



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,13 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}$", "jo")

Review comment:
       Yes i will update later.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] membphis commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531076377



##########
File path: doc/debugging.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/debugging.md)

Review comment:
       add a reference to `doc/README.md`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528636800



##########
File path: apisix/init.lua
##########
@@ -630,7 +630,7 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("status"))
+    local status_code = tonumber(get_var("upstream_status"))
     if status_code then

Review comment:
       What about `if status_code and status_code >= 500 and status_code <= 599`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530808592



##########
File path: doc/zh-cn/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[English](../upstream-status-5xx.md)
+
+## `5xx` 响应状态码
+
+500、502、503等类似的 `5xx` 状态码,是由于服务器错误而响应的状态码,当一个请求出现 `5xx` 状态码时;它可能来源于 `APISIX` 或 `Upstream` 。如何识别这些响应状态码的来源,是一件很有意义的事,它能够快速的帮助我们确定确定问题的所在。

Review comment:
       fixed.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528718641



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,14 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code then
+        if status_code >= 500 and status_code <= 599 then

Review comment:
       The main purpose here is to distinguish whether the 5xx status code comes from APISIX or upstream. If we want to record other response status codes, we need more discussion.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531099546



##########
File path: doc/debugging.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/debugging.md)

Review comment:
       added.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530778235



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--

Review comment:
       Agree, we need a better title. Do you have any good suggestions for the title of the document.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530465858



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] membphis commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531262634



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,24 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local function set_res_header(up_status)
+        core.response.set_header("X-APISIX-Upstream-Status", up_status)

Review comment:
       bad function name, what means `res`?

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,24 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local function set_res_header(up_status)

Review comment:
       bad style, move this function out

##########
File path: doc/README.md
##########
@@ -35,6 +35,7 @@
 * [Changelog](../CHANGELOG.md)
 * [Benchmark](benchmark.md)
 * [Code Style](../CODE_STYLE.md)
+* [Debug function](debugging.md)

Review comment:
       I do not think `debugging.md` is a good name for this doc




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531069999



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '

Review comment:
       updated.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528436004



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       What about checking the last status ("5xx", or " 5xx")?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] membphis commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530469905



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       we should use the last status, current way is wrong




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530781876



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.

Review comment:
       At present, if there are multiple retries between upstream and all upstream nodes are unavailable, `X-APISIX-Upstream-Status` records all status codes. For example: `$upstream_status` is `502, 500, 502`, then `X-APISIX-Upstream-Status: 502, 500, 502`.
   I think that when multiple retries between upstreams fail, all status codes should be recorded so that we can know the corresponding status codes between upstream nodes, which helps us to know the problems between upstream nodes.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530110797



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       @Firstsawyou 
   Normally Nginx won't retry after 200.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528708247



##########
File path: apisix/init.lua
##########
@@ -630,7 +630,7 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("status"))
+    local status_code = tonumber(get_var("upstream_status"))
     if status_code then

Review comment:
       nice, done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530475471



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       Yes, I have updated it. Help take a look.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] membphis commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530711406



##########
File path: doc/zh-cn/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[English](../upstream-status-5xx.md)
+
+## `5xx` 响应状态码
+
+500、502、503等类似的 `5xx` 状态码,是由于服务器错误而响应的状态码,当一个请求出现 `5xx` 状态码时;它可能来源于 `APISIX` 或 `Upstream` 。如何识别这些响应状态码的来源,是一件很有意义的事,它能够快速的帮助我们确定确定问题的所在。

Review comment:
        Double "确定"

##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,419 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                    "methods": ["GET"],
+                    "upstream": {
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        },
+                        "type": "roundrobin"
+                    },
+                    "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status: 200/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)

Review comment:
        Bad title

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,13 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}$", "jo")

Review comment:
       string.sub (upstatus,  -3)  
   
   Should be simpler

##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,419 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                    "methods": ["GET"],
+                    "upstream": {
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        },
+                        "type": "roundrobin"
+                    },
+                    "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status: 200/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                    "methods": ["GET"],
+                    "upstream": {
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        },
+                        "type": "roundrobin",
+                        "timeout": {
+                            "connect": 0.5,
+                            "send": 0.5,
+                            "read": 0.5
+                        }
+                    },
+                    "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-APISIX-Upstream-Status: 504
+
+
+
+=== TEST 5: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                    "methods": ["GET"],
+                    "upstream": {
+                        "nodes": {
+                            "127.0.0.1:1": 1
+                        },
+                        "type": "roundrobin"
+                    },
+                    "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-APISIX-Upstream-Status: 502
+
+
+
+=== TEST 7: set route(id: 1)

Review comment:
        Bad title too




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528459159



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,14 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("status"))

Review comment:
       We cannot use `$status`, since we don't know whether it's from upstream or by APISIX itself.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander merged pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #2817:
URL: https://github.com/apache/apisix/pull/2817


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528321335



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       Note that `upstream_status` could be `500, 502` or other similar stuff if retry between upstreams is involved.

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then

Review comment:
       Need a space before 599.

##########
File path: t/lib/server.lua
##########
@@ -365,4 +365,8 @@ function _M.log()
 end
 
 
+function _M.server_error()
+    error("500 Internal Server Error")
+end
+

Review comment:
       Need one more blank line

##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,165 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin",
+                            "timeout": {
+                                "connect": 0.5,
+                                "send": 0.5,
+                                "read": 0.5
+                            }
+                        },
+                        "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-Apisix-Upstream-Status:504 
+
+
+
+=== TEST 3: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1880": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-Apisix-Upstream-Status:502
+
+
+
+=== TEST 5: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/server_error"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes(500 Internal Server Error)
+--- request
+GET /server_error
+--- error_code: 500
+--- response_body eval
+qr/>500 Internal Server Error/
+--- error_log
+X-Apisix-Upstream-Status:500

Review comment:
       Need a case to test the status code returned from apisix.

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then
+        core.response.set_header("X-Apisix-Upstream-Status", status_code)

Review comment:
       Would it be better to use `X-APISIX-`? CC @membphis 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528346058



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then

Review comment:
       Good suggestion, updated.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] nic-chen commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528335041



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then
+        core.response.set_header("X-Apisix-Upstream-Status", status_code)

Review comment:
       Agree +1




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531350105



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,24 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local function set_res_header(up_status)
+        core.response.set_header("X-APISIX-Upstream-Status", up_status)

Review comment:
       updated.

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,24 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local function set_res_header(up_status)

Review comment:
       done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r529174101



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       It is because `tonumber("500, 502")` will be `nil`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530808291



##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,419 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                    "methods": ["GET"],
+                    "upstream": {
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        },
+                        "type": "roundrobin"
+                    },
+                    "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-APISIX-Upstream-Status: 200/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)

Review comment:
       updated.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528567641



##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,359 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin",
+                            "timeout": {
+                                "connect": 0.5,
+                                "send": 0.5,
+                                "read": 0.5
+                            }
+                        },
+                        "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-Apisix-Upstream-Status: 504 
+
+
+
+=== TEST 5: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-Apisix-Upstream-Status: 502
+
+
+
+=== TEST 7: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/server_error"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 8: hit routes(500 Internal Server Error)
+--- request
+GET /server_error
+--- error_code: 500
+--- response_body eval
+qr/>500 Internal Server Error/
+--- error_log
+X-Apisix-Upstream-Status: 500
+
+
+
+=== TEST 9: set route(id: 1), and bind the upstream_id
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "uri": "/hello",
+                        "upstream_id": "1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 10: set upstream(id: 1, retries = 2), has available upstream
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1980": 1
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 11: hit routes, status code is 200
+--- request
+GET /hello
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 12: set upstream(id: 1, retries = 2), all upstream nodes are unavailable
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1": 1
+
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 13: hit routes, retries failed, status code is 502
+--- request
+GET /hello
+--- error_code: 502
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status: 502/
+--- grep_error_log_out
+X-Apisix-Upstream-Status: 502
+
+
+
+=== TEST 14: the status code returned from apisix
+--- config
+    location /t {
+        content_by_lua_block {
+            ngx.exit(500)
+        }
+    }
+--- request
+GET /t
+--- error_code: 500
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 15: the status code returned from apisix
+--- config
+    location /t {

Review comment:
       Nice, I will update later.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530746917



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.

Review comment:
       You should note the `X-APISIX-Upstream-Status` is the last status code from upstream if proxy trying for several times.

##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--

Review comment:
       I think we will have a bunch of headers for debugging in the future, whereupon we may need a doc to explain meaning of each header, i'm not sure whether this document title is suitable.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528509391



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       If the upstream retry, the status code returns 500, 502 or other similar status codes. This situation is not returned by apisix itself, this should be the status code of the upstream service. We can capture it with `get_var("status")`. If it is the status code returned by apisix itself, the value captured by `get_var("status")` will be a nil.
   This is my opinion. At present, I have not thought of a better solution.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530808411



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,13 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}$", "jo")

Review comment:
       done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r529126522



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       I think there is a problem. When it comes to retries between upstreams and all upstream services are unavailable, the value of `$upstream_status` is nil. APISIX itself returns a 502 status code. I think this 502 status code should be the status code of the upstream service.
   This is the relevant test case:
   https://github.com/apache/apisix/pull/2817/files#diff-8b1dc592c5a25c17fc62106ed68fd5b910eb584064cba052b1af9088d3e74ff5R286-R326




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530746917



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.

Review comment:
       You should note the `X-APISIX-Upstream-Status` is the last status code from upstream if proxy (re)trying for several times.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530093211



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       @spacewander 
   I want to know if the result of `$upstream_status` will appear `502, 200, 502` ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530809166



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "methods": ["GET"],
+    "upstream": {
+        "nodes": {
+            "127.0.0.1:1": 1
+        },
+        "type": "roundrobin"
+    },
+    "uri": "/hello"
+}'
+```
+
+Test:
+
+```shell
+$ curl http://127.0.0.1:9080/hello -v
+......
+< HTTP/1.1 502 Bad Gateway
+< Date: Wed, 25 Nov 2020 14:40:22 GMT
+< Content-Type: text/html; charset=utf-8
+< Content-Length: 154
+< Connection: keep-alive
+< Server: APISIX/2.0
+< X-APISIX-Upstream-Status: 502
+<
+<html>
+<head><title>502 Bad Gateway</title></head>
+<body>
+<center><h1>502 Bad Gateway</h1></center>
+<hr><center>openresty</center>
+</body>
+</html>
+
+```
+
+It has a response header of `X-APISIX-Upstream-Status: 502`.
+
+>Example 2: `502` response status code comes from `APISIX`
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "plugins": {
+        "fault-injection": {
+            "abort": {
+                "http_status": 500,
+                "body": "Fault Injection!\n"
+            }
+        }
+    },
+    "uri": "/hello"
+}'
+```
+
+测试:

Review comment:
       fix.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] nic-chen commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528638476



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,14 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code then
+        if status_code >= 500 and status_code <= 599 then

Review comment:
       could we always respond upstream status unless it's greater than 200 ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528513903



##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,359 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit the route and status code is 200
+--- request
+GET /hello
+--- response_body
+hello world
+--- no_error_log
+[error]
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 3: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin",
+                            "timeout": {
+                                "connect": 0.5,
+                                "send": 0.5,
+                                "read": 0.5
+                            }
+                        },
+                        "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-Apisix-Upstream-Status: 504 
+
+
+
+=== TEST 5: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-Apisix-Upstream-Status: 502
+
+
+
+=== TEST 7: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/server_error"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 8: hit routes(500 Internal Server Error)
+--- request
+GET /server_error
+--- error_code: 500
+--- response_body eval
+qr/>500 Internal Server Error/
+--- error_log
+X-Apisix-Upstream-Status: 500
+
+
+
+=== TEST 9: set route(id: 1), and bind the upstream_id
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "uri": "/hello",
+                        "upstream_id": "1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 10: set upstream(id: 1, retries = 2), has available upstream
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1980": 1
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 11: hit routes, status code is 200
+--- request
+GET /hello
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 12: set upstream(id: 1, retries = 2), all upstream nodes are unavailable
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "nodes": {
+                        "127.0.0.3:1": 1,
+                        "127.0.0.2:1": 1,
+                        "127.0.0.1:1": 1
+
+                    },
+                    "retries": 2,
+                    "type": "roundrobin"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 13: hit routes, retries failed, status code is 502
+--- request
+GET /hello
+--- error_code: 502
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status: 502/
+--- grep_error_log_out
+X-Apisix-Upstream-Status: 502
+
+
+
+=== TEST 14: the status code returned from apisix
+--- config
+    location /t {
+        content_by_lua_block {
+            ngx.exit(500)
+        }
+    }
+--- request
+GET /t
+--- error_code: 500
+--- grep_error_log eval
+qr/X-Apisix-Upstream-Status:/
+--- grep_error_log_out
+
+
+
+=== TEST 15: the status code returned from apisix
+--- config
+    location /t {

Review comment:
       This route doesn't belong to APISIX. You can confirm it via looking at the generated `t/servroot/conf/nginx.conf`.
   You can use `limit-count` plugin to generate a 5xx from APISIX.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528346159



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then
+        core.response.set_header("X-Apisix-Upstream-Status", status_code)
+        core.log.info("X-Apisix-Upstream-Status:", status_code)

Review comment:
       done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528345712



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then

Review comment:
       nice.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530848742



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '

Review comment:
       Unfortunately, we have to revert the change (9080 -> 9180) soon. See https://lists.apache.org/thread.html/rfb780891c940f4930240570d0170fad154a14d7b582ca60b4eae7524%40%3Cdev.apisix.apache.org%3E




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] spacewander commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530784822



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "methods": ["GET"],
+    "upstream": {
+        "nodes": {
+            "127.0.0.1:1": 1
+        },
+        "type": "roundrobin"
+    },
+    "uri": "/hello"
+}'
+```
+
+Test:
+
+```shell
+$ curl http://127.0.0.1:9080/hello -v
+......
+< HTTP/1.1 502 Bad Gateway
+< Date: Wed, 25 Nov 2020 14:40:22 GMT
+< Content-Type: text/html; charset=utf-8
+< Content-Length: 154
+< Connection: keep-alive
+< Server: APISIX/2.0
+< X-APISIX-Upstream-Status: 502
+<
+<html>
+<head><title>502 Bad Gateway</title></head>
+<body>
+<center><h1>502 Bad Gateway</h1></center>
+<hr><center>openresty</center>
+</body>
+</html>
+
+```
+
+It has a response header of `X-APISIX-Upstream-Status: 502`.
+
+>Example 2: `502` response status code comes from `APISIX`
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "plugins": {
+        "fault-injection": {
+            "abort": {
+                "http_status": 500,
+                "body": "Fault Injection!\n"
+            }
+        }
+    },
+    "uri": "/hello"
+}'
+```
+
+测试:

Review comment:
       Should be English.

##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "methods": ["GET"],
+    "upstream": {
+        "nodes": {
+            "127.0.0.1:1": 1
+        },
+        "type": "roundrobin"
+    },
+    "uri": "/hello"
+}'
+```
+
+Test:
+
+```shell
+$ curl http://127.0.0.1:9080/hello -v
+......
+< HTTP/1.1 502 Bad Gateway
+< Date: Wed, 25 Nov 2020 14:40:22 GMT
+< Content-Type: text/html; charset=utf-8
+< Content-Length: 154
+< Connection: keep-alive
+< Server: APISIX/2.0
+< X-APISIX-Upstream-Status: 502
+<
+<html>
+<head><title>502 Bad Gateway</title></head>
+<body>
+<center><h1>502 Bad Gateway</h1></center>
+<hr><center>openresty</center>
+</body>
+</html>
+
+```
+
+It has a response header of `X-APISIX-Upstream-Status: 502`.
+
+>Example 2: `502` response status code comes from `APISIX`
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "plugins": {
+        "fault-injection": {
+            "abort": {
+                "http_status": 500,
+                "body": "Fault Injection!\n"
+            }
+        }
+    },
+    "uri": "/hello"
+}'
+```
+
+测试:
+
+```shell
+$ curl http://127.0.0.1:9080/hello -v
+......
+< HTTP/1.1 500 Internal Server Error
+< Date: Wed, 25 Nov 2020 14:50:20 GMT
+< Content-Type: text/plain; charset=utf-8
+< Transfer-Encoding: chunked
+< Connection: keep-alive
+< Server: APISIX/2.0
+<
+Fault Injection!
+```
+
+There is no response header for `X-APISIX-Upstream-Status`.
+
+>Example 3: `Upstream` has multiple nodes, and all nodes are unavailable
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/upstreams/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "nodes": {
+        "127.0.0.3:1": 1,
+        "127.0.0.2:1": 1,
+        "127.0.0.1:1": 1
+    },
+    "retries": 2,
+    "type": "roundrobin"
+}'
+```
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '

Review comment:
       ditto

##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+    "methods": ["GET"],
+    "upstream": {
+        "nodes": {
+            "127.0.0.1:1": 1
+        },
+        "type": "roundrobin"
+    },
+    "uri": "/hello"
+}'
+```
+
+Test:
+
+```shell
+$ curl http://127.0.0.1:9080/hello -v
+......
+< HTTP/1.1 502 Bad Gateway
+< Date: Wed, 25 Nov 2020 14:40:22 GMT
+< Content-Type: text/html; charset=utf-8
+< Content-Length: 154
+< Connection: keep-alive
+< Server: APISIX/2.0
+< X-APISIX-Upstream-Status: 502
+<
+<html>
+<head><title>502 Bad Gateway</title></head>
+<body>
+<center><h1>502 Bad Gateway</h1></center>
+<hr><center>openresty</center>
+</body>
+</html>
+
+```
+
+It has a response header of `X-APISIX-Upstream-Status: 502`.
+
+>Example 2: `502` response status code comes from `APISIX`
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '

Review comment:
       ditto

##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '

Review comment:
       Should use port 9080.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530795262



##########
File path: doc/upstream-status-5xx.md
##########
@@ -0,0 +1,151 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+[Chinese](zh-cn/upstream-status-5xx.md)
+
+## `5xx` response status code
+
+Similar `5xx` status codes such as 500, 502, 503, etc., are the status codes in response to a server error. When a request has a `5xx` status code; it may come from `APISIX` or `Upstream`. How to identify the source of these response status codes is a very meaningful thing. It can quickly help us determine the problem.
+
+## How to identify the source of the `5xx` response status code
+
+In the response header of the request, through the response header of `X-APISIX-Upstream-Status`, we can effectively identify the source of the `5xx` status code. When the `5xx` status code comes from `Upstream`, the response header `X-APISIX-Upstream-Status` can be seen in the response header, and the value of this response header is the response status code. When the `5xx` status code is derived from `APISIX`, there is no response header information of `X-APISIX-Upstream-Status` in the response header. That is, only when the status code of `5xx` is derived from Upstream will the `X-APISIX-Upstream-Status` response header appear.
+
+## Example
+
+>Example 1: `502` response status code comes from `Upstream` (IP address is not available)
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '

Review comment:
       In other documents, the port of `admin api` has been changed to `9180`, I think it needs to be consistent here too, right?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528331295



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then
+        core.response.set_header("X-Apisix-Upstream-Status", status_code)

Review comment:
       Agree for APISIX

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then
+        core.response.set_header("X-Apisix-Upstream-Status", status_code)
+        core.log.info("X-Apisix-Upstream-Status:", status_code)

Review comment:
       What about adding a space after `:`.

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then

Review comment:
       Not all requests will be proxied to upstream, requests terminated by APISIX itself will have null value of `$upstream_status`, we have to check the validity of `status_code`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530091095



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")

Review comment:
       Agree, I will update later.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528351108



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       How to deal with this situation, do you have any good suggestions?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528440335



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       Yes this is a good way.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r530466136



##########
File path: apisix/init.lua
##########
@@ -630,10 +630,11 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
-    local status_code = tonumber(get_var("upstream_status"))
-    if status_code and status_code >= 500 and status_code <= 599 then
-        core.response.set_header("X-APISIX-Upstream-Status", status_code)
-        core.log.info("X-APISIX-Upstream-Status: ", status_code)
+    local up_status = get_var("upstream_status")
+    local from, _ = re_find(up_status, "5[0-9]{2}", "jo")
+    if from then
+        core.response.set_header("X-APISIX-Upstream-Status", up_status)

Review comment:
       done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] membphis commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528739086



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code and status_code >= 500 and status_code <= 599 then
+        core.response.set_header("X-APISIX-Upstream-Status", status_code)

Review comment:
       How about `X-Upstream-Status`?

##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       `get_var("upstream_status")`, is this wrong? 
   please confirm do we need to fix it?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531302745



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,24 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local function set_res_header(up_status)
+        core.response.set_header("X-APISIX-Upstream-Status", up_status)

Review comment:
       `res` means `response`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] membphis commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531328446



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,24 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local function set_res_header(up_status)
+        core.response.set_header("X-APISIX-Upstream-Status", up_status)

Review comment:
       we should use `resp` here




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r531350211



##########
File path: doc/README.md
##########
@@ -35,6 +35,7 @@
 * [Changelog](../CHANGELOG.md)
 * [Benchmark](benchmark.md)
 * [Code Style](../CODE_STYLE.md)
+* [Debug function](debugging.md)

Review comment:
       updated.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r529131022



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code and status_code >= 500 and status_code <= 599 then
+        core.response.set_header("X-APISIX-Upstream-Status", status_code)

Review comment:
       > I think the `X-APISIX-` prefix is required to distinguish headers source, it's confused if backend also carries headers with `X-Upstream-` prefix.
   
   agree +1




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r529218125



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))

Review comment:
       I understand, thank you very much. I will update later.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528332137



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code >= 500 and status_code <=599 then

Review comment:
       BTW, the corresponding test cases should be replenished.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] Firstsawyou commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528345888



##########
File path: t/node/upstream-status-5xx.t
##########
@@ -0,0 +1,165 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin",
+                            "timeout": {
+                                "connect": 0.5,
+                                "send": 0.5,
+                                "read": 0.5
+                            }
+                        },
+                        "uri": "/sleep1"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: hit routes (timeout)
+--- request
+GET /sleep1
+--- error_code: 504
+--- response_body eval
+qr/504 Gateway Time-out/
+--- error_log
+X-Apisix-Upstream-Status:504 
+
+
+
+=== TEST 3: set route(id: 1), upstream service is not available
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1880": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: hit routes (502 Bad Gateway)
+--- request
+GET /hello
+--- error_code: 502
+--- response_body eval
+qr/502 Bad Gateway/
+--- error_log
+X-Apisix-Upstream-Status:502
+
+
+
+=== TEST 5: set route(id: 1)
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "methods": ["GET"],
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/server_error"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: hit routes(500 Internal Server Error)
+--- request
+GET /server_error
+--- error_code: 500
+--- response_body eval
+qr/>500 Internal Server Error/
+--- error_log
+X-Apisix-Upstream-Status:500

Review comment:
       nice, added.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [apisix] tokers commented on a change in pull request #2817: feat: How to distinguish whether the 5xx status code (eg 500) comes from upstream or apisix

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2817:
URL: https://github.com/apache/apisix/pull/2817#discussion_r528744566



##########
File path: apisix/init.lua
##########
@@ -629,6 +630,12 @@ end
 function _M.http_header_filter_phase()
     core.response.set_header("Server", ver_header)
 
+    local status_code = tonumber(get_var("upstream_status"))
+    if status_code and status_code >= 500 and status_code <= 599 then
+        core.response.set_header("X-APISIX-Upstream-Status", status_code)

Review comment:
       I think the `X-APISIX-` prefix is required to distinguish headers source, it's confused if backend also carries headers with `X-Upstream-` prefix.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org