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..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 @@ -50,12 +49,11 @@ 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..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,41 +32,19 @@ 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; 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(); @@ -98,29 +76,84 @@ TEST(ObjectDescriptorConnectionTracing, Read) { OTelAttribute(sc::thread::kThreadId, _))))))); } -TEST(ObjectDescriptorConnectionTracing, ReadThenRead) { +TEST(ObjectDescriptorConnectionTracing, + SingleReadRangeCompletedDoesNotEndOpenSpan) { namespace sc = ::opentelemetry::semconv; auto span_catcher = InstallSpanCatcher(); 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(expect_context(p)); + EXPECT_CALL(*mock_reader, Read).WillOnce([&p] { return p.get_future(); }); EXPECT_CALL(*mock_connection, Read) - .WillOnce([&](ObjectDescriptorConnection::ReadParams) { - return std::unique_ptr(mock_reader_ptr); + .WillOnce([&, r = std::move(mock_reader)]( + ObjectDescriptorConnection::ReadParams p) mutable { + EXPECT_EQ(p.start, 100); + EXPECT_EQ(p.length, 200); + return std::move(r); }); 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 +166,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 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