-
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(pdk) tracing pdk and core instrumentation #8724
Conversation
eef5f04
to
c6f12e2
Compare
56e866e
to
df6019a
Compare
df6019a
to
5210cb0
Compare
452f937
to
1142d36
Compare
bfa5e60
to
e827c3a
Compare
a1af8c0
to
4f80ebe
Compare
28290cb
to
fa756bc
Compare
🚀 Performance test resultTest Suite: 01-rps 02-flamegraph (baseline,single_route,simple,admin_api) Click to expand
|
Co-authored-by: Wangchong Zhou <fffonion@gmail.com>
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.
🍔
-- @function kong.tracing.new_span | ||
-- @phases rewrite, access, header_filter, response, body_filter, log, admin_api | ||
-- @tparam table span | ||
function self.set_active_span(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.
Would you add some comments to explain why we don't do this in start_span
automatically?
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 have to do the checking for the
ctx.active_span
to see if it exists, it could be expensive. - At most of the time, the caller will not set the active span.
if phase == "rewrite" then | ||
span = instrumentation.plugin_rewrite(plugin) | ||
elseif phase == "header_filter" then | ||
span = instrumentation.plugin_header_filter(plugin) | ||
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.
Could we just do this?
instrumentation.plugin_phase(phase, plugin)
(exposing plugin_callback
with name plugin_phase
)
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 function itself could be no-op, so I think use plugin_xxx
format could be more easier.
|
||
-- Record DB query | ||
function _M.db_query(connector) | ||
local f = connector.query |
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.
Naming could be better.
And what if we save the original function, like
connector.query_origin = connector.query
So we can escape from tracing when needed?
For example:
http_export_request
calls request_uri
which is hooked here.
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 think there is no way back to the original function when we have patched :(
Is there any documentation for us to learn how to use this feature? |
is it compatible with elastic APM? |
|
Summary
Feature request from #7321
Main tasks:
pdk/tracing
new tracing APIs that generally follow the OpenTemetry API spec.pdk/tracing
tracing storage withngx.ctx.KONG_SPANS
and store Active Span inngx.ctx.<namespace>_active_span
.t/01-pdk/15-tracing
span context tests.spec/01-unit/26-tracer_spec
PDK spec test, like input error assertion and checking basic tracer usages.spec/02-integration/14-tracing
integration tests, testing with a custom trace exporter plugin.runloop/handler
release related tables to the table pools, doing cleanup works.propagation
movedtracing_headers
module (of Zipkin) to the core, to be a common module for any exporters, e.g. coming OTLP exporter.tracing/instrumentation
wraps the tracer and adds hooks/hack.kong.conf.default
FT-2587