Client span generation for upstream requests#97
Open
avahahn wants to merge 2 commits intonginxinc:mainfrom
Open
Client span generation for upstream requests#97avahahn wants to merge 2 commits intonginxinc:mainfrom
avahahn wants to merge 2 commits intonginxinc:mainfrom
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>
Contributor
|
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. |
Author
|
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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