tracing: inject the tracing header before the upstream filter chain is started#27269
Merged
wbpcode merged 7 commits intoenvoyproxy:mainfrom May 15, 2023
Merged
Conversation
…s started Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
added 3 commits
May 9, 2023 12:24
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
…ix-tracing-injection
alyssawilk
reviewed
May 10, 2023
Contributor
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Thanks for the quick fix!
added 2 commits
May 10, 2023 16:37
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
…ix-tracing-injection
wbpcode
commented
May 11, 2023
Comment on lines
+110
to
+119
| // The router checks that the connection pool is non-null before creating the upstream request. | ||
| auto upstream_host = conn_pool_->host(); | ||
| if (span_ != nullptr) { | ||
| span_->injectContext(*parent_.downstreamHeaders(), upstream_host); | ||
| } else { | ||
| // No independent child span for current upstream request then inject the parent span's tracing | ||
| // context into the request headers. | ||
| // The injectContext() of the parent span may be called repeatedly when the request is retried. | ||
| parent_.callbacks()->activeSpan().injectContext(*parent_.downstreamHeaders(), upstream_host); | ||
| } |
Member
Author
There was a problem hiding this comment.
And tell the truth, I have a quesion about the host() of pool.
If we can access the host directly by the pool, why we need the host parameter in the onPoolReady() method?
For historical reason?
Contributor
There was a problem hiding this comment.
I suspect so, but not sure.
Member
Author
|
hmmmm, why this pr effect the coverage of tap transport_sockets 🤔
|
…ix-tracing-injection
Member
Author
|
#27382 resolved this coverage problem temporarily. |
Member
Author
|
friendly ping @alyssawilk |
alyssawilk
approved these changes
May 15, 2023
Comment on lines
+110
to
+119
| // The router checks that the connection pool is non-null before creating the upstream request. | ||
| auto upstream_host = conn_pool_->host(); | ||
| if (span_ != nullptr) { | ||
| span_->injectContext(*parent_.downstreamHeaders(), upstream_host); | ||
| } else { | ||
| // No independent child span for current upstream request then inject the parent span's tracing | ||
| // context into the request headers. | ||
| // The injectContext() of the parent span may be called repeatedly when the request is retried. | ||
| parent_.callbacks()->activeSpan().injectContext(*parent_.downstreamHeaders(), upstream_host); | ||
| } |
Contributor
There was a problem hiding this comment.
I suspect so, but not sure.
wbpcode
pushed a commit
to wbpcode/envoy
that referenced
this pull request
May 16, 2023
…s started (envoyproxy#27269) * tracing: inject the tracing header before the upstream filter chain is started Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * fix test Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> --------- Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
wbpcode
pushed a commit
to wbpcode/envoy
that referenced
this pull request
May 23, 2023
…s started (envoyproxy#27269) * tracing: inject the tracing header before the upstream filter chain is started Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * fix test Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> --------- Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
phlax
pushed a commit
that referenced
this pull request
May 24, 2023
…s started (#27269) * tracing: inject the tracing header before the upstream filter chain is started Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * fix test Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> --------- Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
reskin89
pushed a commit
to reskin89/envoy
that referenced
this pull request
Jul 11, 2023
…s started (envoyproxy#27269) * tracing: inject the tracing header before the upstream filter chain is started Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * fix test Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> * update comment Signed-off-by: wbpcode <wangbaiping@corp.netease.com> --------- Signed-off-by: wbpcode <wangbaiping@corp.netease.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
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
2 participants
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.
Commit Message: tracing: inject the tracing header before the upstream filter chain is started
Additional Description:
The #20503 introduced a unexpected behavior change: that is the tracing headers will be injected after the
request_headers_to_addis handled.It means that users connot access the tracing headers in the
request_headers_to_add. But some users require this. Here is the ticket #23972.I originally intended to resolve this problem by using the upstream header mutation filter.
However, the upstream filter chain will be started directly without waiting the upstream connection to be ready (unexpected design, why, shorter latency?) But the tracing headers will be injected after the upstream connection is ready. So, the upstream header mutation filter still cannot access the tracing headers in some case.
This PR just make sure the tracing headers injection will complete before the upstream filter chain start.
Risk Level: Low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]