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 2021/05/25 13:40:00 UTC

[GitHub] [skywalking-nginx-lua] yxudong opened a new pull request #85: add flag to makesure Tracer:finish called

yxudong opened a new pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85


   Fix about this issue.
   
   https://github.com/apache/apisix/issues/4301
   
   Add a flag to check whether the `Tracer:finish()` was called in the `body_by_lua_filter*` phase, if not, calling it in `log_by_lua*` phase.
   
   


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850994998


   > > Also, e2e setting should be updated, https://github.com/apache/skywalking-nginx-lua/blob/master/test/e2e/e2e-test/nginx/docker/conf.d/nginx.conf
   > 
   > The sample usage does not need to be modified with add a flag in source code.
   > If so, this file seems don't need to update it.
   
   Make sense. Forgot we changed the 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] [skywalking-nginx-lua] wu-sheng commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r641168873



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)
         ngx.ctx.exitSpan = nil
+        ngx.ctx.is_finished = true
     end
 end
 
 function Tracer:prepareForReport()
+    if not ngx.ctx.is_finished then
+        self.finish()

Review comment:
       If timeout, what is the value?




-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-847883120


   0.6.0 changelog should be updated, https://github.com/apache/skywalking-nginx-lua/blob/master/CHANGES.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] [skywalking-nginx-lua] yxudong edited a comment on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong edited a comment on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850070143


   > https://github.com/apache/skywalking-nginx-lua/blob/master/CHANGES.md This should be updated, 0.6.0 should be added.
   
   OK, I'm plan to update the doc and changelog after this PR merged.


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-847897967


   We need you to update doc and e2e nginx.config to verify the new usage of API.


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r638800798



##########
File path: lib/skywalking/tracer.lua
##########
@@ -91,9 +92,13 @@ function Tracer:finish()
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)
         ngx.ctx.exitSpan = nil
     end
+    ngx.ctx.is_called_finish = true

Review comment:
       Why not check this in the `#finish` method?




-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-847882552


   Are you going to update the document in another PR?


-- 
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] yxudong commented on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-847897752


   OK, I'll update the document in another PR.


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850073667


   > OK, I'm plan to update the doc and changelog after this PR merged.
   
   It should be done in the same PR.


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850068304


   https://github.com/apache/skywalking-nginx-lua/blob/master/CHANGES.md This should be updated, 0.6.0 should be 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] [skywalking-nginx-lua] yxudong commented on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850074138


   > > OK, I'm plan to update the doc and changelog after this PR merged.
   > 
   > It should be done in the same PR.
   
   OK, I'll do it 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] [skywalking-nginx-lua] wu-sheng commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r638837216



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)

Review comment:
       According to https://github.com/apache/apisix/issues/4301#issuecomment-847912259, we need to update the timestamp of the exit span if `#finish` method is called again.




-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r638865243



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)

Review comment:
       Yes, but you are going to call finish in the `header` first, then, the duration doesn't include the time of receiving body. Right? We need a way to update the end time as accurate 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



[GitHub] [skywalking-nginx-lua] yxudong commented on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850982456


   > Also, e2e setting should be updated, https://github.com/apache/skywalking-nginx-lua/blob/master/test/e2e/e2e-test/nginx/docker/conf.d/nginx.conf
   
   The sample usage does not need to be modified with add a flag in source code.
   If so, this file seems don't need to update 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] yxudong commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r641173378



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)
         ngx.ctx.exitSpan = nil
+        ngx.ctx.is_finished = true
     end
 end
 
 function Tracer:prepareForReport()
+    if not ngx.ctx.is_finished then
+        self.finish()

Review comment:
       > If timeout, what is the value?
   
   It's 504 in my example case.




-- 
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] yxudong edited a comment on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong edited a comment on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-847897752


   OK, I'll update the document in another PR after merge.


-- 
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] yxudong commented on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-847899443


   > We need you to update doc and e2e nginx.config to verify the new usage of API.
   
   OK, I'll do 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] yxudong commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r641159342



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)

Review comment:
       Sorry for reply later.
   
   With the way of add a flag to makesure `#finish` method will be called, it's no need to call finish in the `header_filter_by_lua` phase. Just use this project according to the origin doc.
   
   I rearranged the logic. There is two way to fix this bug.
   1. Add a flag to makesure `#finish` method must will be called. If `#finish` method not be called in `body_filter_by_lua` phase, it's will be called in `log_by_lua` phase. It's the way I take now.
   2. Call `#finish` method in the `header_filter_by_lua` phase first. Then call `#finish` method in the `body_filter_by_lua` phase after that, updating the timestamp of the exit span at the same time. With this way there is no need to add flag, because `header_filter_by_lua` phase will always be execcute. But this will change a lot of the code.
   
   The way 1 is the simplest way to fix this issue, and no serious problems will arise. So I take way 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] [skywalking-nginx-lua] wu-sheng commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r641164391



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)
         ngx.ctx.exitSpan = nil
+        ngx.ctx.is_finished = true
     end
 end
 
 function Tracer:prepareForReport()
+    if not ngx.ctx.is_finished then
+        self.finish()

Review comment:
       Have you tested here about whether `ngx.var.upstream_status` is available here? If not, what it says?




-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r641163531



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)

Review comment:
       Make sense to me.




-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85


   


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r638800378



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,6 +79,7 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_called_finish = false

Review comment:
       ```suggestion
       ctx.is_finished = false
   ```




-- 
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] yxudong commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r638845885



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)

Review comment:
       > the timestamp of the exit span
   
   I see the timestamp of the exit span is real-time update when `#finish` method is called at L92.
   Do you mean this?




-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850069511


   @dmsolr @gxthrj Could you verify the codes?


-- 
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] yxudong commented on pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850070143


   > https://github.com/apache/skywalking-nginx-lua/blob/master/CHANGES.md This should be updated, 0.6.0 should be added.
   
   OK, I'm plan to update the doc and changelog after this pr merged.


-- 
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 #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#issuecomment-850823907


   Also, e2e setting should be updated, https://github.com/apache/skywalking-nginx-lua/blob/master/test/e2e/e2e-test/nginx/docker/conf.d/nginx.conf


-- 
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] yxudong commented on a change in pull request #85: add flag to makesure Tracer:finish called

Posted by GitBox <gi...@apache.org>.
yxudong commented on a change in pull request #85:
URL: https://github.com/apache/skywalking-nginx-lua/pull/85#discussion_r641168015



##########
File path: lib/skywalking/tracer.lua
##########
@@ -79,21 +79,26 @@ function Tracer:start(upstream_name, correlation)
     ctx.tracingContext = tracingContext
     ctx.entrySpan = entrySpan
     ctx.exitSpan = exitSpan
+    ctx.is_finished = false
 end
 
 function Tracer:finish()
     -- Finish the exit span when received the first response package from upstream
-    if ngx.ctx.exitSpan ~= nil then
+    if ngx.ctx.exitSpan ~= nil and not ngx.ctx.is_finished then
         local upstream_status = tonumber(ngx.var.upstream_status)
         if upstream_status then
             Span.tag(ngx.ctx.exitSpan, 'http.status', upstream_status)
         end
         Span.finish(ngx.ctx.exitSpan, ngx.now() * 1000)
         ngx.ctx.exitSpan = nil
+        ngx.ctx.is_finished = true
     end
 end
 
 function Tracer:prepareForReport()
+    if not ngx.ctx.is_finished then
+        self.finish()

Review comment:
       > Have you tested here about whether `ngx.var.upstream_status` is available here? If not, what it says?
   
   Yes, `ngx.var.upstream_status` is available at there. I have tested.




-- 
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