-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tracing): Add instana header support in propagation #13915
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this great contribution! A couple of notes:
- For consistency, this should be supported by the Zipkin plugin as well, which requires a small schema update similar to what was done for OpenTelemetry in this PR, and an integration test in
spec/03-plugins/34-zipkin/zipkin_spec.lua
. - The test in
spec/01-unit/26-observability/02-propagation_strategies_spec.lua
should be updated as well to include this new header format
In addition to my previous comment, this needs a rebase. |
e28fb70
to
0c0f894
Compare
|
||
trace_id = from_hex(trace_id_raw) or nil | ||
span_id = from_hex(span_id_raw) or nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated trace_id
, span_id
, level_id
variable name to trace_id_raw
, span_id_raw
and level_id_raw
Also since we dont use correlation_type
and correlationId
here, changed
level_id_raw = level_id_raw:match("^([0-1])$")
or level_id_raw:match("^([0-1]),correlationType=(.-);correlationId=(.*)")
to
level_id_raw = level_id_raw:match("^([0-1])$")
or level_id_raw:match("^([0-1]).")
b21ca04
to
e26f7ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
Please also address my comments from the previous review:
-
For consistency, this should be supported by the Zipkin plugin as well, which requires a small schema update similar to what was done for OpenTelemetry in this PR, and an integration test in
spec/03-plugins/34-zipkin/zipkin_spec.lua
-
The test in
spec/01-unit/26-observability/02-propagation_strategies_spec.lua
should be updated as well to include this new header format
local trace_id = from_hex(trace_id_raw) or nil | ||
local span_id = from_hex(span_id_raw) or nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a nil check:
local trace_id = from_hex(trace_id_raw) or nil | |
local span_id = from_hex(span_id_raw) or nil | |
local trace_id = trace_id_raw and from_hex(trace_id_raw) or nil | |
local span_id = span_id_raw and from_hex(span_id_raw) or nil |
local trace_id_raw = headers["x-instana-t"] | ||
local span_id_raw = headers["x-instana-s"] | ||
local level_id_raw = headers["x-instana-l"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the value of headers["name"]
can be a string or a table, depending on whether the same header was passed more than once in the request, see: https://github.com/openresty/lua-nginx-module?tab=readme-ov-file#ngxreqget_headers
We should handle the scenario where multiple headers are passed, for example by ignoring the header if passed more than once, or grabbing the first value.
level_id_raw = level_id_raw:match("^([0-1])$") | ||
or level_id_raw:match("^([0-1]).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just:
level_id_raw = level_id_raw:match("^([0-1])$") | |
or level_id_raw:match("^([0-1]).") | |
level_id_raw = level_id_raw:sub(1, 1) |
level_id_raw = level_id_raw:match("^([0-1])$") | ||
or level_id_raw:match("^([0-1]).") | ||
end | ||
local should_sample = level_id_raw or "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this must be type boolean, see here
local should_sample = level_id_raw or "1" | |
local should_sample = level_id_raw == "1" |
d79c73a
to
834a15d
Compare
834a15d
to
ee041d6
Compare
Summary
Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
Fix #12494