resolve comments

Signed-off-by: Nic <qianyong@api7.ai>

Author: nic-6443

apisix/init.lua

@@ -220,21 +220,23 @@ function _M.ssl_client_hello_phase()
             core.log.error("failed to fetch ssl config: ", err)
         end
         core.log.error("failed to match any SSL certificate by SNI: ", sni)
-        tracer.finish(ngx_ctx, span, tracer.status.ERROR, "failed match SNI")
+        span:set_status(tracer.status.ERROR, "no matched SSL")
+        span:finish(ngx_ctx)
         ngx_exit(-1)
     end
 
     ok, err = apisix_ssl.set_protocols_by_clienthello(ngx_ctx.matched_ssl.value.ssl_protocols)
     if not ok then
         core.log.error("failed to set ssl protocols: ", err)
-        tracer.finish(ngx_ctx, span, tracer.status.ERROR, "failed set protocols")
+        span:set_status(tracer.status.ERROR, "failed set protocols")
+        span:finish(ngx_ctx)
         ngx_exit(-1)
     end
 
     -- in stream subsystem, ngx.ssl.server_name() return hostname of ssl session in preread phase,
     -- so that we can't get real SNI without recording it in ngx.ctx during client_hello phase
     ngx.ctx.client_hello_sni = sni
-    tracer.finish(ngx_ctx, span)
+    span:finish(ngx_ctx)
 end
 
 
@@ -732,7 +734,8 @@ function _M.http_access_phase()
 
     local route = api_ctx.matched_route
     if not route then
-        tracer.finish(ngx.ctx, match_span, tracer.status.ERROR, "no matched route")
+        match_span:set_status(tracer.status.ERROR, "no matched route")
+        match_span:finish(ngx.ctx)
         -- run global rule when there is no matching route
         local global_rules, conf_version = apisix_global_rules.global_rules()
         plugin.run_global_rules(api_ctx, global_rules, conf_version, nil)
@@ -741,7 +744,7 @@ function _M.http_access_phase()
         return core.response.exit(404,
                     {error_msg = "404 Route Not Found"})
     end
-    tracer.finish(ngx_ctx, match_span)
+    match_span:finish(ngx_ctx)
 
     core.log.info("matched route: ",
                   core.json.delay_encode(api_ctx.matched_route, true))
@@ -833,7 +836,7 @@ function _M.http_access_phase()
         end
         plugin.run_plugin("access", plugins, api_ctx)
     end
-    tracer.finish(ngx_ctx, span)
+    span:finish(ngx_ctx)
 
     _M.handle_upstream(api_ctx, route, enable_websocket)
 
@@ -916,7 +919,7 @@ function _M.http_header_filter_phase()
         end
         core.response.set_header("Apisix-Plugins", core.table.concat(deduplicate, ", "))
     end
-    tracer.finish(ngx_ctx, span)
+    span:finish(ngx_ctx)
 
     tracer.start(ngx_ctx, "apisix.phase.body_filter", tracer.kind.server)
 end

apisix/plugin.lua

@@ -1232,7 +1232,7 @@ function _M.run_plugin(phase, plugins, api_ctx)
             local span = tracer.start(api_ctx.ngx_ctx, "apisix.phase." .. phase
                                         .. ".plugins." .. api_ctx._plugin_name)
             phase_func(conf, api_ctx)
-            tracer.finish(api_ctx.ngx_ctx, span)
+            span:finish(api_ctx.ngx_ctx)
             api_ctx._plugin_name = nil
         end
     end
@@ -1340,7 +1340,7 @@ function _M.run_global_rules(api_ctx, global_rules, conf_version, phase_name)
         api_ctx.conf_type = orig_conf_type
         api_ctx.conf_version = orig_conf_version
         api_ctx.conf_id = orig_conf_id
-        tracer.finish(api_ctx.ngx_ctx, span)
+        span:finish(api_ctx.ngx_ctx)
     end
 end
 

