bzp2010 commented on code in PR #12686:
URL: https://github.com/apache/apisix/pull/12686#discussion_r2471659658
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -149,11 +150,15 @@ function _M.match_and_set(api_ctx, match_only, alt_sni)
local err
if not radixtree_router or
radixtree_router_ver ~= ssl_certificates.conf_version then
+ local span = tracer.new_span("create_router", tracer.kind.internal)
radixtree_router, err = create_router(ssl_certificates.values)
if not radixtree_router then
+ span:set_status(tracer.status.ERROR, "failed create router")
+ tracer.finish_current_span()
return false, "failed to create radixtree router: " .. err
end
radixtree_router_ver = ssl_certificates.conf_version
+ tracer.finish_current_span()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
