From cd2ae5e2f26f1e8902b1b603847f2314b914b2d1 Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Mon, 15 Jul 2024 18:04:02 +0000 Subject: [PATCH 1/9] test(AsyncRpcTracing) fix crashes in tracing wrappers for streams --- .../async_streaming_read_rpc_tracing.h | 10 ++++- .../async_streaming_read_rpc_tracing_test.cc | 37 +++++++++++++++++++ google/cloud/internal/grpc_opentelemetry.h | 2 +- .../cloud/internal/grpc_request_metadata.cc | 2 + 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing.h b/google/cloud/internal/async_streaming_read_rpc_tracing.h index 5bd1cdc956adf..dbb6d241b3976 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_read_rpc_tracing.h @@ -15,6 +15,7 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_ASYNC_STREAMING_READ_RPC_TRACING_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_ASYNC_STREAMING_READ_RPC_TRACING_H +#define GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #include "google/cloud/internal/async_streaming_read_rpc.h" #include "google/cloud/internal/grpc_opentelemetry.h" @@ -53,6 +54,7 @@ class AsyncStreamingReadRpcTracing : public AsyncStreamingReadRpc { EndSpan(*ss); auto started = f.get(); span_->SetAttribute("gl-cpp.stream_started", started); + this->started_ = started; return started; }); } @@ -89,13 +91,19 @@ class AsyncStreamingReadRpcTracing : public AsyncStreamingReadRpc { private: Status End(Status status) { if (!context_) return status; - return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); + if (started_) { + std::cout << "----------------stream started----------------" << std::endl; + return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); + } else { + return EndSpan(*std::move(span_), std::move(status)); + } } std::shared_ptr context_; std::unique_ptr> impl_; opentelemetry::nostd::shared_ptr span_; int read_count_ = 0; + bool started_ = false; }; } // namespace internal diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc index 9d1109e832228..162c076ccb7fd 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#define GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #include "google/cloud/internal/async_streaming_read_rpc_tracing.h" #include "google/cloud/internal/make_status.h" @@ -199,6 +200,42 @@ TEST(AsyncStreamingReadRpcTracing, SpanEndsOnDestruction) { EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); } +TEST(AsyncStreamingReadRpcTracing, UnstartedStreamShouldNotExtractMetadata) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + { + auto mock = std::make_unique(); + auto span = MakeSpan("span"); + auto context = std::make_shared(); + TestedStream stream(context, std::move(mock), span); + } + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); +} + +TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + { + auto mock = std::make_unique(); + EXPECT_CALL(*mock, Start).WillOnce([] { return make_ready_future(true); }); + EXPECT_CALL(*mock, Finish) + .WillOnce(Return(make_ready_future(internal::AbortedError("fail")))); + EXPECT_CALL(*mock, GetRequestMetadata) + .WillOnce(Return(RpcMetadata{{{"key", "value"}}, {}})); + + auto span = MakeSpan("span"); + auto context = std::make_shared(); + TestedStream stream(context, std::move(mock), span); + EXPECT_TRUE(stream.Start().get()); + std::cout << "-------------start to destruct stream-----------" << std::endl; + } + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/internal/grpc_opentelemetry.h b/google/cloud/internal/grpc_opentelemetry.h index 500e3c67c363f..e43e21c664bbd 100644 --- a/google/cloud/internal/grpc_opentelemetry.h +++ b/google/cloud/internal/grpc_opentelemetry.h @@ -33,7 +33,7 @@ namespace google { namespace cloud { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace internal { - +#define GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY /** diff --git a/google/cloud/internal/grpc_request_metadata.cc b/google/cloud/internal/grpc_request_metadata.cc index 8cecfd683c287..d8dd342c1902c 100644 --- a/google/cloud/internal/grpc_request_metadata.cc +++ b/google/cloud/internal/grpc_request_metadata.cc @@ -72,9 +72,11 @@ RpcMetadata GetRequestMetadataFromContext(grpc::ClientContext const& context, GrpcMetadataView view) { RpcMetadata metadata; AddContextMetadata(context, metadata); + std::cout << "1-111111111111111" << std::endl; if (view == GrpcMetadataView::kWithServerMetadata) { AddServerRequestMetadata(context, metadata); } + std::cout << "1-2222222222222222" << std::endl; return metadata; } From 67193ef768b1a41c562395a11deb1c72967f20d7 Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Mon, 15 Jul 2024 20:15:03 +0000 Subject: [PATCH 2/9] clean up debug logs --- .../async_streaming_read_rpc_tracing.h | 2 -- .../async_streaming_read_rpc_tracing_test.cc | 21 +++++++++---------- google/cloud/internal/grpc_opentelemetry.h | 1 - .../cloud/internal/grpc_request_metadata.cc | 2 -- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing.h b/google/cloud/internal/async_streaming_read_rpc_tracing.h index dbb6d241b3976..6adda8b98b9e0 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_read_rpc_tracing.h @@ -15,7 +15,6 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_ASYNC_STREAMING_READ_RPC_TRACING_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_ASYNC_STREAMING_READ_RPC_TRACING_H -#define GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #include "google/cloud/internal/async_streaming_read_rpc.h" #include "google/cloud/internal/grpc_opentelemetry.h" @@ -92,7 +91,6 @@ class AsyncStreamingReadRpcTracing : public AsyncStreamingReadRpc { Status End(Status status) { if (!context_) return status; if (started_) { - std::cout << "----------------stream started----------------" << std::endl; return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); } else { return EndSpan(*std::move(span_), std::move(status)); diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc index 162c076ccb7fd..c66def9021181 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -#define GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #include "google/cloud/internal/async_streaming_read_rpc_tracing.h" #include "google/cloud/internal/make_status.h" @@ -216,24 +215,24 @@ TEST(AsyncStreamingReadRpcTracing, UnstartedStreamShouldNotExtractMetadata) { TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { auto span_catcher = testing_util::InstallSpanCatcher(); - { - auto mock = std::make_unique(); - EXPECT_CALL(*mock, Start).WillOnce([] { return make_ready_future(true); }); - EXPECT_CALL(*mock, Finish) - .WillOnce(Return(make_ready_future(internal::AbortedError("fail")))); - EXPECT_CALL(*mock, GetRequestMetadata) - .WillOnce(Return(RpcMetadata{{{"key", "value"}}, {}})); - auto span = MakeSpan("span"); + auto mock = std::make_unique(); auto context = std::make_shared(); + EXPECT_CALL(*mock, Start).WillOnce([context] { + SetServerMetadata(*context, {}); + return make_ready_future(true); + }); + TestedStream stream(context, std::move(mock), span); EXPECT_TRUE(stream.Start().get()); - std::cout << "-------------start to destruct stream-----------" << std::endl; } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); + EXPECT_THAT( + spans, testing::UnorderedElementsAre( + AllOf(SpanNamed("Start")), + AllOf(SpanNamed("span")))); } } // namespace diff --git a/google/cloud/internal/grpc_opentelemetry.h b/google/cloud/internal/grpc_opentelemetry.h index e43e21c664bbd..7c2e17faccc20 100644 --- a/google/cloud/internal/grpc_opentelemetry.h +++ b/google/cloud/internal/grpc_opentelemetry.h @@ -33,7 +33,6 @@ namespace google { namespace cloud { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace internal { -#define GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY /** diff --git a/google/cloud/internal/grpc_request_metadata.cc b/google/cloud/internal/grpc_request_metadata.cc index d8dd342c1902c..8cecfd683c287 100644 --- a/google/cloud/internal/grpc_request_metadata.cc +++ b/google/cloud/internal/grpc_request_metadata.cc @@ -72,11 +72,9 @@ RpcMetadata GetRequestMetadataFromContext(grpc::ClientContext const& context, GrpcMetadataView view) { RpcMetadata metadata; AddContextMetadata(context, metadata); - std::cout << "1-111111111111111" << std::endl; if (view == GrpcMetadataView::kWithServerMetadata) { AddServerRequestMetadata(context, metadata); } - std::cout << "1-2222222222222222" << std::endl; return metadata; } From bd03c2960efcb1fb427b333701a3747deef70d1b Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Mon, 15 Jul 2024 20:23:03 +0000 Subject: [PATCH 3/9] clean up --- google/cloud/internal/grpc_opentelemetry.h | 1 + 1 file changed, 1 insertion(+) diff --git a/google/cloud/internal/grpc_opentelemetry.h b/google/cloud/internal/grpc_opentelemetry.h index 7c2e17faccc20..500e3c67c363f 100644 --- a/google/cloud/internal/grpc_opentelemetry.h +++ b/google/cloud/internal/grpc_opentelemetry.h @@ -33,6 +33,7 @@ namespace google { namespace cloud { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace internal { + #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY /** From a4f4594a832abc6f7c8d96e982c176651277ba5f Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Mon, 15 Jul 2024 21:26:25 +0000 Subject: [PATCH 4/9] add fix for write and readwrite streams --- .../async_read_write_stream_tracing.h | 9 ++++- .../async_read_write_stream_tracing_test.cc | 36 +++++++++++++++++++ .../async_streaming_read_rpc_tracing.h | 2 +- .../async_streaming_write_rpc_tracing.h | 23 +++++++----- .../async_streaming_write_rpc_tracing_test.cc | 36 +++++++++++++++++++ 5 files changed, 95 insertions(+), 11 deletions(-) diff --git a/google/cloud/internal/async_read_write_stream_tracing.h b/google/cloud/internal/async_read_write_stream_tracing.h index 799e411a6ceba..c00c8545a1f1e 100644 --- a/google/cloud/internal/async_read_write_stream_tracing.h +++ b/google/cloud/internal/async_read_write_stream_tracing.h @@ -54,6 +54,7 @@ class AsyncStreamingReadWriteRpcTracing EndSpan(*ss); auto started = f.get(); span_->SetAttribute("gl-cpp.stream_started", started); + started_ = started; return started; }); } @@ -112,7 +113,12 @@ class AsyncStreamingReadWriteRpcTracing private: Status End(Status status) { if (!context_) return status; - return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); + if (started_) { + return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); + } else { + return EndSpan(*std::move(span_), std::move(status)); + } + } std::shared_ptr context_; @@ -120,6 +126,7 @@ class AsyncStreamingReadWriteRpcTracing opentelemetry::nostd::shared_ptr span_; int read_count_ = 0; int write_count_ = 0; + bool started_ = false; }; } // namespace internal diff --git a/google/cloud/internal/async_read_write_stream_tracing_test.cc b/google/cloud/internal/async_read_write_stream_tracing_test.cc index d01fd589d8bc5..d6cc1e15dd70d 100644 --- a/google/cloud/internal/async_read_write_stream_tracing_test.cc +++ b/google/cloud/internal/async_read_write_stream_tracing_test.cc @@ -321,6 +321,42 @@ TEST(AsyncStreamingReadWriteRpcTracing, SpanEndsOnDestruction) { EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); } +TEST(AsyncStreamingReadWriteRpcTracing, UnstartedStreamShouldNotExtractMetadata) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + { + auto mock = std::make_unique(); + auto span = MakeSpan("span"); + auto context = std::make_shared(); + TestedStream stream(context, std::move(mock), span); + } + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); +} + +TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { + auto span_catcher = testing_util::InstallSpanCatcher(); + { + auto span = MakeSpan("span"); + auto mock = std::make_unique(); + auto context = std::make_shared(); + EXPECT_CALL(*mock, Start).WillOnce([context] { + SetServerMetadata(*context, {}); + return make_ready_future(true); + }); + + TestedStream stream(context, std::move(mock), span); + EXPECT_TRUE(stream.Start().get()); + } + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT( + spans, testing::UnorderedElementsAre( + AllOf(SpanNamed("Start")), + AllOf(SpanNamed("span")))); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing.h b/google/cloud/internal/async_streaming_read_rpc_tracing.h index 6adda8b98b9e0..a1d65d4015981 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_read_rpc_tracing.h @@ -53,7 +53,7 @@ class AsyncStreamingReadRpcTracing : public AsyncStreamingReadRpc { EndSpan(*ss); auto started = f.get(); span_->SetAttribute("gl-cpp.stream_started", started); - this->started_ = started; + started_ = started; return started; }); } diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing.h b/google/cloud/internal/async_streaming_write_rpc_tracing.h index e67096369d5b9..c3a8dc9a9e5f7 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_write_rpc_tracing.h @@ -36,11 +36,7 @@ class AsyncStreamingWriteRpcTracing : context_(std::move(context)), impl_(std::move(impl)), span_(std::move(span)) {} - ~AsyncStreamingWriteRpcTracing() override { - if (context_) { - (void)EndSpan(*std::move(context_), *std::move(span_), Status()); - } - } + ~AsyncStreamingWriteRpcTracing() override { (void)End(StatusOr()); } void Cancel() override { span_->AddEvent("gl-cpp.cancel"); @@ -58,6 +54,7 @@ class AsyncStreamingWriteRpcTracing EndSpan(*ss); auto started = f.get(); span_->SetAttribute("gl-cpp.stream_started", started); + started_ = started; return started; }); } @@ -94,10 +91,7 @@ class AsyncStreamingWriteRpcTracing return impl_->Finish().then( [this, fs = std::move(finish_span)](future> f) { EndSpan(*fs); - auto response = f.get(); - if (!context_) return response; - return EndSpan(*std::move(context_), *std::move(span_), - std::move(response)); + return End(f.get()); }); } @@ -106,10 +100,21 @@ class AsyncStreamingWriteRpcTracing } private: + StatusOr End(StatusOr status) { + if (!context_) return status; + if (started_) { + return EndSpan(*std::move(context_), *std::move(span_), + std::move(status)); + } else { + return EndSpan(*std::move(span_), std::move(status)); + } + } + std::shared_ptr context_; std::unique_ptr> impl_; opentelemetry::nostd::shared_ptr span_; int write_count_ = 0; + int started_ = false; }; } // namespace internal diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc index 31cb9911a2113..c0efb4e9bfe65 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc @@ -225,6 +225,42 @@ TEST(AsyncStreamingWriteRpcTracing, SpanEndsOnDestruction) { EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); } +TEST(AsyncStreamingWriteRpcTracing, UnstartedStreamShouldNotExtractMetadata) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + { + auto mock = std::make_unique(); + auto span = MakeSpan("span"); + auto context = std::make_shared(); + TestedStream stream(context, std::move(mock), span); + } + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); +} + +TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { + auto span_catcher = testing_util::InstallSpanCatcher(); + { + auto span = MakeSpan("span"); + auto mock = std::make_unique(); + auto context = std::make_shared(); + EXPECT_CALL(*mock, Start).WillOnce([context] { + SetServerMetadata(*context, {}); + return make_ready_future(true); + }); + + TestedStream stream(context, std::move(mock), span); + EXPECT_TRUE(stream.Start().get()); + } + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT( + spans, testing::UnorderedElementsAre( + AllOf(SpanNamed("Start")), + AllOf(SpanNamed("span")))); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END From eb9845e8aeaad36c0f410efdc963b723442b9eb1 Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Tue, 16 Jul 2024 17:04:13 +0000 Subject: [PATCH 5/9] fix unittests, integration tests, format --- .../golden_kitchen_sink_tracing_stub_test.cc | 9 +++-- .../async_read_write_stream_tracing.h | 7 ++-- .../async_read_write_stream_tracing_test.cc | 39 +++++++++++++++---- .../async_streaming_read_rpc_tracing.h | 6 +-- .../async_streaming_read_rpc_tracing_test.cc | 37 ++++++++++++++---- .../async_streaming_write_rpc_tracing.h | 7 ++-- .../async_streaming_write_rpc_tracing_test.cc | 34 +++++++++++++--- 7 files changed, 106 insertions(+), 33 deletions(-) diff --git a/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc b/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc index 9dcc6979eeb87..7c388c455cc46 100644 --- a/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc +++ b/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc @@ -324,6 +324,8 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingRead) { EXPECT_THAT(finish, StatusIs(StatusCode::kAborted)); auto spans = span_catcher->GetSpans(); + // Start() return false, the metadata will not be extracted when + // ending the span, so the span will not contain `grpc.peer` in the end. EXPECT_THAT( spans, Contains(AllOf( @@ -332,7 +334,6 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingRead) { "google.test.admin.database.v1.GoldenKitchenSink/StreamingRead"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("grpc.peer", _), OTelAttribute("gl-cpp.status_code", kErrorCode))))); } @@ -362,6 +363,8 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { EXPECT_THAT(finish, StatusIs(StatusCode::kAborted)); auto spans = span_catcher->GetSpans(); + // Start() return false, the metadata will not be extracted when + // ending the span, so the span will not contain `grpc.peer` in the end. EXPECT_THAT( spans, Contains(AllOf( @@ -370,7 +373,6 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { "google.test.admin.database.v1.GoldenKitchenSink/StreamingWrite"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("grpc.peer", _), OTelAttribute("gl-cpp.status_code", kErrorCode))))); } @@ -400,6 +402,8 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { EXPECT_THAT(finish, StatusIs(StatusCode::kAborted)); auto spans = span_catcher->GetSpans(); + // Start() return false, the metadata will not be extracted when + // ending the span, so the span will not contain `grpc.peer` in the end. EXPECT_THAT( spans, Contains(AllOf( @@ -408,7 +412,6 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { "StreamingReadWrite"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("grpc.peer", _), OTelAttribute("gl-cpp.status_code", kErrorCode))))); } diff --git a/google/cloud/internal/async_read_write_stream_tracing.h b/google/cloud/internal/async_read_write_stream_tracing.h index c00c8545a1f1e..30658d9312198 100644 --- a/google/cloud/internal/async_read_write_stream_tracing.h +++ b/google/cloud/internal/async_read_write_stream_tracing.h @@ -114,11 +114,10 @@ class AsyncStreamingReadWriteRpcTracing Status End(Status status) { if (!context_) return status; if (started_) { - return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); - } else { - return EndSpan(*std::move(span_), std::move(status)); + return EndSpan(*std::move(context_), *std::move(span_), + std::move(status)); } - + return EndSpan(*std::move(span_), std::move(status)); } std::shared_ptr context_; diff --git a/google/cloud/internal/async_read_write_stream_tracing_test.cc b/google/cloud/internal/async_read_write_stream_tracing_test.cc index d6cc1e15dd70d..98ebfec889848 100644 --- a/google/cloud/internal/async_read_write_stream_tracing_test.cc +++ b/google/cloud/internal/async_read_write_stream_tracing_test.cc @@ -261,7 +261,7 @@ TEST(AsyncStreamingReadWriteRpcTracing, WritesDone) { AllOf(SpanNamed("Finish"), SpanWithParent(span)))); } -TEST(AsyncStreamingReadWriteRpcTracing, Finish) { +TEST(AsyncStreamingReadWriteRpcTracing, FinishWithoutStart) { auto span_catcher = testing_util::InstallSpanCatcher(); auto span = MakeSpan("span"); @@ -272,6 +272,31 @@ TEST(AsyncStreamingReadWriteRpcTracing, Finish) { TestedStream stream(context(), std::move(mock), span); EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); + // The stream is not started, the metadata will not be extracted when + // ending the span, so the span will not contain `grpc.peer` in the end. + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, + testing::UnorderedElementsAre( + AllOf(SpanNamed("span"), + SpanWithStatus(opentelemetry::trace::StatusCode::kError, + "fail")), + AllOf(SpanNamed("Finish"), SpanWithParent(span)))); +} + +TEST(AsyncStreamingReadWriteRpcTracing, FinishWithStart) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + auto span = MakeSpan("span"); + auto mock = std::make_unique(); + + EXPECT_CALL(*mock, Start).WillOnce([] { return make_ready_future(true); }); + EXPECT_CALL(*mock, Finish) + .WillOnce(Return(make_ready_future(internal::AbortedError("fail")))); + + TestedStream stream(context(), std::move(mock), span); + EXPECT_TRUE(stream.Start().get()); + EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); + auto spans = span_catcher->GetSpans(); EXPECT_THAT( spans, @@ -280,7 +305,8 @@ TEST(AsyncStreamingReadWriteRpcTracing, Finish) { SpanNamed("span"), SpanHasAttributes(OTelAttribute("grpc.peer", _)), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail")), - AllOf(SpanNamed("Finish"), SpanWithParent(span)))); + AllOf(SpanNamed("Finish"), SpanWithParent(span)), + AllOf(SpanNamed("Start"), SpanWithParent(span)))); } TEST(AsyncStreamingReadWriteRpcTracing, GetRequestMetadata) { @@ -321,7 +347,8 @@ TEST(AsyncStreamingReadWriteRpcTracing, SpanEndsOnDestruction) { EXPECT_THAT(spans, ElementsAre(SpanNamed("span"))); } -TEST(AsyncStreamingReadWriteRpcTracing, UnstartedStreamShouldNotExtractMetadata) { +TEST(AsyncStreamingReadWriteRpcTracing, + UnstartedStreamShouldNotExtractMetadata) { auto span_catcher = testing_util::InstallSpanCatcher(); { @@ -351,10 +378,8 @@ TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT( - spans, testing::UnorderedElementsAre( - AllOf(SpanNamed("Start")), - AllOf(SpanNamed("span")))); + EXPECT_THAT(spans, testing::UnorderedElementsAre(AllOf(SpanNamed("Start")), + AllOf(SpanNamed("span")))); } } // namespace diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing.h b/google/cloud/internal/async_streaming_read_rpc_tracing.h index a1d65d4015981..12fa088549e83 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_read_rpc_tracing.h @@ -91,10 +91,10 @@ class AsyncStreamingReadRpcTracing : public AsyncStreamingReadRpc { Status End(Status status) { if (!context_) return status; if (started_) { - return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); - } else { - return EndSpan(*std::move(span_), std::move(status)); + return EndSpan(*std::move(context_), *std::move(span_), + std::move(status)); } + return EndSpan(*std::move(span_), std::move(status)); } std::shared_ptr context_; diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc index c66def9021181..60a1c77e5ed35 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc @@ -150,7 +150,7 @@ TEST(AsyncStreamingReadRpcTracing, Read) { AllOf(SpanNamed("Finish"), SpanWithParent(span)))); } -TEST(AsyncStreamingReadRpcTracing, Finish) { +TEST(AsyncStreamingReadRpcTracing, FinishWithoutStart) { auto span_catcher = testing_util::InstallSpanCatcher(); auto span = MakeSpan("span"); @@ -161,6 +161,30 @@ TEST(AsyncStreamingReadRpcTracing, Finish) { TestedStream stream(context(), std::move(mock), span); EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); + // The stream is not started, the metadata will not be extracted when + // ending the span, so the span will not contain `grpc.peer` in the end. + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, + testing::UnorderedElementsAre( + AllOf(SpanNamed("span"), + SpanWithStatus(opentelemetry::trace::StatusCode::kError, + "fail")), + AllOf(SpanNamed("Finish"), SpanWithParent(span)))); +} + +TEST(AsyncStreamingReadRpcTracing, FinishWithStart) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + auto span = MakeSpan("span"); + auto mock = std::make_unique(); + EXPECT_CALL(*mock, Start).WillOnce([] { return make_ready_future(true); }); + EXPECT_CALL(*mock, Finish) + .WillOnce(Return(make_ready_future(internal::AbortedError("fail")))); + + TestedStream stream(context(), std::move(mock), span); + EXPECT_TRUE(stream.Start().get()); + EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); + auto spans = span_catcher->GetSpans(); EXPECT_THAT( spans, @@ -169,7 +193,8 @@ TEST(AsyncStreamingReadRpcTracing, Finish) { SpanNamed("span"), SpanHasAttributes(OTelAttribute("grpc.peer", _)), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail")), - AllOf(SpanNamed("Finish"), SpanWithParent(span)))); + AllOf(SpanNamed("Finish"), SpanWithParent(span)), + AllOf(SpanNamed("Start"), SpanWithParent(span)))); } TEST(AsyncStreamingReadRpcTracing, GetRequestMetadata) { @@ -223,16 +248,14 @@ TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { SetServerMetadata(*context, {}); return make_ready_future(true); }); - + TestedStream stream(context, std::move(mock), span); EXPECT_TRUE(stream.Start().get()); } auto spans = span_catcher->GetSpans(); - EXPECT_THAT( - spans, testing::UnorderedElementsAre( - AllOf(SpanNamed("Start")), - AllOf(SpanNamed("span")))); + EXPECT_THAT(spans, testing::UnorderedElementsAre(AllOf(SpanNamed("Start")), + AllOf(SpanNamed("span")))); } } // namespace diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing.h b/google/cloud/internal/async_streaming_write_rpc_tracing.h index c3a8dc9a9e5f7..9c71d950b699a 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_write_rpc_tracing.h @@ -104,17 +104,16 @@ class AsyncStreamingWriteRpcTracing if (!context_) return status; if (started_) { return EndSpan(*std::move(context_), *std::move(span_), - std::move(status)); - } else { - return EndSpan(*std::move(span_), std::move(status)); + std::move(status)); } + return EndSpan(*std::move(span_), std::move(status)); } std::shared_ptr context_; std::unique_ptr> impl_; opentelemetry::nostd::shared_ptr span_; int write_count_ = 0; - int started_ = false; + bool started_ = false; }; } // namespace internal diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc index c0efb4e9bfe65..6eb0e21cf3a41 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc @@ -175,16 +175,41 @@ TEST(AsyncStreamingWriteRpcTracing, WritesDone) { AllOf(SpanNamed("Finish"), SpanWithParent(span)))); } +TEST(AsyncStreamingWriteRpcTracing, FinishWithoutStart) { + auto span_catcher = testing_util::InstallSpanCatcher(); + + auto span = MakeSpan("span"); + auto mock = std::make_unique(); + EXPECT_CALL(*mock, Finish) + .WillOnce(Return( + make_ready_future>(internal::AbortedError("fail")))); + + TestedStream stream(context(), std::move(mock), span); + EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); + + // The stream is not started, the metadata will not be extracted when + // ending the span, so the span will not contain `grpc.peer` in the end. + auto spans = span_catcher->GetSpans(); + EXPECT_THAT(spans, + testing::UnorderedElementsAre( + AllOf(SpanNamed("span"), + SpanWithStatus(opentelemetry::trace::StatusCode::kError, + "fail")), + AllOf(SpanNamed("Finish"), SpanWithParent(span)))); +} + TEST(AsyncStreamingWriteRpcTracing, Finish) { auto span_catcher = testing_util::InstallSpanCatcher(); auto span = MakeSpan("span"); auto mock = std::make_unique(); + EXPECT_CALL(*mock, Start).WillOnce([] { return make_ready_future(true); }); EXPECT_CALL(*mock, Finish) .WillOnce(Return( make_ready_future>(internal::AbortedError("fail")))); TestedStream stream(context(), std::move(mock), span); + EXPECT_TRUE(stream.Start().get()); EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); auto spans = span_catcher->GetSpans(); @@ -195,7 +220,8 @@ TEST(AsyncStreamingWriteRpcTracing, Finish) { SpanNamed("span"), SpanHasAttributes(OTelAttribute("grpc.peer", _)), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail")), - AllOf(SpanNamed("Finish"), SpanWithParent(span)))); + AllOf(SpanNamed("Finish"), SpanWithParent(span)), + AllOf(SpanNamed("Start"), SpanWithParent(span)))); } TEST(AsyncStreamingWriteRpcTracing, GetRequestMetadata) { @@ -255,10 +281,8 @@ TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT( - spans, testing::UnorderedElementsAre( - AllOf(SpanNamed("Start")), - AllOf(SpanNamed("span")))); + EXPECT_THAT(spans, testing::UnorderedElementsAre(AllOf(SpanNamed("Start")), + AllOf(SpanNamed("span")))); } } // namespace From 0f905d55c42413e3dc16f10e650447d2564c0ab1 Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Tue, 16 Jul 2024 21:44:24 +0000 Subject: [PATCH 6/9] nit and fix --- .../golden_kitchen_sink_tracing_stub_test.cc | 15 ++++++-------- .../async_read_write_stream_tracing.h | 2 +- .../async_read_write_stream_tracing_test.cc | 18 ++++++++++------- .../async_streaming_read_rpc_tracing.h | 2 +- .../async_streaming_read_rpc_tracing_test.cc | 18 ++++++++++------- .../async_streaming_write_rpc_tracing.h | 6 ++++-- .../async_streaming_write_rpc_tracing_test.cc | 20 +++++++++++-------- 7 files changed, 46 insertions(+), 35 deletions(-) diff --git a/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc b/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc index 7c388c455cc46..edc3e2c9ad8f5 100644 --- a/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc +++ b/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc @@ -324,8 +324,6 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingRead) { EXPECT_THAT(finish, StatusIs(StatusCode::kAborted)); auto spans = span_catcher->GetSpans(); - // Start() return false, the metadata will not be extracted when - // ending the span, so the span will not contain `grpc.peer` in the end. EXPECT_THAT( spans, Contains(AllOf( @@ -334,7 +332,8 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingRead) { "google.test.admin.database.v1.GoldenKitchenSink/StreamingRead"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("gl-cpp.status_code", kErrorCode))))); + OTelAttribute("gl-cpp.status_code", kErrorCode)), + Not(SpanHasAttributes(OTelAttribute("grpc.peer", _)))))); } TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { @@ -363,8 +362,6 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { EXPECT_THAT(finish, StatusIs(StatusCode::kAborted)); auto spans = span_catcher->GetSpans(); - // Start() return false, the metadata will not be extracted when - // ending the span, so the span will not contain `grpc.peer` in the end. EXPECT_THAT( spans, Contains(AllOf( @@ -373,7 +370,8 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { "google.test.admin.database.v1.GoldenKitchenSink/StreamingWrite"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("gl-cpp.status_code", kErrorCode))))); + OTelAttribute("gl-cpp.status_code", kErrorCode)), + Not(SpanHasAttributes(OTelAttribute("grpc.peer", _)))))); } TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { @@ -402,8 +400,6 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { EXPECT_THAT(finish, StatusIs(StatusCode::kAborted)); auto spans = span_catcher->GetSpans(); - // Start() return false, the metadata will not be extracted when - // ending the span, so the span will not contain `grpc.peer` in the end. EXPECT_THAT( spans, Contains(AllOf( @@ -412,7 +408,8 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { "StreamingReadWrite"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("gl-cpp.status_code", kErrorCode))))); + OTelAttribute("gl-cpp.status_code", kErrorCode)), + Not(SpanHasAttributes(OTelAttribute("grpc.peer", _)))))); } TEST(GoldenKitchenSinkTracingStubTest, ExplicitRouting1) { diff --git a/google/cloud/internal/async_read_write_stream_tracing.h b/google/cloud/internal/async_read_write_stream_tracing.h index 30658d9312198..8d6be0a09541d 100644 --- a/google/cloud/internal/async_read_write_stream_tracing.h +++ b/google/cloud/internal/async_read_write_stream_tracing.h @@ -112,7 +112,7 @@ class AsyncStreamingReadWriteRpcTracing private: Status End(Status status) { - if (!context_) return status; + if (!span_) return status; if (started_) { return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); diff --git a/google/cloud/internal/async_read_write_stream_tracing_test.cc b/google/cloud/internal/async_read_write_stream_tracing_test.cc index 98ebfec889848..bf008b611213d 100644 --- a/google/cloud/internal/async_read_write_stream_tracing_test.cc +++ b/google/cloud/internal/async_read_write_stream_tracing_test.cc @@ -272,14 +272,14 @@ TEST(AsyncStreamingReadWriteRpcTracing, FinishWithoutStart) { TestedStream stream(context(), std::move(mock), span); EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); - // The stream is not started, the metadata will not be extracted when - // ending the span, so the span will not contain `grpc.peer` in the end. auto spans = span_catcher->GetSpans(); EXPECT_THAT(spans, - testing::UnorderedElementsAre( + UnorderedElementsAre( AllOf(SpanNamed("span"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, - "fail")), + "fail"), + Not(SpanHasAttributes( + OTelAttribute("grpc.peer", _)))), AllOf(SpanNamed("Finish"), SpanWithParent(span)))); } @@ -369,7 +369,7 @@ TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { auto mock = std::make_unique(); auto context = std::make_shared(); EXPECT_CALL(*mock, Start).WillOnce([context] { - SetServerMetadata(*context, {}); + SetServerMetadata(*context, RpcMetadata{{{"key", "value"}}}); return make_ready_future(true); }); @@ -378,8 +378,12 @@ TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, testing::UnorderedElementsAre(AllOf(SpanNamed("Start")), - AllOf(SpanNamed("span")))); + EXPECT_THAT(spans, + UnorderedElementsAre( + SpanNamed("Start"), + AllOf(SpanNamed("span"), + SpanHasAttributes(OTelAttribute( + "rpc.grpc.response.metadata.key", "value"))))); } } // namespace diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing.h b/google/cloud/internal/async_streaming_read_rpc_tracing.h index 12fa088549e83..71e77f2d02732 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_read_rpc_tracing.h @@ -89,7 +89,7 @@ class AsyncStreamingReadRpcTracing : public AsyncStreamingReadRpc { private: Status End(Status status) { - if (!context_) return status; + if (!span_) return status; if (started_) { return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc index 60a1c77e5ed35..d19ce344ff60e 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc @@ -161,14 +161,14 @@ TEST(AsyncStreamingReadRpcTracing, FinishWithoutStart) { TestedStream stream(context(), std::move(mock), span); EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); - // The stream is not started, the metadata will not be extracted when - // ending the span, so the span will not contain `grpc.peer` in the end. auto spans = span_catcher->GetSpans(); EXPECT_THAT(spans, - testing::UnorderedElementsAre( + UnorderedElementsAre( AllOf(SpanNamed("span"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, - "fail")), + "fail"), + Not(SpanHasAttributes( + OTelAttribute("grpc.peer", _)))), AllOf(SpanNamed("Finish"), SpanWithParent(span)))); } @@ -245,7 +245,7 @@ TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { auto mock = std::make_unique(); auto context = std::make_shared(); EXPECT_CALL(*mock, Start).WillOnce([context] { - SetServerMetadata(*context, {}); + SetServerMetadata(*context, RpcMetadata{{{"key", "value"}}}); return make_ready_future(true); }); @@ -254,8 +254,12 @@ TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, testing::UnorderedElementsAre(AllOf(SpanNamed("Start")), - AllOf(SpanNamed("span")))); + EXPECT_THAT(spans, + UnorderedElementsAre( + SpanNamed("Start"), + AllOf(SpanNamed("span"), + SpanHasAttributes(OTelAttribute( + "rpc.grpc.response.metadata.key", "value"))))); } } // namespace diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing.h b/google/cloud/internal/async_streaming_write_rpc_tracing.h index 9c71d950b699a..e962f7ac95c41 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing.h +++ b/google/cloud/internal/async_streaming_write_rpc_tracing.h @@ -36,7 +36,9 @@ class AsyncStreamingWriteRpcTracing : context_(std::move(context)), impl_(std::move(impl)), span_(std::move(span)) {} - ~AsyncStreamingWriteRpcTracing() override { (void)End(StatusOr()); } + ~AsyncStreamingWriteRpcTracing() override { + (void)End(make_status_or({})); + } void Cancel() override { span_->AddEvent("gl-cpp.cancel"); @@ -101,7 +103,7 @@ class AsyncStreamingWriteRpcTracing private: StatusOr End(StatusOr status) { - if (!context_) return status; + if (!span_) return status; if (started_) { return EndSpan(*std::move(context_), *std::move(span_), std::move(status)); diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc index 6eb0e21cf3a41..dda8bdbfe6610 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc @@ -187,18 +187,18 @@ TEST(AsyncStreamingWriteRpcTracing, FinishWithoutStart) { TestedStream stream(context(), std::move(mock), span); EXPECT_THAT(stream.Finish().get(), StatusIs(StatusCode::kAborted, "fail")); - // The stream is not started, the metadata will not be extracted when - // ending the span, so the span will not contain `grpc.peer` in the end. auto spans = span_catcher->GetSpans(); EXPECT_THAT(spans, - testing::UnorderedElementsAre( + UnorderedElementsAre( AllOf(SpanNamed("span"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, - "fail")), + "fail"), + Not(SpanHasAttributes( + OTelAttribute("grpc.peer", _)))), AllOf(SpanNamed("Finish"), SpanWithParent(span)))); } -TEST(AsyncStreamingWriteRpcTracing, Finish) { +TEST(AsyncStreamingWriteRpcTracing, FinishWithStart) { auto span_catcher = testing_util::InstallSpanCatcher(); auto span = MakeSpan("span"); @@ -272,7 +272,7 @@ TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { auto mock = std::make_unique(); auto context = std::make_shared(); EXPECT_CALL(*mock, Start).WillOnce([context] { - SetServerMetadata(*context, {}); + SetServerMetadata(*context, RpcMetadata{{{"key", "value"}}}); return make_ready_future(true); }); @@ -281,8 +281,12 @@ TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, testing::UnorderedElementsAre(AllOf(SpanNamed("Start")), - AllOf(SpanNamed("span")))); + EXPECT_THAT(spans, + UnorderedElementsAre( + SpanNamed("Start"), + AllOf(SpanNamed("span"), + SpanHasAttributes(OTelAttribute( + "rpc.grpc.response.metadata.key", "value"))))); } } // namespace From e5f8569604cf65322cdc4650ee3984255eac6f27 Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Tue, 16 Jul 2024 22:47:22 +0000 Subject: [PATCH 7/9] clang-tidy --- google/cloud/internal/async_read_write_stream_tracing_test.cc | 4 ++-- .../cloud/internal/async_streaming_read_rpc_tracing_test.cc | 4 ++-- .../cloud/internal/async_streaming_write_rpc_tracing_test.cc | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/google/cloud/internal/async_read_write_stream_tracing_test.cc b/google/cloud/internal/async_read_write_stream_tracing_test.cc index bf008b611213d..613954657f288 100644 --- a/google/cloud/internal/async_read_write_stream_tracing_test.cc +++ b/google/cloud/internal/async_read_write_stream_tracing_test.cc @@ -369,7 +369,7 @@ TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { auto mock = std::make_unique(); auto context = std::make_shared(); EXPECT_CALL(*mock, Start).WillOnce([context] { - SetServerMetadata(*context, RpcMetadata{{{"key", "value"}}}); + SetServerMetadata(*context, RpcMetadata{{{"hk", "hv"}}, {{"tk", "tv"}}}); return make_ready_future(true); }); @@ -383,7 +383,7 @@ TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { SpanNamed("Start"), AllOf(SpanNamed("span"), SpanHasAttributes(OTelAttribute( - "rpc.grpc.response.metadata.key", "value"))))); + "rpc.grpc.response.metadata.hk", "hv"))))); } } // namespace diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc index d19ce344ff60e..af3ec52173d8a 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc @@ -245,7 +245,7 @@ TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { auto mock = std::make_unique(); auto context = std::make_shared(); EXPECT_CALL(*mock, Start).WillOnce([context] { - SetServerMetadata(*context, RpcMetadata{{{"key", "value"}}}); + SetServerMetadata(*context, RpcMetadata{{{"hk", "hv"}}, {{"tk", "tv"}}}); return make_ready_future(true); }); @@ -259,7 +259,7 @@ TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { SpanNamed("Start"), AllOf(SpanNamed("span"), SpanHasAttributes(OTelAttribute( - "rpc.grpc.response.metadata.key", "value"))))); + "rpc.grpc.response.metadata.hk", "hv"))))); } } // namespace diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc index dda8bdbfe6610..5b51f5d4ab732 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc @@ -272,7 +272,7 @@ TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { auto mock = std::make_unique(); auto context = std::make_shared(); EXPECT_CALL(*mock, Start).WillOnce([context] { - SetServerMetadata(*context, RpcMetadata{{{"key", "value"}}}); + SetServerMetadata(*context, RpcMetadata{{{"hk", "hv"}}, {{"tk", "tv"}}}); return make_ready_future(true); }); @@ -286,7 +286,7 @@ TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { SpanNamed("Start"), AllOf(SpanNamed("span"), SpanHasAttributes(OTelAttribute( - "rpc.grpc.response.metadata.key", "value"))))); + "rpc.grpc.response.metadata.hk", "hv"))))); } } // namespace From eab9ede8a8ef0600ada48e45d206743aebc24cb6 Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Tue, 16 Jul 2024 23:33:44 +0000 Subject: [PATCH 8/9] checkers --- .../internal/async_read_write_stream_tracing_test.cc | 11 +++++------ .../internal/async_streaming_read_rpc_tracing_test.cc | 11 +++++------ .../async_streaming_write_rpc_tracing_test.cc | 11 +++++------ 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/google/cloud/internal/async_read_write_stream_tracing_test.cc b/google/cloud/internal/async_read_write_stream_tracing_test.cc index 613954657f288..8476e4b649fa4 100644 --- a/google/cloud/internal/async_read_write_stream_tracing_test.cc +++ b/google/cloud/internal/async_read_write_stream_tracing_test.cc @@ -378,12 +378,11 @@ TEST(AsyncStreamingReadWriteRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, - UnorderedElementsAre( - SpanNamed("Start"), - AllOf(SpanNamed("span"), - SpanHasAttributes(OTelAttribute( - "rpc.grpc.response.metadata.hk", "hv"))))); + EXPECT_THAT(spans, UnorderedElementsAre( + SpanNamed("Start"), + AllOf(SpanNamed("span"), + SpanHasAttributes(OTelAttribute( + "rpc.grpc.response.metadata.hk", "hv"))))); } } // namespace diff --git a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc index af3ec52173d8a..8879d4882af90 100644 --- a/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_read_rpc_tracing_test.cc @@ -254,12 +254,11 @@ TEST(AsyncStreamingReadRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, - UnorderedElementsAre( - SpanNamed("Start"), - AllOf(SpanNamed("span"), - SpanHasAttributes(OTelAttribute( - "rpc.grpc.response.metadata.hk", "hv"))))); + EXPECT_THAT(spans, UnorderedElementsAre( + SpanNamed("Start"), + AllOf(SpanNamed("span"), + SpanHasAttributes(OTelAttribute( + "rpc.grpc.response.metadata.hk", "hv"))))); } } // namespace diff --git a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc index 5b51f5d4ab732..f7906db2982c5 100644 --- a/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc +++ b/google/cloud/internal/async_streaming_write_rpc_tracing_test.cc @@ -281,12 +281,11 @@ TEST(AsyncStreamingWriteRpcTracing, StartedStreamShouldExtractMetadata) { } auto spans = span_catcher->GetSpans(); - EXPECT_THAT(spans, - UnorderedElementsAre( - SpanNamed("Start"), - AllOf(SpanNamed("span"), - SpanHasAttributes(OTelAttribute( - "rpc.grpc.response.metadata.hk", "hv"))))); + EXPECT_THAT(spans, UnorderedElementsAre( + SpanNamed("Start"), + AllOf(SpanNamed("span"), + SpanHasAttributes(OTelAttribute( + "rpc.grpc.response.metadata.hk", "hv"))))); } } // namespace From a7abbd88a2ba7dcdfa637233d9fb1d539c68ddce Mon Sep 17 00:00:00 2001 From: Yao Cui Date: Wed, 17 Jul 2024 14:56:30 +0000 Subject: [PATCH 9/9] remove no attribute expectation --- .../tests/golden_kitchen_sink_tracing_stub_test.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc b/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc index edc3e2c9ad8f5..89df2e797df03 100644 --- a/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc +++ b/generator/integration_tests/tests/golden_kitchen_sink_tracing_stub_test.cc @@ -332,8 +332,7 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingRead) { "google.test.admin.database.v1.GoldenKitchenSink/StreamingRead"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("gl-cpp.status_code", kErrorCode)), - Not(SpanHasAttributes(OTelAttribute("grpc.peer", _)))))); + OTelAttribute("gl-cpp.status_code", kErrorCode))))); } TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { @@ -370,8 +369,7 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingWrite) { "google.test.admin.database.v1.GoldenKitchenSink/StreamingWrite"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("gl-cpp.status_code", kErrorCode)), - Not(SpanHasAttributes(OTelAttribute("grpc.peer", _)))))); + OTelAttribute("gl-cpp.status_code", kErrorCode))))); } TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { @@ -408,8 +406,7 @@ TEST(GoldenKitchenSinkTracingStubTest, AsyncStreamingReadWrite) { "StreamingReadWrite"), SpanWithStatus(opentelemetry::trace::StatusCode::kError, "fail"), SpanHasAttributes( - OTelAttribute("gl-cpp.status_code", kErrorCode)), - Not(SpanHasAttributes(OTelAttribute("grpc.peer", _)))))); + OTelAttribute("gl-cpp.status_code", kErrorCode))))); } TEST(GoldenKitchenSinkTracingStubTest, ExplicitRouting1) {