From 4ed446db151f5c32dc51273ebcbe067a5e6b9170 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 05:25:30 +0000 Subject: [PATCH 1/4] 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 18a5fdd2b59c850056051d0853f5906d672f3fa2 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 06:18:19 +0000 Subject: [PATCH 2/4] 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 7ad2f9c21a180e6b2d3ee441eeea51d5dfe39abb Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 06:23:39 +0000 Subject: [PATCH 3/4] 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 db2efffecbed1e428d3fa34f0e2123bb688b1ff3 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 5 Aug 2026 10:34:46 +0000 Subject: [PATCH 4/4] 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;