Client span generation for upstream requests#97
Conversation
This commit adds support to configure client span generation for
upstream requests. Client span generation can be configured per
upstream using a configuration snippet like the one below:
upstream echo.sunnypup.io {
otel_upstream_span_enable;
server echo.sunnypup.io:443;
}
The primary vehicle for this is the load balancer API. Existing
load balancer callbacks and data are stored in a new context type.
New callbacks are added for getting, freeing requests as well as
setting and saving ssl sessions. These modifications are made if
and only if the otel_upstream_span_enable configuration directive
is made (per upstream).
In addition to the above, A new enumeration is made in the batch
exporter for span type. Currently Client and Server are recognized,
however this can be extended as needed in the future. This enum is
matched with corresponding elements in the underlying opentelemetry
library.
The logic path between getOtelCtx/ensureOtelCtx/onRequestStart is
modified to make sure that even upstream calls made by subrequests
have the correct metadata needed for proper span propagation.
Additionally, some logic is refactored out of onRequestEnd for
reuse in the upstream span generation.
Finally, a new routine is added to set client span specific attrs.
Note: these changes do require that NGINX is compiled with SSL
support. This is needed because use of the load balancer API,
specifically wrapping around the data argument to callbacks, must
be handled properly when setting SSL sessions or else a memory
corruption error can be triggered by the round robin SSL handlers
recieving bad data.
Signed-off-by: Ava Hahn <a.hahn@f5.com>
This commit adds some loose testing for the functionality included in the previous commit. A new test is minted that verifies the type, quantity, and return code of client and server spans after a request that leverages an upstream. Signed-off-by: Ava Hahn <a.hahn@f5.com>
|
As discussed offline some time ago, there has to be some global switch for this functionality. It should also cover proxy_pass without explicit upstream. |
|
Thank you @p-pautov. Would a configuration switch at the server block level work? I understand it would have to be configured per-server but it saves the effort of configuring per upstream and can be implemented relatively simply. I will make sure that proxy_pass is covered. |
|
This PR would really unblock a major use case for us, because without proper client spans for upstreams nginx-otel currently breaks our service graphs, and a global switch is not important for our scenario. |
|
This would also unbreak our service graphs. We use Nginx in front of PHP-FPM (Symfony) and send the OTLP data to Elastic APM. Because the CLIENT span is missing on the fastcgi_pass, Elastic completely drops the edge in the Service Map topology. Having an opt-in config to generate the upstream client span would solve a massive visibility issue for Elastic/Kibana users 👍 |
Proposed changes
Fixes #17
See the below commit description for details:
A second commit is included to provide additional testing:
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocument