From 103cdbae0502832efe5d1ccaeba06fed643404d6 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 05:25:30 +0000 Subject: [PATCH 1/5] fix(storage): do not close object descriptor span on reader completion --- .../object_descriptor_connection_tracing.cc | 4 +- ...ject_descriptor_connection_tracing_test.cc | 81 +++++++++++++++---- 2 files changed, 68 insertions(+), 17 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 4c0d582628575..7322bdb291ae9 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc @@ -50,12 +50,12 @@ class AsyncObjectDescriptorConnectionTracing std::unique_ptr Read(ReadParams p) override { internal::OTelScope scope(span_); - auto result = impl_->Read(p); span_->AddEvent("gl-cpp.open.read", {{sc::thread::kThreadId, internal::CurrentThreadId()}, {"read-start", p.start}, {"read-length", p.length}}); - return MakeTracingReaderConnection(span_, std::move(result)); + return impl_->Read(p); + ; } void MakeSubsequentStream() override { 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 5719e76c954a7..7d014a424d3d4 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 @@ -98,7 +98,8 @@ TEST(ObjectDescriptorConnectionTracing, Read) { OTelAttribute(sc::thread::kThreadId, _))))))); } -TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { +TEST(ObjectDescriptorConnectionTracing, + SingleReadRangeCompletedDoesNotEndOpenSpan) { namespace sc = ::opentelemetry::semconv; auto span_catcher = InstallSpanCatcher(); @@ -106,21 +107,74 @@ TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { std::make_shared(); auto* mock_reader_ptr = new MockAsyncReaderConnection; PromiseWithOTelContext p; - EXPECT_CALL(*mock_reader_ptr, Read).WillOnce(expect_context(p)); + EXPECT_CALL(*mock_reader_ptr, Read).WillOnce([&p] { return p.get_future(); }); EXPECT_CALL(*mock_connection, Read) - .WillOnce([&](ObjectDescriptorConnection::ReadParams) { + .WillOnce([&](ObjectDescriptorConnection::ReadParams p) { + EXPECT_EQ(p.start, 100); + EXPECT_EQ(p.length, 200); 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)); + auto reader = connection->Read({100, 200}); + auto f = reader->Read(); + // Simulate stream completion (EOF) + p.set_value(Status{}); (void)f.get(); + // Before resetting the connection, the Open span must NOT be ended yet. + EXPECT_THAT(span_catcher->GetSpans(), ::testing::IsEmpty()); + + connection.reset(); // End the span now + + auto spans = span_catcher->GetSpans(); + EXPECT_THAT( + spans, + ElementsAre(AllOf( + SpanNamed("test-span"), + SpanWithStatus(opentelemetry::trace::StatusCode::kOk), + SpanHasInstrumentationScope(), SpanKindIsClient(), + SpanEventsAre(AllOf( + EventNamed("gl-cpp.open.read"), + SpanEventAttributesAre( + OTelAttribute("read-length", 200), + OTelAttribute("read-start", 100), + OTelAttribute(sc::thread::kThreadId, _))))))); +} + +TEST(ObjectDescriptorConnectionTracing, MultipleReadRanges) { + namespace sc = ::opentelemetry::semconv; + auto span_catcher = InstallSpanCatcher(); + + auto mock_connection = + std::make_shared(); + auto mock_reader1 = std::make_unique(); + auto mock_reader2 = std::make_unique(); + + EXPECT_CALL(*mock_connection, Read) + .WillOnce([&](ObjectDescriptorConnection::ReadParams p) { + EXPECT_EQ(p.start, 0); + EXPECT_EQ(p.length, 100); + return std::move(mock_reader1); + }) + .WillOnce([&](ObjectDescriptorConnection::ReadParams p) { + EXPECT_EQ(p.start, 100); + EXPECT_EQ(p.length, 200); + return std::move(mock_reader2); + }); + + auto connection = MakeTracingObjectDescriptorConnection( + internal::MakeSpan("test-span"), std::move(mock_connection)); + + auto reader1 = connection->Read({0, 100}); + auto reader2 = connection->Read({100, 200}); + + // Span is still active and not ended yet + EXPECT_THAT(span_catcher->GetSpans(), ::testing::IsEmpty()); + connection.reset(); // End the span auto spans = span_catcher->GetSpans(); @@ -133,18 +187,15 @@ TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { SpanEventsAre( AllOf(EventNamed("gl-cpp.open.read"), SpanEventAttributesAre( - OTelAttribute("read-length", 0), + OTelAttribute("read-length", 100), OTelAttribute("read-start", 0), OTelAttribute(sc::thread::kThreadId, _))), - AllOf(EventNamed("gl-cpp.read"), + AllOf(EventNamed("gl-cpp.open.read"), SpanEventAttributesAre( - OTelAttribute("message.starting_offset", - 123), - OTelAttribute(sc::thread::kThreadId, _), - OTelAttribute("rpc.message.id", 1), - // THIS WAS THE MISSING ATTRIBUTE: - OTelAttribute("rpc.message.type", - "RECEIVED"))))))); + OTelAttribute("read-length", 200), + OTelAttribute("read-start", 100), + OTelAttribute(sc::thread::kThreadId, + _))))))); } } // namespace From 04a0f6ec57280d48fdb43ec5401452f61dd7f398 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 06:18:19 +0000 Subject: [PATCH 2/5] remove unwanted code --- .../internal/async/object_descriptor_connection_tracing.cc | 1 - 1 file changed, 1 deletion(-) 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 7322bdb291ae9..d839220fd3e13 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc @@ -55,7 +55,6 @@ class AsyncObjectDescriptorConnectionTracing {"read-start", p.start}, {"read-length", p.length}}); return impl_->Read(p); - ; } void MakeSubsequentStream() override { From 2e480f219f7c48cbcf90a0f390ba644f7cbef7f1 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 06:23:39 +0000 Subject: [PATCH 3/5] address the review comment --- ...ject_descriptor_connection_tracing_test.cc | 30 ++++--------------- 1 file changed, 5 insertions(+), 25 deletions(-) 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 7d014a424d3d4..108c212556c4b 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 @@ -38,35 +38,14 @@ using ::google::cloud::storage_mocks::MockAsyncReaderConnection; 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::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 -// 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::semconv; auto span_catcher = InstallSpanCatcher(); @@ -105,15 +84,16 @@ TEST(ObjectDescriptorConnectionTracing, auto mock_connection = std::make_shared(); - auto* mock_reader_ptr = new MockAsyncReaderConnection; + auto mock_reader = std::make_unique(); PromiseWithOTelContext p; - EXPECT_CALL(*mock_reader_ptr, Read).WillOnce([&p] { return p.get_future(); }); + EXPECT_CALL(*mock_reader, Read).WillOnce([&p] { return p.get_future(); }); EXPECT_CALL(*mock_connection, Read) - .WillOnce([&](ObjectDescriptorConnection::ReadParams p) { + .WillOnce([&, r = std::move(mock_reader)]( + ObjectDescriptorConnection::ReadParams p) mutable { EXPECT_EQ(p.start, 100); EXPECT_EQ(p.length, 200); - return std::unique_ptr(mock_reader_ptr); + return std::move(r); }); auto connection = MakeTracingObjectDescriptorConnection( From 22cdab1820a8e8f3bb4506842f7a32c988bedf09 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 10:34:46 +0000 Subject: [PATCH 4/5] fix ci failures --- .../internal/async/object_descriptor_connection_tracing_test.cc | 1 - 1 file changed, 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 108c212556c4b..c1a742ee081e8 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 @@ -32,7 +32,6 @@ namespace { using ReadResponse = ::google::cloud::storage::AsyncReaderConnection::ReadResponse; using ::google::cloud::storage::ObjectDescriptorConnection; -using ::google::cloud::storage::ReadPayload; using ::google::cloud::storage_mocks::MockAsyncObjectDescriptorConnection; using ::google::cloud::storage_mocks::MockAsyncReaderConnection; using ::google::cloud::testing_util::EventNamed; From 53c75b106acf1cedef11f29c2ff4e9fe2b707723 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 6 Aug 2026 04:14:53 +0000 Subject: [PATCH 5/5] make sure ReadRange spans are under Open span --- .../object_descriptor_connection_tracing.cc | 1 - .../async/object_descriptor_reader_tracing.cc | 24 +++++++++++++++---- .../async/object_descriptor_reader_tracing.h | 5 +++- 3 files changed, 23 insertions(+), 7 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 d839220fd3e13..bd25480c01a1a 100644 --- a/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_connection_tracing.cc @@ -14,7 +14,6 @@ #include "google/cloud/storage/internal/async/object_descriptor_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" #include 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 8b95c508c3360..c606db4f7017f 100644 --- a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc +++ b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.cc @@ -31,13 +31,21 @@ namespace sc = ::opentelemetry::semconv; class ObjectDescriptorReaderTracing : public ObjectDescriptorReader { public: - explicit ObjectDescriptorReaderTracing(std::shared_ptr impl) - : ObjectDescriptorReader(std::move(impl)) {} + explicit ObjectDescriptorReaderTracing( + std::shared_ptr impl, + opentelemetry::nostd::shared_ptr parent_span) + : ObjectDescriptorReader(std::move(impl)), + parent_span_(std::move(parent_span)) {} ~ObjectDescriptorReaderTracing() override = default; future Read() override { - auto span = internal::MakeSpan("storage::AsyncConnection::ReadRange"); + opentelemetry::trace::StartSpanOptions options; + if (parent_span_ && parent_span_->GetContext().IsValid()) { + options.parent = parent_span_->GetContext(); + } + auto span = + internal::MakeSpan("storage::AsyncConnection::ReadRange", options); internal::OTelScope scope(span); return ObjectDescriptorReader::Read().then( [span = std::move(span), @@ -64,13 +72,19 @@ class ObjectDescriptorReaderTracing : public ObjectDescriptorReader { return result; }); } + + private: + opentelemetry::nostd::shared_ptr parent_span_; }; } // namespace std::unique_ptr -MakeTracingObjectDescriptorReader(std::shared_ptr impl) { - return std::make_unique(std::move(impl)); +MakeTracingObjectDescriptorReader( + std::shared_ptr impl, + opentelemetry::nostd::shared_ptr parent_span) { + return std::make_unique( + std::move(impl), std::move(parent_span)); } GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.h b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.h index 1a010cdbb366b..5df80f5038a2d 100644 --- a/google/cloud/storage/internal/async/object_descriptor_reader_tracing.h +++ b/google/cloud/storage/internal/async/object_descriptor_reader_tracing.h @@ -25,7 +25,10 @@ namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN std::unique_ptr -MakeTracingObjectDescriptorReader(std::shared_ptr impl); +MakeTracingObjectDescriptorReader( + std::shared_ptr impl, + opentelemetry::nostd::shared_ptr parent_span = + opentelemetry::trace::Tracer::GetCurrentSpan()); GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal