From b043ac59fd4f34d26dc74b08cf99fddbf2b962c3 Mon Sep 17 00:00:00 2001 From: samugi Date: Tue, 21 Nov 2023 16:35:04 +0100 Subject: [PATCH] fix(tracing): allow passing nil to span:set_attribute * passing a nil value to `span:set_attribute` is a NOOP if the attribute does not already exists, else it means unsetting that attribute * considered a fix because previously this was causing a stack trace when the DNS spans were created without a port --- kong/pdk/tracing.lua | 5 +++-- spec/01-unit/26-tracing/01-tracer_pdk_spec.lua | 13 +++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/kong/pdk/tracing.lua b/kong/pdk/tracing.lua index ef9d81e0db9..d71a3be50b0 100644 --- a/kong/pdk/tracing.lua +++ b/kong/pdk/tracing.lua @@ -296,11 +296,12 @@ end -- -- @function span:set_attribute -- @tparam string key --- @tparam string|number|boolean value +-- @tparam string|number|boolean|nil value -- @usage -- span:set_attribute("net.transport", "ip_tcp") -- span:set_attribute("net.peer.port", 443) -- span:set_attribute("exception.escaped", true) +-- span:set_attribute("unset.this", nil) function span_mt:set_attribute(key, value) -- key is decided by the programmer, so if it is not a string, we should -- error out. @@ -309,7 +310,7 @@ function span_mt:set_attribute(key, value) end local vtyp = type(value) - if vtyp ~= "string" and vtyp ~= "number" and vtyp ~= "boolean" then + if vtyp ~= "string" and vtyp ~= "number" and vtyp ~= "boolean" and vtyp ~= "nil" then -- we should not error out here, as most of the caller does not catch -- errors, and they are hooking to core facilities, which may cause -- unexpected behavior. diff --git a/spec/01-unit/26-tracing/01-tracer_pdk_spec.lua b/spec/01-unit/26-tracing/01-tracer_pdk_spec.lua index 285c980adf8..0c16635d92d 100644 --- a/spec/01-unit/26-tracing/01-tracer_pdk_spec.lua +++ b/spec/01-unit/26-tracing/01-tracer_pdk_spec.lua @@ -189,11 +189,20 @@ describe("Tracer PDK", function() assert.has_no.error(function () span:finish() end) end) - it("fails set_attribute", function () + it("set_attribute validation", function () local span = c_tracer.start_span("meow") + -- nil value is allowed as a noop span:set_attribute("key1") - assert.spy(log_spy).was_called_with(ngx.ERR, match.is_string()) + assert.spy(log_spy).was_not_called_with(ngx.ERR, match.is_string()) + assert.is_nil(span.attributes["key1"]) + + span:set_attribute("key1", "value1") + assert.equal("value1", span.attributes["key1"]) + + -- nil value unsets the attribute + span:set_attribute("key1") + assert.is_nil(span.attributes["key1"]) span:set_attribute("key1", function() end) assert.spy(log_spy).was_called_with(ngx.ERR, match.is_string())