address comments

* set mocked stream_info as parent context in tests. * close span before notify the callbacks. * Remove struct of alias Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli · May 8, 2024 · c16123e · c16123e
1 parent 085b774
commit c16123e
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 8 deletions.
diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc
@@ -242,8 +242,8 @@ void AsyncStreamImpl::notifyRemoteClose(Grpc::Status::GrpcStatus status,
  if (status != Grpc::Status::WellKnownGrpcStatus::Ok) {
  current_span_->setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True);
  }
- callbacks_.onRemoteClose(status, message);
  current_span_->finishSpan();
+ callbacks_.onRemoteClose(status, message);
 }
 
 void AsyncStreamImpl::onComplete() {

diff --git a/source/extensions/filters/http/gcp_authn/gcp_authn_impl.cc b/source/extensions/filters/http/gcp_authn/gcp_authn_impl.cc
@@ -48,7 +48,7 @@ void GcpAuthnClient::fetchToken(RequestCallbacks& callbacks, Http::RequestMessag
  }
 
  // Set up the request options.
- struct Envoy::Http::AsyncClient::StreamOptions options =
+ Envoy::Http::AsyncClient::RequestOptions options =
  Envoy::Http::AsyncClient::RequestOptions()
  .setTimeout(std::chrono::milliseconds(
  DurationUtil::durationToMilliseconds(config_.http_uri().timeout())))

diff --git a/test/extensions/filters/http/ext_proc/client_test.cc b/test/extensions/filters/http/ext_proc/client_test.cc
@@ -79,14 +79,16 @@ class ExtProcStreamTest : public testing::Test, public ExternalProcessorCallback
 };
 
 TEST_F(ExtProcStreamTest, OpenCloseStream) {
- auto stream = client_->start(*this, config_with_hash_key_, Http::AsyncClient::StreamOptions());
+ auto options = Http::AsyncClient::StreamOptions().setParentContext(stream_info_);
+ auto stream = client_->start(*this, config_with_hash_key_, options);
  EXPECT_CALL(stream_, closeStream());
  EXPECT_CALL(stream_, resetStream());
  stream->close();
 }
 
 TEST_F(ExtProcStreamTest, SendToStream) {
- auto stream = client_->start(*this, config_with_hash_key_, Http::AsyncClient::StreamOptions());
+ auto options = Http::AsyncClient::StreamOptions().setParentContext(stream_info_);
+ auto stream = client_->start(*this, config_with_hash_key_, options);
  // Send something and ensure that we get it. Doesn't really matter what.
  EXPECT_CALL(stream_, sendMessageRaw_(_, false));
  ProcessingRequest req;
@@ -97,14 +99,16 @@ TEST_F(ExtProcStreamTest, SendToStream) {
 }
 
 TEST_F(ExtProcStreamTest, SendAndClose) {
- auto stream = client_->start(*this, config_with_hash_key_, Http::AsyncClient::StreamOptions());
+ auto options = Http::AsyncClient::StreamOptions().setParentContext(stream_info_);
+ auto stream = client_->start(*this, config_with_hash_key_, options);
  EXPECT_CALL(stream_, sendMessageRaw_(_, true));
  ProcessingRequest req;
  stream->send(std::move(req), true);
 }
 
 TEST_F(ExtProcStreamTest, ReceiveFromStream) {
- auto stream = client_->start(*this, config_with_hash_key_, Http::AsyncClient::StreamOptions());
+ auto options = Http::AsyncClient::StreamOptions().setParentContext(stream_info_);
+ auto stream = client_->start(*this, config_with_hash_key_, options);
  ASSERT_NE(stream_callbacks_, nullptr);
  // Send something and ensure that we get it. Doesn't really matter what.
  ProcessingResponse resp;
@@ -134,7 +138,8 @@ TEST_F(ExtProcStreamTest, ReceiveFromStream) {
 }
 
 TEST_F(ExtProcStreamTest, StreamClosed) {
- auto stream = client_->start(*this, config_with_hash_key_, Http::AsyncClient::StreamOptions());
+ auto options = Http::AsyncClient::StreamOptions().setParentContext(stream_info_);
+ auto stream = client_->start(*this, config_with_hash_key_, options);
  ASSERT_NE(stream_callbacks_, nullptr);
  EXPECT_FALSE(last_response_);
  EXPECT_FALSE(grpc_closed_);
@@ -147,7 +152,8 @@ TEST_F(ExtProcStreamTest, StreamClosed) {
 }
 
 TEST_F(ExtProcStreamTest, StreamError) {
- auto stream = client_->start(*this, config_with_hash_key_, Http::AsyncClient::StreamOptions());
+ auto options = Http::AsyncClient::StreamOptions().setParentContext(stream_info_);
+ auto stream = client_->start(*this, config_with_hash_key_, options);
  ASSERT_NE(stream_callbacks_, nullptr);
  EXPECT_FALSE(last_response_);
  EXPECT_FALSE(grpc_closed_);