skywalking: use upstream host address as addressUsedAtClient in propagation header#20367
skywalking: use upstream host address as addressUsedAtClient in propagation header#20367mattklein123 merged 14 commits intoenvoyproxy:mainfrom
Conversation
|
/assign-from @envoyproxy/envoy-maintainers |
|
@envoyproxy/envoy-maintainers assignee is @KBaichoo |
KBaichoo
left a comment
There was a problem hiding this comment.
//test/extensions/tracers/xray:xray_test fails to build btw
There was a problem hiding this comment.
Thanks. This API change has big impact. So I think we need to think about it more carefully. I am thinking if there are some others method to achieve your target.
For the startSpan, the request path can be obtained from TraceContext and upstream address is unnecessary for the EntrySpan of SkyWalking.
May be we can update injectContext to add new parameter to provides upstream info. The ExitSpan of SkyWalking can get path and also remote address here.
This API will be more natural, because injectContext is called when the tracing info is propagated upstream, and a parameter providing upstream info is more reasonable.
|
/assign |
@wbpcode Thank you for your review. I also concerned about the impact of this change. Thus I considered to add new parameter in |
…on header Signed-off-by: Shikugawa <rei@tetrate.io>
51e0248 to
1be2955
Compare
KBaichoo
left a comment
There was a problem hiding this comment.
thanks, seems like there are still build failures
wbpcode
left a comment
There was a problem hiding this comment.
It seems that there are still some issues that have not been resolved. The problem seems to be more complicated than expected. How to get the correct upstream when injectContext? It seems that only UpstreamRequest can get the target upstream correctly. 🤔
…stream-info Signed-off-by: Shikugawa <rei@tetrate.io>
3ed958c to
7b0a5bf
Compare
|
It looks like the work is almost done. 😄 There are still some problems with CI, some test cases are not fixed. /wait |
|
Hi, @Shikugawa, although we don't get a conclusion of #20454 , we still can complete this work first. And then we can discusss the method about how to inject peer address continuely in the #20454. |
|
After discussion with @wu-sheng, I will pick this up. Thanks for all your great contributions @Shikugawa. |
wbpcode
left a comment
There was a problem hiding this comment.
LGTM. Thanks cc @Shikugawa.
cc @mattklein123 could you take a look for the final pass when you have free time? Thanks.
mattklein123
left a comment
There was a problem hiding this comment.
Does this need a small release note? Otherwise LGTM, thanks.
/wait
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
|
/retest |
|
Retrying Azure Pipelines: |
- Update bazel/repositories.bzl - Update .bazelrc. Uncommented platform_mappings override as it is necessary for nighthawk to build. - Update .bazelversion - Update stream_decoder with updated interface (envoyproxy/envoy#20367). Setting to nullptr is fine as an appropriate default is set if nullptr is passed. In addition, tracing is not a large concern for nighthawk. - Changes due to (envoyproxy/envoy#20862). Due to cipher deprecation, there is only 1 RSA and DSA cipher default left. As a result, to preserve testing coverage, the appropriate certs need to be loaded to the nighthawk instances to test multiple ciphers. As a result, created a new nighthawk configuration file and refactored integration tests. - Updated update process documentation to create PR as last step. - Temporarily disable the `clang-tidy` CI step until #849 gets resolved. Signed-off-by: tomjzzhang <4367421+tomjzzhang@users.noreply.github.com>
…gation header (envoyproxy#20367) Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa rei@tetrate.io
Commit Message: skywalking: use remote IP address as addressUsedAtClient in propagation header
Additional Description: In Apache SkyWalking, the request pathname may be used as the span name, or the remote IP address may be set in propagation.
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]