apisix/secret.lua

@@ -158,11 +158,12 @@ local function fetch_by_uri_secret(secret_uri)
 
     local value, err = sm.get(conf, opts.key)
     if err then
-        tracer.finish(ngx.ctx, span, tracer.status.ERROR, err)
+        span:set_status(tracer.status.ERROR, err)
+        span:finish(ngx.ctx)
         return nil, err
     end
 
-    tracer.finish(ngx.ctx, span)
+    span:finish(ngx.ctx)
     return value
 end
 

apisix/ssl/router/radixtree_sni.lua

@@ -179,10 +179,11 @@ function _M.match_and_set(api_ctx, match_only, alt_sni)
             -- with it sometimes
             core.log.error("failed to find any SSL certificate by SNI: ", sni)
         end
-        tracer.finish(api_ctx.ngx_ctx, span, tracer.status.ERROR, "failed match SNI")
+        span:set_status(tracer.status.ERROR, "failed match SNI")
+        span:finish(api_ctx.ngx_ctx)
         return false
     end
-    tracer.finish(api_ctx.ngx_ctx, span)
+    span:finish(api_ctx.ngx_ctx)
 
     if api_ctx.matched_sni == "*" then
         -- wildcard matches everything, no need for further validation

apisix/tracer.lua

@@ -14,9 +14,9 @@
 -- See the License for the specific language governing permissions and
 -- limitations under the License.
 --
-local table = require("apisix.core.table")
 local tablepool = require("tablepool")
 local span = require("apisix.utils.span")
+local noop_span = require("apisix.utils.noop_span").new()
 local span_kind = require("opentelemetry.trace.span_kind")
 local span_status = require("opentelemetry.trace.span_status")
 local local_conf = require("apisix.core.config_local").local_conf()
