From 32fdac9641ee6b1e09bd3fadee675fe5d3d5f45e Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 12 Jun 2025 11:19:13 +0000 Subject: [PATCH 1/8] Add check for writeHandle before reansforming write_object_spec to append_object_spec --- google/cloud/storage/internal/async/handle_redirect_error.cc | 4 ++-- .../storage/internal/async/handle_redirect_error_test.cc | 4 +++- google/cloud/storage/internal/async/writer_connection_impl.cc | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/internal/async/handle_redirect_error.cc b/google/cloud/storage/internal/async/handle_redirect_error.cc index 1444a8bda6f19..a4a9afc1efe2c 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error.cc @@ -23,10 +23,10 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN void EnsureFirstMessageAppendObjectSpec( google::storage::v2::BidiWriteObjectRequest& request, google::rpc::Status const& rpc_status) { - for (auto const& rpc_status_detail : rpc_status.details()) { + for (auto const& any : rpc_status.details()) { google::storage::v2::BidiWriteObjectRedirectedError error = google::storage::v2::BidiWriteObjectRedirectedError{}; - if (!rpc_status_detail.UnpackTo(&error)) continue; + if (!any.UnpackTo(&error)) continue; if (!error.has_write_handle()) continue; if (request.has_write_object_spec()) { auto spec = request.write_object_spec(); diff --git a/google/cloud/storage/internal/async/handle_redirect_error_test.cc b/google/cloud/storage/internal/async/handle_redirect_error_test.cc index 0324afd1fb069..99c3486260f46 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error_test.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error_test.cc @@ -75,7 +75,7 @@ TEST(EnsureFirstMessageAppendObjectSpec, Success) { ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( R"pb( write_object_spec { - resource { bucket: "projects/_/buckets/b", name: "o" } + resource { bucket: "projects/_/buckets/b", name: "o" } if_metageneration_match: 1 if_metageneration_not_match: 1 } @@ -124,6 +124,8 @@ TEST(EnsureFirstMessageAppendObjectSpec, WriteHandleIsNotSet) { EXPECT_FALSE(request.has_append_object_spec()); } + + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal diff --git a/google/cloud/storage/internal/async/writer_connection_impl.cc b/google/cloud/storage/internal/async/writer_connection_impl.cc index 56fefa762fba0..018248391da15 100644 --- a/google/cloud/storage/internal/async/writer_connection_impl.cc +++ b/google/cloud/storage/internal/async/writer_connection_impl.cc @@ -234,7 +234,8 @@ future> AsyncWriterConnectionImpl::OnQuery( "Expected error in Finish() after non-ok Read()")) .then([this](auto g) { auto result = g.get(); - google::rpc::Status grpc_status = ExtractGrpcStatus(result); + google::rpc::Status grpc_status = + ExtractGrpcStatus(result); EnsureFirstMessageAppendObjectSpec(request_, grpc_status); ApplyWriteRedirectErrors(*request_.mutable_append_object_spec(), grpc_status); From 5278143d299e4f0278fba5b6e1f3c4ac34fda3cf Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Mon, 16 Jun 2025 04:12:42 +0000 Subject: [PATCH 2/8] Correct the formatting --- .../storage/internal/async/handle_redirect_error_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/google/cloud/storage/internal/async/handle_redirect_error_test.cc b/google/cloud/storage/internal/async/handle_redirect_error_test.cc index 99c3486260f46..0324afd1fb069 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error_test.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error_test.cc @@ -75,7 +75,7 @@ TEST(EnsureFirstMessageAppendObjectSpec, Success) { ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( R"pb( write_object_spec { - resource { bucket: "projects/_/buckets/b", name: "o" } + resource { bucket: "projects/_/buckets/b", name: "o" } if_metageneration_match: 1 if_metageneration_not_match: 1 } @@ -124,8 +124,6 @@ TEST(EnsureFirstMessageAppendObjectSpec, WriteHandleIsNotSet) { EXPECT_FALSE(request.has_append_object_spec()); } - - } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal From da838174c45364764b87863f786468b3378ef1a4 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Tue, 17 Jun 2025 04:07:11 +0000 Subject: [PATCH 3/8] rename the any variable to rpc_status_detail --- google/cloud/storage/internal/async/handle_redirect_error.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/async/handle_redirect_error.cc b/google/cloud/storage/internal/async/handle_redirect_error.cc index a4a9afc1efe2c..1444a8bda6f19 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error.cc @@ -23,10 +23,10 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN void EnsureFirstMessageAppendObjectSpec( google::storage::v2::BidiWriteObjectRequest& request, google::rpc::Status const& rpc_status) { - for (auto const& any : rpc_status.details()) { + for (auto const& rpc_status_detail : rpc_status.details()) { google::storage::v2::BidiWriteObjectRedirectedError error = google::storage::v2::BidiWriteObjectRedirectedError{}; - if (!any.UnpackTo(&error)) continue; + if (!rpc_status_detail.UnpackTo(&error)) continue; if (!error.has_write_handle()) continue; if (request.has_write_object_spec()) { auto spec = request.write_object_spec(); From 6331c10f7c052afaa44500f3bf6d546226fdb004 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Tue, 17 Jun 2025 05:27:47 +0000 Subject: [PATCH 4/8] Format write_connection_impl.cc file --- google/cloud/storage/internal/async/writer_connection_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/async/writer_connection_impl.cc b/google/cloud/storage/internal/async/writer_connection_impl.cc index 018248391da15..56fefa762fba0 100644 --- a/google/cloud/storage/internal/async/writer_connection_impl.cc +++ b/google/cloud/storage/internal/async/writer_connection_impl.cc @@ -234,8 +234,7 @@ future> AsyncWriterConnectionImpl::OnQuery( "Expected error in Finish() after non-ok Read()")) .then([this](auto g) { auto result = g.get(); - google::rpc::Status grpc_status = - ExtractGrpcStatus(result); + google::rpc::Status grpc_status = ExtractGrpcStatus(result); EnsureFirstMessageAppendObjectSpec(request_, grpc_status); ApplyWriteRedirectErrors(*request_.mutable_append_object_spec(), grpc_status); From f4a44d8369d373be80bb2aef5ba399c101eb701b Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 25 Jun 2025 12:14:10 +0000 Subject: [PATCH 5/8] feat(storage): Link traces between Open and ReadRange traces. --- .../object_descriptor_connection_tracing.cc | 5 +- ...ject_descriptor_connection_tracing_test.cc | 73 +++++++++++++++++++ .../async/object_descriptor_reader_tracing.cc | 19 +---- 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc index 8f5e71fcddfb5..8e139f366e61b 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/storage/internal/async/object_descriptor_connection_tracing.h" +#include "google/cloud/storage/internal/async/reader_connection_tracing.h" #include "google/cloud/storage/async/reader_connection.h" #include "google/cloud/internal/opentelemetry.h" #include "google/cloud/version.h" @@ -58,7 +59,9 @@ class AsyncObjectDescriptorConnectionTracing {{sc::kThreadId, internal::CurrentThreadId()}, {"read-start", p.start}, {"read-length", p.length}}); - return result; + return MakeTracingReaderConnection( + span_, + std::move(result)); } private: diff --git a/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc b/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc index fbecb948f7dc3..3a32ad7eb8c0d 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc @@ -36,9 +36,13 @@ using ReadResponse = using ::google::cloud::storage_experimental::ObjectDescriptorConnection; using ::google::cloud::storage_mocks::MockAsyncObjectDescriptorConnection; using ::google::cloud::storage_mocks::MockAsyncReaderConnection; +using ::google::cloud::storage_experimental::ReadPayload; using ::google::cloud::testing_util::EventNamed; using ::google::cloud::testing_util::InstallSpanCatcher; using ::google::cloud::testing_util::OTelAttribute; +using ::google::cloud::testing_util::OTelContextCaptured; +using ::google::cloud::testing_util::ThereIsAnActiveSpan; +using ::google::cloud::testing_util::PromiseWithOTelContext; using ::google::cloud::testing_util::SpanEventAttributesAre; using ::google::cloud::testing_util::SpanHasInstrumentationScope; using ::google::cloud::testing_util::SpanKindIsClient; @@ -46,6 +50,25 @@ using ::google::cloud::testing_util::SpanNamed; using ::google::cloud::testing_util::SpanWithStatus; using ::testing::_; +// A helper to set expectations on a mock async reader. It captures the OTel +// context and returns a future that can be controlled by the test. +auto expect_context = [](auto& p) { + return [&p] { + EXPECT_TRUE(ThereIsAnActiveSpan()); + EXPECT_TRUE(OTelContextCaptured()); + return p.get_future(); + }; +}; + +// A helper to be used in a `.then()` clause. It verifies the OTel context +// has been detached before the user receives the result. +auto expect_no_context = [](auto f) { + auto t = f.get(); + EXPECT_FALSE(ThereIsAnActiveSpan()); + EXPECT_FALSE(OTelContextCaptured()); + return t; +}; + TEST(ObjectDescriptorConnectionTracing, Read) { namespace sc = ::opentelemetry::trace::SemanticConventions; auto span_catcher = InstallSpanCatcher(); @@ -76,6 +99,56 @@ TEST(ObjectDescriptorConnectionTracing, Read) { OTelAttribute(sc::kThreadId, _))))))); } +TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { + namespace sc = ::opentelemetry::trace::SemanticConventions; + auto span_catcher = InstallSpanCatcher(); + + auto mock_connection = std::make_shared(); + auto* mock_reader_ptr = new MockAsyncReaderConnection; + PromiseWithOTelContext p; + EXPECT_CALL(*mock_reader_ptr, Read).WillOnce(expect_context(p)); + + EXPECT_CALL(*mock_connection, Read) + .WillOnce([&](ObjectDescriptorConnection::ReadParams) { + return std::unique_ptr( + mock_reader_ptr); + }); + + auto connection = MakeTracingObjectDescriptorConnection( + internal::MakeSpan("test-span"), std::move(mock_connection)); + + auto reader = connection->Read({}); + auto f = reader->Read().then(expect_no_context); + p.set_value(ReadPayload("test-payload").set_offset(123)); + (void)f.get(); + + connection.reset(); // End the span + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT( + spans, + ElementsAre(AllOf( + SpanNamed("test-span"), + SpanWithStatus(opentelemetry::trace::StatusCode::kOk), + SpanHasInstrumentationScope(), SpanKindIsClient(), + SpanHasEvents( + AllOf(EventNamed("gl-cpp.open.read"), + SpanEventAttributesAre( + OTelAttribute("read-length", 0), + OTelAttribute("read-start", 0), + OTelAttribute(sc::kThreadId, _))), + AllOf( + EventNamed("gl-cpp.read"), + SpanEventAttributesAre( + OTelAttribute("message.starting_offset", + 123), + OTelAttribute(sc::kThreadId, _), + OTelAttribute("rpc.message.id", 1), + // THIS WAS THE MISSING ATTRIBUTE: + OTelAttribute("rpc.message.type", + "RECEIVED"))))))); +} + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal diff --git a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc index 39f70fd93011d..27c23a3237603 100644 --- a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc @@ -41,30 +41,15 @@ class ObjectDescriptorReaderTracing : public ObjectDescriptorReader { ~ObjectDescriptorReaderTracing() override = default; future Read() override { - auto span = internal::MakeSpan("storage::AsyncConnection::ReadObjectRange"); - internal::OTelScope scope(span); return ObjectDescriptorReader::Read().then( - [span = std::move(span), - oc = opentelemetry::context::RuntimeContext::GetCurrent()]( + []( auto f) -> ReadResponse { auto result = f.get(); - internal::DetachOTelContext(oc); + // internal::DetachOTelContext(oc); if (!absl::holds_alternative(result)) { auto const& payload = absl::get(result); - span->AddEvent( - "gl-cpp.read-range", - {{/*sc::kRpcMessageType=*/"rpc.message.type", "RECEIVED"}, - {sc::kThreadId, internal::CurrentThreadId()}, - {"message.size", static_cast(payload.size())}}); - } else { - span->AddEvent( - "gl-cpp.read-range", - {{/*sc::kRpcMessageType=*/"rpc.message.type", "RECEIVED"}, - {sc::kThreadId, internal::CurrentThreadId()}}); - return internal::EndSpan(*span, - absl::get(std::move(result))); } return result; }); From cbeec75816e4405aac3b7dc55c477067ced12088 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 25 Jun 2025 12:18:02 +0000 Subject: [PATCH 6/8] Remove unnecessary changes --- .../async/object_descriptor_reader_tracing.cc | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc index 27c23a3237603..39f70fd93011d 100644 --- a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc @@ -41,15 +41,30 @@ class ObjectDescriptorReaderTracing : public ObjectDescriptorReader { ~ObjectDescriptorReaderTracing() override = default; future Read() override { + auto span = internal::MakeSpan("storage::AsyncConnection::ReadObjectRange"); + internal::OTelScope scope(span); return ObjectDescriptorReader::Read().then( - []( + [span = std::move(span), + oc = opentelemetry::context::RuntimeContext::GetCurrent()]( auto f) -> ReadResponse { auto result = f.get(); - // internal::DetachOTelContext(oc); + internal::DetachOTelContext(oc); if (!absl::holds_alternative(result)) { auto const& payload = absl::get(result); + span->AddEvent( + "gl-cpp.read-range", + {{/*sc::kRpcMessageType=*/"rpc.message.type", "RECEIVED"}, + {sc::kThreadId, internal::CurrentThreadId()}, + {"message.size", static_cast(payload.size())}}); + } else { + span->AddEvent( + "gl-cpp.read-range", + {{/*sc::kRpcMessageType=*/"rpc.message.type", "RECEIVED"}, + {sc::kThreadId, internal::CurrentThreadId()}}); + return internal::EndSpan(*span, + absl::get(std::move(result))); } return result; }); From 5c6eec22284b48ee726d75663361dc93048bce50 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 26 Jun 2025 10:08:37 +0000 Subject: [PATCH 7/8] formatted the files --- .../object_descriptor_connection_tracing.cc | 6 +-- ...ject_descriptor_connection_tracing_test.cc | 47 +++++++++---------- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc index 8e139f366e61b..78595dfda6bca 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc @@ -13,8 +13,8 @@ // limitations under the License. #include "google/cloud/storage/internal/async/object_descriptor_connection_tracing.h" -#include "google/cloud/storage/internal/async/reader_connection_tracing.h" #include "google/cloud/storage/async/reader_connection.h" +#include "google/cloud/storage/internal/async/reader_connection_tracing.h" #include "google/cloud/internal/opentelemetry.h" #include "google/cloud/version.h" #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY @@ -59,9 +59,7 @@ class AsyncObjectDescriptorConnectionTracing {{sc::kThreadId, internal::CurrentThreadId()}, {"read-start", p.start}, {"read-length", p.length}}); - return MakeTracingReaderConnection( - span_, - std::move(result)); + return MakeTracingReaderConnection(span_, std::move(result)); } private: diff --git a/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc b/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc index 3a32ad7eb8c0d..1792558a89630 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc @@ -34,20 +34,20 @@ namespace { using ReadResponse = ::google::cloud::storage_experimental::AsyncReaderConnection::ReadResponse; using ::google::cloud::storage_experimental::ObjectDescriptorConnection; +using ::google::cloud::storage_experimental::ReadPayload; using ::google::cloud::storage_mocks::MockAsyncObjectDescriptorConnection; using ::google::cloud::storage_mocks::MockAsyncReaderConnection; -using ::google::cloud::storage_experimental::ReadPayload; using ::google::cloud::testing_util::EventNamed; using ::google::cloud::testing_util::InstallSpanCatcher; using ::google::cloud::testing_util::OTelAttribute; using ::google::cloud::testing_util::OTelContextCaptured; -using ::google::cloud::testing_util::ThereIsAnActiveSpan; using ::google::cloud::testing_util::PromiseWithOTelContext; using ::google::cloud::testing_util::SpanEventAttributesAre; using ::google::cloud::testing_util::SpanHasInstrumentationScope; using ::google::cloud::testing_util::SpanKindIsClient; using ::google::cloud::testing_util::SpanNamed; using ::google::cloud::testing_util::SpanWithStatus; +using ::google::cloud::testing_util::ThereIsAnActiveSpan; using ::testing::_; // A helper to set expectations on a mock async reader. It captures the OTel @@ -103,7 +103,8 @@ TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { namespace sc = ::opentelemetry::trace::SemanticConventions; auto span_catcher = InstallSpanCatcher(); - auto mock_connection = std::make_shared(); + auto mock_connection = + std::make_shared(); auto* mock_reader_ptr = new MockAsyncReaderConnection; PromiseWithOTelContext p; EXPECT_CALL(*mock_reader_ptr, Read).WillOnce(expect_context(p)); @@ -126,27 +127,25 @@ TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { auto spans = span_catcher->GetSpans(); EXPECT_THAT( - spans, - ElementsAre(AllOf( - SpanNamed("test-span"), - SpanWithStatus(opentelemetry::trace::StatusCode::kOk), - SpanHasInstrumentationScope(), SpanKindIsClient(), - SpanHasEvents( - AllOf(EventNamed("gl-cpp.open.read"), - SpanEventAttributesAre( - OTelAttribute("read-length", 0), - OTelAttribute("read-start", 0), - OTelAttribute(sc::kThreadId, _))), - AllOf( - EventNamed("gl-cpp.read"), - SpanEventAttributesAre( - OTelAttribute("message.starting_offset", - 123), - OTelAttribute(sc::kThreadId, _), - OTelAttribute("rpc.message.id", 1), - // THIS WAS THE MISSING ATTRIBUTE: - OTelAttribute("rpc.message.type", - "RECEIVED"))))))); + spans, ElementsAre(AllOf( + SpanNamed("test-span"), + SpanWithStatus(opentelemetry::trace::StatusCode::kOk), + SpanHasInstrumentationScope(), SpanKindIsClient(), + SpanHasEvents( + AllOf(EventNamed("gl-cpp.open.read"), + SpanEventAttributesAre( + OTelAttribute("read-length", 0), + OTelAttribute("read-start", 0), + OTelAttribute(sc::kThreadId, _))), + AllOf(EventNamed("gl-cpp.read"), + SpanEventAttributesAre( + OTelAttribute( + "message.starting_offset", 123), + OTelAttribute(sc::kThreadId, _), + OTelAttribute("rpc.message.id", 1), + // THIS WAS THE MISSING ATTRIBUTE: + OTelAttribute("rpc.message.type", + "RECEIVED"))))))); } } // namespace From 09cfa27bacedca143b7488ff197fc83cf452bb05 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Fri, 27 Jun 2025 04:30:51 +0000 Subject: [PATCH 8/8] Use ordered matcher instead of unordered matcher --- .../internal/async/object_descriptor_connection_tracing_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc b/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc index 1792558a89630..46c1e049c2208 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing_test.cc @@ -131,7 +131,7 @@ TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { SpanNamed("test-span"), SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasInstrumentationScope(), SpanKindIsClient(), - SpanHasEvents( + SpanEventsAre( AllOf(EventNamed("gl-cpp.open.read"), SpanEventAttributesAre( OTelAttribute("read-length", 0),