From 968de120c00aef23bf81b0db7052c1a7507ee2d7 Mon Sep 17 00:00:00 2001 From: samugi Date: Tue, 11 Apr 2023 23:44:06 +0200 Subject: [PATCH] fix(tracing): sampled flags and propagation fix an issue where incoming propagation headers with disabled sampling produced invalid propagation of noop spans and broken traces This fix: * ensures propagation is not attempted on noop spans * ensures no span is sampled when sampling is disabled in the incoming headers * propagates the correct header in when sampling is disabled in the incoming headers --- kong/pdk/tracing.lua | 43 ++++-- kong/tracing/propagation.lua | 5 + .../14-tracing/02-propagation_spec.lua | 130 +++++++++++++++--- .../plugins/tcp-trace-exporter/handler.lua | 3 + .../kong/plugins/trace-propagator/handler.lua | 6 +- 5 files changed, 156 insertions(+), 31 deletions(-) diff --git a/kong/pdk/tracing.lua b/kong/pdk/tracing.lua index f851c5dea2fd..7d8c54128590 100644 --- a/kong/pdk/tracing.lua +++ b/kong/pdk/tracing.lua @@ -156,9 +156,17 @@ local function create_span(tracer, options) local trace_id = span.parent and span.parent.trace_id or options.trace_id or generate_trace_id() - local sampled = span.parent and span.parent.should_sample - or options.should_sample - or tracer and tracer.sampler(trace_id) + + local sampled + if span.parent and span.parent.should_sample ~= nil then + sampled = span.parent.should_sample + + elseif options.should_sample ~= nil then + sampled = options.should_sample + + else + sampled = tracer and tracer.sampler(trace_id) + end if not sampled then return noop_span @@ -170,16 +178,17 @@ local function create_span(tracer, options) span.span_id = generate_span_id() span.trace_id = trace_id span.kind = options.span_kind or SPAN_KIND.INTERNAL - -- indicates whether the span should be reported - span.should_sample = span.parent and span.parent.should_sample - or options.should_sample - or sampled + span.should_sample = true setmetatable(span, span_mt) return span end local function link_span(tracer, span, name, options) + if not span.should_sample then + kong.log.debug("skipping non-sampled span") + return + end if tracer and type(tracer) ~= "table" then error("invalid tracer", 2) end @@ -386,6 +395,7 @@ noop_tracer.link_span = NOOP noop_tracer.active_span = NOOP noop_tracer.set_active_span = NOOP noop_tracer.process_span = NOOP +noop_tracer.set_should_sample = NOOP --- New Tracer local function new_tracer(name, options) @@ -482,12 +492,29 @@ local function new_tracer(name, options) end for _, span in ipairs(ctx.KONG_SPANS) do - if span.tracer.name == self.name then + if span.tracer and span.tracer.name == self.name then processor(span, ...) end end end + --- Update the value of should_sample for all spans + -- + -- @function span:set_should_sample + -- @tparam bool should_sample value for the sample parameter + function self:set_should_sample(should_sample) + local ctx = ngx.ctx + if not ctx.KONG_SPANS then + return + end + + for _, span in ipairs(ctx.KONG_SPANS) do + if span.is_recording ~= false then + span.should_sample = should_sample + end + end + end + tracer_memo[name] = setmetatable(self, tracer_mt) return tracer_memo[name] end diff --git a/kong/tracing/propagation.lua b/kong/tracing/propagation.lua index c7bcc692dff5..21a883346d02 100644 --- a/kong/tracing/propagation.lua +++ b/kong/tracing/propagation.lua @@ -427,6 +427,11 @@ end local function set(conf_header_type, found_header_type, proxy_span, conf_default_header_type) + if proxy_span.is_recording == false then + kong.log.debug("skipping propagation of noop span") + return + end + local set_header = kong.service.request.set_header -- If conf_header_type is set to `preserve`, found_header_type is used over default_header_type; diff --git a/spec/02-integration/14-tracing/02-propagation_spec.lua b/spec/02-integration/14-tracing/02-propagation_spec.lua index ec3bd998626f..e1086239097e 100644 --- a/spec/02-integration/14-tracing/02-propagation_spec.lua +++ b/spec/02-integration/14-tracing/02-propagation_spec.lua @@ -1,47 +1,59 @@ local helpers = require "spec.helpers" local cjson = require "cjson" +local utils = require "kong.tools.utils" +local to_hex = require("resty.string").to_hex + +local rand_bytes = utils.get_rand_bytes local TCP_PORT = 35001 + +local function gen_id(len) + return to_hex(rand_bytes(len)) +end + for _, strategy in helpers.each_strategy() do local proxy_client describe("tracing propagation spec #" .. strategy, function() - describe("spans hierarchy", function () + lazy_setup(function() + local bp, _ = assert(helpers.get_db_utils(strategy, { + "routes", + "plugins", + }, { "tcp-trace-exporter", "trace-propagator" })) - lazy_setup(function() - local bp, _ = assert(helpers.get_db_utils(strategy, { - "routes", - "plugins", - }, { "tcp-trace-exporter", "trace-propagator" })) - - bp.routes:insert({ - hosts = { "propagate.test" }, - }) + bp.routes:insert({ + hosts = { "propagate.test" }, + }) - bp.plugins:insert({ - name = "tcp-trace-exporter", - config = { - host = "127.0.0.1", - port = TCP_PORT, - custom_spans = false, - } - }) + bp.plugins:insert({ + name = "tcp-trace-exporter", + config = { + host = "127.0.0.1", + port = TCP_PORT, + custom_spans = false, + } + }) - bp.plugins:insert({ - name = "trace-propagator" - }) + bp.plugins:insert({ + name = "trace-propagator" + }) + end) + describe("spans hierarchy", function () + lazy_setup(function() assert(helpers.start_kong { database = strategy, nginx_conf = "spec/fixtures/custom_nginx.template", plugins = "tcp-trace-exporter,trace-propagator", tracing_instrumentations = "balancer", }) - proxy_client = helpers.proxy_client() end) lazy_teardown(function() + if proxy_client then + proxy_client:close() + end helpers.stop_kong() end) @@ -75,5 +87,79 @@ for _, strategy in helpers.each_strategy() do assert.equals("00-" .. trace_id .. "-" .. span_id .. "-01", traceparent) end) end) + + describe("parsing incoming headers", function () + local trace_id = gen_id(16) + local span_id = gen_id(8) + + lazy_setup(function() + assert(helpers.start_kong { + database = strategy, + nginx_conf = "spec/fixtures/custom_nginx.template", + plugins = "tcp-trace-exporter,trace-propagator", + tracing_instrumentations = "request,router,balancer,plugin_access,plugin_header_filter", + }) + proxy_client = helpers.proxy_client() + end) + + lazy_teardown(function() + if proxy_client then + proxy_client:close() + end + helpers.stop_kong() + end) + + it("enables sampling when incoming header has sampled enabled", function () + local thread = helpers.tcp_server(TCP_PORT) + local r = assert(proxy_client:send { + method = "GET", + path = "/request", + headers = { + ["Host"] = "propagate.test", + traceparent = string.format("00-%s-%s-01", trace_id, span_id), + } + }) + assert.res_status(200, r) + local body = r:read_body() + body = assert(body and cjson.decode(body)) + + local ok, res = thread:join() + assert.True(ok) + assert.is_string(res) + + -- all spans are returned + local spans = cjson.decode(res) + assert.is_same(6, #spans, res) + + local traceparent = assert(body.headers.traceparent) + assert.matches("00%-" .. trace_id .. "%-%x+%-01", traceparent) + end) + + it("disables sampling when incoming header has sampled disabled", function () + local thread = helpers.tcp_server(TCP_PORT) + local r = assert(proxy_client:send { + method = "GET", + path = "/request", + headers = { + ["Host"] = "propagate.test", + traceparent = string.format("00-%s-%s-00", trace_id, span_id), + } + }) + assert.res_status(200, r) + local body = r:read_body() + body = assert(body and cjson.decode(body)) + + local ok, res = thread:join() + assert.True(ok) + assert.is_string(res) + + -- no spans are returned + local spans = cjson.decode(res) + assert.is_same(0, #spans, res) + + local traceparent = assert(body.headers.traceparent) + assert.equals("00-" .. trace_id .. "-" .. span_id .. "-00", traceparent) + end) + end) end) end diff --git a/spec/fixtures/custom_plugins/kong/plugins/tcp-trace-exporter/handler.lua b/spec/fixtures/custom_plugins/kong/plugins/tcp-trace-exporter/handler.lua index 8fcd0833771e..fdb187aceab8 100644 --- a/spec/fixtures/custom_plugins/kong/plugins/tcp-trace-exporter/handler.lua +++ b/spec/fixtures/custom_plugins/kong/plugins/tcp-trace-exporter/handler.lua @@ -104,6 +104,9 @@ function _M:log(config) local spans = {} local process_span = function (span) + if span.should_sample == false then + return + end local s = table.clone(span) s.tracer = nil s.parent = nil diff --git a/spec/fixtures/custom_plugins/kong/plugins/trace-propagator/handler.lua b/spec/fixtures/custom_plugins/kong/plugins/trace-propagator/handler.lua index fc46432551a6..0a7bb1bb8463 100644 --- a/spec/fixtures/custom_plugins/kong/plugins/trace-propagator/handler.lua +++ b/spec/fixtures/custom_plugins/kong/plugins/trace-propagator/handler.lua @@ -16,7 +16,11 @@ function _M:access(conf) local tracer = kong.tracing.new("trace-propagator") local root_span = tracer.start_span("root") - local header_type, trace_id, span_id, parent_id = propagation_parse(headers) + local header_type, trace_id, span_id, parent_id, should_sample = propagation_parse(headers) + + if should_sample == false then + tracer:set_should_sample(should_sample) + end if trace_id then root_span.trace_id = trace_id