tracing: inject tracing context in router#5661
tracing: inject tracing context in router#5661mattklein123 merged 2 commits intoenvoyproxy:masterfrom
Conversation
This moves the moment that headers related to tracing are injected in downstream requests. Fixes envoyproxy#5504 Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for fixing this. A comment to get started.
/wait
test/common/router/router_test.cc
Outdated
|
|
||
| Http::TestHeaderMapImpl headers; | ||
| HttpTestUtility::addDefaultHeaders(headers); | ||
| EXPECT_CALL(callbacks_, activeSpan()).Times(1); |
There was a problem hiding this comment.
These type of accessor expectations aren't really useful. Since callbacks_ isn't a NiceMock, I would just do something like this in the test constructor.
EXPECT_CALL(callbacks_, activeSpan()).WillRepeatedly(Return(...))
Then, some test should actually make sure the span has injectContext called on it with an expectation.
There was a problem hiding this comment.
Thanks for the review!
Yup, they were added to appease the existing code / RouterTest constructor
I think the value it returns is already handled here
I'll see what I can do to tidy it up and making it more precisely check that injectContext was called.
There was a problem hiding this comment.
Yes understood, my suggestion is how to still use a strict mock and appease it for this type of case, then you don't need to repeat it in every test which isn't useful.
Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>
|
Updated based on the suggestion from @mattklein123 |
|
/retest |
|
🔨 rebuilding |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this looks perfect. @objectiser @rnburn any concerns?
This moves the moment that headers related to tracing are injected in downstream requests. Fixes envoyproxy#5504 Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com> Signed-off-by: Dan Zhang <danzh@google.com>
This moves the moment that headers related to tracing are injected in downstream requests. Fixes envoyproxy#5504 Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Description:
This moves the moment that headers related to tracing are injected
in proxied requests.
More details in issue #5504 and the earlier PR #5529 that didn't fix this correctly
Signed-off-by: Caleb Gilmour caleb.gilmour@datadoghq.com
Risk Level: Low
Testing: unit tests and E2E with
Docs Changes: N/A
Release Notes: N/A
Fixes #5504