-
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(opentelemetry): support variable resource attributes #13839
base: master
Are you sure you want to change the base?
feat(opentelemetry): support variable resource attributes #13839
Conversation
failing test is expected, let's rebase after #13877 is merged |
6dc12b9
to
eaf8f25
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.
Thank you so much for your contribution, this is great! I left a few comments for things that I feel could be improved a bit. There's some code duplication with request-transformer
that we may want to take care of, but that refactor can be done later.
kong/plugins/opentelemetry/utils.lua
Outdated
@@ -13,6 +21,8 @@ local DEFAULT_HEADERS = { | |||
|
|||
local _log_prefix = "[otel] " | |||
|
|||
local compiled_resource_attributes = {} |
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.
keeping this variable at the module level's scope can be problematic because it will persist across different requests, which may introduce cross-request pollution. I'd recommend we just return the compiled resource attributes from the function that computes them.
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.
@samugi Thanks for quick response. I have updated the pr as suggested.
eaf8f25
to
8a185af
Compare
0581567
to
4cfc804
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.
thanks for the quick action! I left a couple more suggestions, then I think it's good to go.
options = { | ||
compiled_resource_attributes = otel_utils.read_resource_attributes(conf.resource_attributes) | ||
} |
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.
options = { | |
compiled_resource_attributes = otel_utils.read_resource_attributes(conf.resource_attributes) | |
} | |
local compiled, err = otel_utils.read_resource_attributes(conf.resource_attributes) | |
if not compiled then | |
ngx_log(ngx_WARN, _log_prefix, "resource attributes template failed to compile: ", err) | |
end | |
options.compiled_resource_attributes = compiled |
local function http_export_logs(conf, logs_batch) | ||
local function http_export_logs(params, logs_batch) | ||
local conf = params.conf | ||
local resource_attributes = params.options.compiled_resource_attributes |
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.
local resource_attributes = params.options.compiled_resource_attributes | |
local resource_attributes = params.options.compiled_resource_attributes | |
or conf.resource_attributes |
local function http_export_traces(conf, spans) | ||
local function http_export_traces(params, spans) | ||
local conf = params.conf | ||
local resource_attributes = params.options.compiled_resource_attributes |
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.
local resource_attributes = params.options.compiled_resource_attributes | |
local resource_attributes = params.options.compiled_resource_attributes | |
or conf.resource_attributes |
kong/plugins/opentelemetry/utils.lua
Outdated
if err then | ||
return nil, err | ||
end | ||
|
||
if res then | ||
compiled_resource_attributes[current_name] = res | ||
end |
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.
if err then | |
return nil, err | |
end | |
if res then | |
compiled_resource_attributes[current_name] = res | |
end | |
if not res then | |
return nil, err | |
end | |
compiled_resource_attributes[current_name] = res |
local otel_utils = require "kong.plugins.opentelemetry.utils" | ||
local dynamic_hook = require "kong.dynamic_hook" | ||
local o11y_logs = require "kong.observability.logs" | ||
local kong_meta = require "kong.meta" |
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.
local otel_utils = require "kong.plugins.opentelemetry.utils" | |
local dynamic_hook = require "kong.dynamic_hook" | |
local o11y_logs = require "kong.observability.logs" | |
local kong_meta = require "kong.meta" | |
local otel_utils = require "kong.plugins.opentelemetry.utils" | |
local dynamic_hook = require "kong.dynamic_hook" | |
local o11y_logs = require "kong.observability.logs" | |
local kong_meta = require "kong.meta" | |
local _log_prefix = otel_utils._log_prefix | |
local ngx_log = ngx.log | |
local ngx_WARN = ngx.WARN |
4cfc804
to
aba7105
Compare
Summary
This change allows render resource attributes as lua code.
For example,
host.id
is0.0.0.0:9000
or depends onheaders.host
value.Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
Fix #12538