update opencensus to tonic/std::future#543
Merged
hawkw merged 10 commits intoeliza/0.2-httpfrom Jun 1, 2020
Merged
Conversation
hawkw
commented
May 29, 2020
| .push(control::add_origin::Layer::new()) | ||
| .into_new_service() | ||
| .new_service(addr.clone()); | ||
| .with_fixed_target(addr.clone()); |
Contributor
Author
There was a problem hiding this comment.
This seemed like a better approach to me than making linkerd2-opencensus depend on linkerd2-app-core just to get the ControlAddr type, wdyt?
Comment on lines
48
to
+49
| // We hold the response future, but never poll it. | ||
| _rsp: ResponseFuture<ExportTraceServiceResponse, T::Future>, | ||
| _rsp: Pin< |
Contributor
Author
There was a problem hiding this comment.
Lucio Franco mentioned to me that not polling this future may result in the h2 connection window filling up, so I may look into changing this code so that it is polled (even though we don't care about the result).
This branch updates the service profiles controller client and stack layers to `std::future`, and puts service profiles back in the outbound and inbound stacks. Retries do not actually happen yet because the retry code hasn't been updated. All of the tests for profile destination overrides can now be re-enabled. In addition, some of the the tests in `profiles.rs` could also be re-enabled, with the exception of the ones that actually expect retries to happen. Depends on #536. Signed-off-by: Eliza Weisman <[email protected]>
This PR updates everything remaining in the `linkerd2-proxy-http` crate to use `std::future`. In particular, this includes the following modules: * `add_header` * `insert` * `override_authority` * `header_from_target` The updates for these modules was pretty mechanical and there are no significant changes, just updates to use the new versions of the `Future` and `Service` traits. In addition, I've put back the stack layers from these modules, where possible (in some cases, these modules were used by other code that has yet to be updated or is pending in a separate branch). I've also re-enabled some of the the `discovery` integration tests for the `linkerd2-dst-override` header. Some of these tests depend on service profiles as well, so they will be enabled once #537 merges. Furthermore, I've done a little cleanup now that everything in the crate has been updated. I removed the `grpc` module entirely. Its only purpose was converting between the `tower_grpc::Body` and `hyper::Payload` traits, and `tonic` and the `std::future` version of `hyper` both use the `http_body::Body` trait, so this conversion is unnecessary. Finally, since all the code which depended on the old versions of `tokio` and `futures` has now been updated, I've removed the dependencies on the legacy versions, and renamed the `futures_03` and `tokio_02` renaming imports to just `futures` and `tokio`. Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
(where "works" == "compiles") Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
force-pushed
the
eliza/0.2-opencensus
branch
from
995cb5b to
48e1464
Compare
olix0r
approved these changes
Jun 1, 2020
hawkw
added a commit
that referenced
this pull request
Jun 9, 2020
**Note**: this branch was already approved (as #543), but was accidentally merged into the dev branch for #541, which it depended on and was set as the base until that branch merged. It looks like, when merging #543, I forgot to change the base branch back to `master-tokio-0.2`, and these changes never made it to master. --- This branch updates the proxy's OpenCensus tracing code and collector client to use Tonic and `std::future`, and puts the OpenCensus stack layers back in the inbound and outbound proxy. Of note: the previous implementation constructed a single client `Service` which was reused when restarting a new RPC if the stream terminated. Because Tonic's generated RPCs borrow the client service mutably for the duration of the RPC future, if we want these futures to be `'static`, we must move the service into the future. Rather than requiring the service to be `Clone`, I solved this by replacing the `Service` instance held by the client with a `NewService` instance. I don't *think* this should be an issue, let me know if I'm wrong? I'd like to do some additional testing with the OpenCensus integration tests but I'm opening this PR now to start getting reviews. Signed-off-by: Eliza Weisman <[email protected]>
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.
This branch updates the proxy's OpenCensus tracing code and collector
client to use Tonic and
std::future, and puts the OpenCensus stacklayers back in the inbound and outbound proxy.
Of note: the previous implementation constructed a single client
Servicewhich was reused when restarting a new RPC if the streamterminated. Because Tonic's generated RPCs borrow the client service
mutably for the duration of the RPC future, if we want these futures to
be
'static, we must move the service into the future. Rather thanrequiring the service to be
Clone, I solved this by replacing theServiceinstance held by the client with aNewServiceinstance. Idon't think this should be an issue, let me know if I'm wrong?
I'd like to do some additional testing with the OpenCensus integration
tests but I'm opening this PR now to start getting reviews.
Depends on #541.
Signed-off-by: Eliza Weisman [email protected]