-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: add more spans to opentelemetry plugin #12686
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
base: master
Are you sure you want to change the base?
Conversation
| self.end_time = 0 | ||
| self.kind = kind | ||
| self.attributes = self.attributes or {} | ||
| self.children = self.children or {} |
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 table attributes and children, they can be reused
we can run the flame graph, to confirm if we need this optimize
apisix/utils/span.lua
Outdated
|
|
||
| function _M.set_status(self, code, message) | ||
| code = span_status.validate(code) | ||
| local status = { |
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 status = self.status
if not status then
status = {
code = code,
message = ""
}
self.status = status
else
status.code = code
end
if code == span_status.ERROR then
status.message = message
end
apisix/utils/span.lua
Outdated
|
|
||
|
|
||
| function _M.set_attributes(self, ...) | ||
| for _, attr in ipairs({ ... }) do |
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.
current way is slow, make a try with new way:
for ... select('#' )| self.end_time = util.time_nano() | ||
| end | ||
|
|
||
| function _M.release(self) |
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 same as current, the table pool will call table.clear auto
| function _M.release(self) | |
| function _M.release(self) | |
| tablepool.release(pool_name, self) | |
| end |
apisix/utils/stack.lua
Outdated
|
|
||
|
|
||
| function _M.clear(self) | ||
| for i = 1, self._n do |
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.
we can call table.clear(self._data), which is much easier
apisix/utils/tracer.lua
Outdated
| end | ||
|
|
||
| function _M.finish_all_spans(code, message) | ||
| if not ngx.ctx._apisix_spans then |
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 apisix_spans = ngx.ctx._apisix_spans
if not apisix_spans then
return
end
for _, sp in pairs(apisix_spans) do
if code then
sp:set_status(code, message)
end
sp:finish()
end| return false, "failed to create radixtree router: " .. err | ||
| end | ||
| radixtree_router_ver = ssl_certificates.conf_version | ||
| tracer.finish_current_span() |
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.
Frankly, this API is confusing. The call to end a span should be something like span:finish(). Currently, using tracer.finish_current_span() implies that spans are managed by a context record within the tracer. This creates confusion for me—could it lead to misuse or conflicts when handling multiple requests in parallel or when a single request contains yield operations? Especially since we might optimize it in the future to use some kind of table pooling mechanism.
This is a concern. While I initially suspect it might not be an issue in single-threaded Nginx, we should avoid this confusion in the API design. It requires programmers to be thoroughly familiar with the OpenResty programming model, understanding the extent to which data is shared and how conflicts might arise. This imposes an additional explanation burden on us.
This is from a DX perspective. Technically, we may need to rethink how the stack is used to properly connect all spans.
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 the same issue when I review this code first time
APISIX will encounter an error if there are concurrent requests.
New way(should same as @bzp2010)

Description
Run jaeger in local by https://www.jaegertracing.io/docs/2.11/getting-started/#all-in-one
Which issue(s) this PR fixes:
Fixes #
Checklist