You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/09/24 02:01:14 UTC
[GitHub] [skywalking-nginx-lua] Jijun opened a new pull request #42: add error check, status code great or equal than 500 would be in erro…
Jijun opened a new pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42
add error condition check , so we can filter error at skywalking ui. some java plugins make error occured when status code great or equal than 400, but at http side 4xx should be "client error"
so i add error tag when status code great or equal than 500
----------------------------------------------------------------
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] [skywalking-nginx-lua] dmsolr commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
dmsolr commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-699574290
In my mind, the Span ERROR means Nginx has some wrong or upstream returns an unsuccess(include 4xx and 5xx). I think that will make confusion to others.
Now, Tag is searchable. So, I think to make a new Tag to represent client_error(4xx), server_error(5xx). That will be better.
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng commented on a change in pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#discussion_r495518659
##########
File path: lib/skywalking/tracer.lua
##########
@@ -68,6 +68,10 @@ end
function Tracer:finish()
-- Finish the exit span when received the first response package from upstream
if ngx.ctx.exitSpan ~= nil then
+ local upstream_status = tonumber(ngx.var.upstream_status)
+ if upstream_status and upstream_status >= 500 then
+ Span.errorOccurred(ngx.ctx.exitSpan)
Review comment:
@Jijun
----------------------------------------------------------------
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] [skywalking-nginx-lua] dmsolr commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
dmsolr commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698828749
It is basically good to me. But I wonder if this will lead to some misunderstanding.
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng merged pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698912080
> It is basically good to me. But I wonder if this will lead to some misunderstanding.
What is your concern? Could you be clear?
----------------------------------------------------------------
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] [skywalking-nginx-lua] dmsolr commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
dmsolr commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-699575365
Got 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] [skywalking-nginx-lua] wu-sheng commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698067976
Please recheck your codes, the e2e test fails.
----------------------------------------------------------------
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] [skywalking-nginx-lua] Jijun commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
Jijun commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698706522
it's my fault, i forget to remove duplicate line about http.status tag.
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng commented on a change in pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#discussion_r495822580
##########
File path: lib/skywalking/tracer.lua
##########
@@ -77,7 +81,11 @@ function Tracer:prepareForReport()
local TC = require('tracing_context')
local Segment = require('segment')
if ngx.ctx.entrySpan ~= nil then
- Span.tag(ngx.ctx.entrySpan, 'http.status', ngx.var.status)
+ local ngxstatus = ngx.var.status
+ Span.tag(ngx.ctx.entrySpan, 'http.status', ngxstatus)
+ if tonumber(ngxstatus) >= 500 then
+ Span.errorOccurred(ngx.ctx.entrySpan)
+ end
Review comment:
Why do this again in the reporting stage? I think all logic should be included in `Tracer:finish`. Could you explain?
----------------------------------------------------------------
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] [skywalking-nginx-lua] dmsolr commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
dmsolr commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698828749
It is basically good to me. But I wonder if this will lead to some misunderstanding.
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-699574888
tag=`status_code` is already available at the core level and other agents. We could add it here.
Also, this logic is widely used in the Java agent too, take a look at this, https://github.com/apache/skywalking/blob/master/apm-sniffer/apm-sdk-plugin/httpClient-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpClient/v4/HttpClientExecuteInterceptor.java#L84-L87. You could find many codes like this. They even use `>=400`, rather than `>=500`.
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698912080
> It is basically good to me. But I wonder if this will lead to some misunderstanding.
What is your concern? Could you be clear?
----------------------------------------------------------------
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] [skywalking-nginx-lua] wu-sheng commented on a change in pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#discussion_r495518643
##########
File path: lib/skywalking/tracer.lua
##########
@@ -68,6 +68,10 @@ end
function Tracer:finish()
-- Finish the exit span when received the first response package from upstream
if ngx.ctx.exitSpan ~= nil then
+ local upstream_status = tonumber(ngx.var.upstream_status)
+ if upstream_status and upstream_status >= 500 then
+ Span.errorOccurred(ngx.ctx.exitSpan)
Review comment:
According to the discussion, I suggest you add `status_code` tag 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] [skywalking-nginx-lua] wu-sheng commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-700414229
@Jijun Please update main repo lua e2e with the latest master commit id?
----------------------------------------------------------------
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] [skywalking-nginx-lua] Jijun commented on pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
Jijun commented on pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#issuecomment-698706522
it's my fault, i forget to remove duplicate line about http.status tag.
----------------------------------------------------------------
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] [skywalking-nginx-lua] Jijun commented on a change in pull request #42: add error check, status code great or equal than 500 would be in erro…
Posted by GitBox <gi...@apache.org>.
Jijun commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-nginx-lua/pull/42#discussion_r496353731
##########
File path: lib/skywalking/tracer.lua
##########
@@ -77,7 +81,11 @@ function Tracer:prepareForReport()
local TC = require('tracing_context')
local Segment = require('segment')
if ngx.ctx.entrySpan ~= nil then
- Span.tag(ngx.ctx.entrySpan, 'http.status', ngx.var.status)
+ local ngxstatus = ngx.var.status
+ Span.tag(ngx.ctx.entrySpan, 'http.status', ngxstatus)
+ if tonumber(ngxstatus) >= 500 then
+ Span.errorOccurred(ngx.ctx.entrySpan)
+ end
Review comment:
i follow the origin author's logic and made minimal changes as far as possible.
----------------------------------------------------------------
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