envoy
envoy copied to clipboard
envoyproxy
ext_proc: propagate tracing context
Commit Message: ext_proc: propagate tracing context
Additional Description: Propagate tracing headers in bidirectional GRPC streams to enable tracing in external processors. I mostly followed the suggestion described in the issue. Please note this is my first C++ code, I kindly ask to give as many references you can requesting changes, there's probably a bunch.
Risk Level: Low
Testing: Ran the existing tests (bazel test --test_output=streamed //test/common/grpc/...) and manually tested tracing with datadog using a few GRPC / HTTP implementations (ext_proc, ext_authz, ratelimit, lua http call). This was my test setup and here is a trace report in datadog with these changes (orange is envoy spans and yellow ones collected in the external processor:
Docs Changes: n/a Release Notes: extp_roc: propagate tracing headers to the external processor. Platform Specific Features: n/a Fixes https://github.com/envoyproxy/envoy/issues/21119
(I messed up https://github.com/envoyproxy/envoy/pull/33565)
Hi @cainelli, welcome and thank you for your contribution.
We will try to review your Pull Request as quickly as possible.
In the meantime, please take a look at the contribution guidelines if you have not done so already.
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/33665 was opened by cainelli.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
Following up on https://github.com/envoyproxy/envoy/pull/33565#issuecomment-2061660995
/assign @wbpcode
cainelli is not allowed to assign users.
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/33665#issuecomment-2064710861 was created by @cainelli.
CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/33665 was synchronize by cainelli.
removing my assignment and deps label - i think they were added as a result of merge mash
cc @tyxia could you take a look again because I know you use this filter heavily.
@wbpcode @tyxia I pushed the requested changes, can you please take a look again?
@cainelli wondering if this should have a changelog - not sure, but it seems like a behaviour change
@cainelli wondering if this should have a changelog - not sure, but it seems like a behaviour change
Makes sense, I added a line in the changelog.
/retest
/unassign @tyxia /assign @yanavlasov
Seems that @tyxia is busy recently, assign this to another code owner.
@cainelli would it be possible to add end-to end test with tracing enabled to ext_proc_integration_test? Otherwise it feels like we have tests that validate bits a pieces of the tracing but nothing that makes sure that all expected spans were created.
/wait-any
@cainelli would it be possible to add end-to end test with tracing enabled to ext_proc_integration_test? Otherwise it feels like we have tests that validate bits a pieces of the tracing but nothing that makes sure that all expected spans were created.
@yanavlasov I managed to get a basic integration test checking if the tracing headers are being propagated to extproc but not sure if that was what you had in mind. I used the opentracing filter which I assume is not ideal but did not find a test tracing filter.
I had a hard time to understand how the integration tests works and failed to find a reference how to test spans there. Please let me know if that's sufficient for now or if you have any pointer to another filter doing it.
@cainelli would it be possible to add end-to end test with tracing enabled to ext_proc_integration_test? Otherwise it feels like we have tests that validate bits a pieces of the tracing but nothing that makes sure that all expected spans were created.
@yanavlasov I managed to get a basic integration test checking if the tracing headers are being propagated to extproc but not sure if that was what you had in mind. I used the opentracing filter which I assume is not ideal but did not find a test tracing filter.
I had a hard time to understand how the integration tests works and failed to find a reference how to test spans there. Please let me know if that's sufficient for now or if you have any pointer to another filter doing it.
@cainelli Will it be possible to add a simple test tracing filter? test/extensions/filters/http/ext_proc/logging_test_filter.cc could be an example though it is for testing logging purpose.
Having an e2e integration test here can not only validate the functionality of this PR but also prevent other people from breaking your feature accidentally.
@tyxia I have added a test tracing filter for the integration tests, really apologies for any eventual non-idiomatic/bad pattern code, as mentioned this is my first c++ contribution and I'm trying hard. The places I wasn't so sure and would appreciate any input.
- The overall proposed architecture for tests tracing in integration tests, it feels mock could be useful but I did not find a way to inject and assert during the integration tests. If that is feasible and it makes more sense please let me know how you'd approach it.
- If mock is not doable, is it ok asserting on the destructor? It just felt okish but I'm not sure of side effects with this approach.
- I only added a couple of integration tests so you get the concept but could add more if the overall approach is acceptable.
- I could move this test tracer to
test/integration/filtersif you think it is generic enough for other filters.