@@ -36,75 +36,42 @@ local _M = {
 
 function _M.start(ctx, name, kind)
     if not enable_tracing then
-        return
+        return noop_span
     end
 
-    local tracing = ctx and ctx.tracing
+    local tracing = ctx.tracing
     if not tracing then
-        local root_span = span.new()
         tracing = tablepool.fetch("tracing", 0, 8)
         tracing.spans = tablepool.fetch("tracing_spans", 20, 0)
-        tracing.root_span = root_span
-        tracing.current_span = root_span
-        table.insert(tracing.spans, root_span)
-        root_span.id = 1
         ctx.tracing = tracing
     end
     if tracing.skip then
-        return
+        return noop_span
     end
 
-    local spans = tracing.spans
-    local sp = span.new(name, kind)
-
-    table.insert(spans, sp)
-    local id = #spans
-    sp.id = id
-    local parent = tracing.current_span
-    if parent then
-        sp:set_parent(parent.id)
-        parent:append_child(id)
-    end
-    tracing.current_span = sp
+    local sp = span.new(ctx, name, kind)
     return sp
 end
 
 
-local function finish_span(spans, sp, code, message)
-    if not sp or sp.end_time then
-        return
-    end
-    for _, id in ipairs(sp.child_ids or {}) do
-        finish_span(spans, spans[id])
-    end
-    if code then
-        sp:set_status(code, message)
-    end
-    sp:finish()
-end
-
-
-function _M.finish(ctx, sp, code, message)
-    local tracing = ctx and ctx.tracing
+function _M.finish_all(ctx, code, message)
+    local tracing = ctx.tracing
     if not tracing then
         return
     end
 
-    sp = sp or tracing.current_span
-    if not sp then
-        return
-    end
+    tracing.current_span:set_status(code, message)
+    tracing.current_span:finish(ctx)
 
-    finish_span(tracing.spans, sp, code, message)
-    if sp == tracing.root_span then
-        return
+    while tracing.current_span.parent_id do
+        tracing.current_span = tracing.spans[tracing.current_span.parent_id]
+        tracing.current_span:finish(ctx)
     end
-    tracing.current_span = tracing.spans[sp.parent_id]
 end
 
 
 function _M.release(ctx)
-    local tracing = ctx and ctx.tracing
+    local tracing = ctx.tracing
     if not tracing then
         return
     end
@@ -117,18 +84,4 @@ function _M.release(ctx)
 end
 
 
-function _M.finish_all(ctx, code, message)
-    local tracing = ctx and ctx.tracing
-    if not tracing then
-        return
-    end
-
-    local spans = tracing.spans
-    tracing.current_span = tracing.root_span
-    for _, id in ipairs(tracing.root_span.child_ids or {}) do
-        finish_span(spans, spans[id], code, message)
-    end
-end
-
-
 return _M

apisix/utils/noop_span.lua

@@ -0,0 +1,47 @@
+--
+-- 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.
+--
+local setmetatable = setmetatable
+
+local _M = {}
+
+
+local mt = {
+    __index = _M
+}
+
+function _M.new(ctx, name, kind)
+    return setmetatable({}, mt)
+end
+
+
+function _M.set_status(self, code, message)
+end
+
+
+function _M.set_attributes(self, ...)
+end
+
+
+function _M.finish(self)
+end
+
+
+function _M.release(self)
+end
+
+
+return _M

apisix/utils/span.lua

@@ -19,6 +19,7 @@ local util = require("opentelemetry.util")
 local span_status = require("opentelemetry.trace.span_status")
 local setmetatable = setmetatable
 local table = table
+local new_tab = require("table.new")
 local select = select
 local pool_name = "opentelemetry_span"
 local update_time = ngx.update_time
@@ -36,30 +37,42 @@ local function get_time()
 end
 
 
-function _M.new(name, kind)
-    local self = tablepool.fetch(pool_name, 0, 16)
-    self.start_time = get_time()
-    self.name = name
-    self.kind = kind
-    return setmetatable(self, mt)
-end
-
 
-function _M.append_child(self, child_id)
-    if not self.child_ids then
-        self.child_ids = table.new(10, 0)
+local function append_child(sp, child_id)
+    if not sp.child_ids then
+        sp.child_ids = new_tab(10, 0)
     end
-    table.insert(self.child_ids, child_id)
+    table.insert(sp.child_ids, child_id)
 end
 
 
-function _M.set_parent(self, parent_id)
-    self.parent_id = parent_id
+local function set_parent(sp, parent_id)
+    sp.parent_id = parent_id
 end
 
 
-function _M.release(self)
-    tablepool.release(pool_name, self)
+function _M.new(ctx, name, kind)
+    local tracing = ctx.tracing
+
+    local self = tablepool.fetch(pool_name, 0, 16)
+    self.start_time = get_time()
+    self.name = name
+    self.kind = kind
+
+    table.insert(tracing.spans, self)
+    local id = #tracing.spans
+    self.id = id
+
+    local parent = tracing.current_span
+    if parent then
+        set_parent(self, parent.id)
+        append_child(parent, id)
+    else
+        tracing.root_span = self
+    end
+
+    ctx.tracing.current_span = self
+    return setmetatable(self, mt)
 end
 
 
@@ -94,8 +107,18 @@ function _M.set_attributes(self, ...)
 end
 
 
-function _M.finish(self)
+function _M.finish(self, ctx)
+    local tracing = ctx.tracing
     self.end_time = get_time()
+    if not self.parent_id then
+        return
+    end
+    ctx.tracing.current_span = tracing.spans[self.parent_id]
+end
+
+
+function _M.release(self)
+    tablepool.release(pool_name, self)
 end
 
 

apisix/utils/upstream.lua

@@ -102,7 +102,7 @@ local function parse_domain_for_nodes(nodes)
             core.table.insert(new_nodes, node)
         end
     end
-    tracer.finish(ngx.ctx, span)
+    span:finish(ngx.ctx)
     return new_nodes
 end
 _M.parse_domain_for_nodes = parse_domain_for_nodes