Skip to content

Commit

Permalink
fix(tracing): allow passing nil to span:set_attribute
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
samugi committed Nov 21, 2023
1 parent 67970ea commit b043ac5
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 4 deletions.
5 changes: 3 additions & 2 deletions kong/pdk/tracing.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
13 changes: 11 additions & 2 deletions spec/01-unit/26-tracing/01-tracer_pdk_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down

0 comments on commit b043ac5

Please sign in to comment.