From 2b080c1a4c8d95aea617686d7f3444413ae4f552 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 25 May 2021 13:30:43 +0530 Subject: [PATCH] Return Scope object instead of unique_ptr from Tracer::WithActiveSpan() (#788) --- api/include/opentelemetry/trace/tracer.h | 5 +- api/test/trace/tracer_test.cc | 25 +-- .../opentelemetry/exporters/etw/etw_tracer.h | 16 -- exporters/etw/test/etw_tracer_test.cc | 144 +++++++++++------- 4 files changed, 100 insertions(+), 90 deletions(-) diff --git a/api/include/opentelemetry/trace/tracer.h b/api/include/opentelemetry/trace/tracer.h index 7fe53139d1..57761e5f90 100644 --- a/api/include/opentelemetry/trace/tracer.h +++ b/api/include/opentelemetry/trace/tracer.h @@ -143,10 +143,7 @@ class Tracer * @param span the span that should be set as the new active span. * @return a Scope that controls how long the span will be active. */ - static nostd::unique_ptr WithActiveSpan(nostd::shared_ptr &span) noexcept - { - return nostd::unique_ptr(new Scope{span}); - } + static Scope WithActiveSpan(nostd::shared_ptr &span) noexcept { return Scope{span}; } /** * Get the currently active span. diff --git a/api/test/trace/tracer_test.cc b/api/test/trace/tracer_test.cc index 62777232a2..8e16f15e25 100644 --- a/api/test/trace/tracer_test.cc +++ b/api/test/trace/tracer_test.cc @@ -20,19 +20,20 @@ TEST(TracerTest, GetCurrentSpan) auto current = tracer->GetCurrentSpan(); ASSERT_FALSE(current->GetContext().IsValid()); - auto scope_first = tracer->WithActiveSpan(span_first); - current = tracer->GetCurrentSpan(); - ASSERT_EQ(current, span_first); + { + auto scope_first = tracer->WithActiveSpan(span_first); + current = tracer->GetCurrentSpan(); + ASSERT_EQ(current, span_first); + + { + auto scope_second = tracer->WithActiveSpan(span_second); + current = tracer->GetCurrentSpan(); + ASSERT_EQ(current, span_second); + } + current = tracer->GetCurrentSpan(); + ASSERT_EQ(current, span_first); + } - auto scope_second = tracer->WithActiveSpan(span_second); - current = tracer->GetCurrentSpan(); - ASSERT_EQ(current, span_second); - - scope_second.reset(nullptr); - current = tracer->GetCurrentSpan(); - ASSERT_EQ(current, span_first); - - scope_first.reset(nullptr); current = tracer->GetCurrentSpan(); ASSERT_FALSE(current->GetContext().IsValid()); } diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h index 4a6b191735..7cf6edea24 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h @@ -477,22 +477,12 @@ class Tracer : public trace::Tracer UpdateStatus(currentSpan, evt); etwProvider().write(provHandle, evt, ActivityIdPtr, RelatedActivityIdPtr, 0, encoding); } - - { - // Atomically remove the span from list of spans - const std::lock_guard lock(scopes_mutex_); - auto spanId = ToLowerBase16(spanBase.GetContext().span_id()); - scopes_.erase(spanId); - } }; const trace::TraceId &trace_id() { return traceId_; }; friend class Span; - std::mutex scopes_mutex_; // protects scopes_ - std::map> scopes_; - /** * @brief Init a reference to etw::ProviderHandle * @return Provider Handle @@ -648,12 +638,6 @@ class Tracer : public trace::Tracer etwProvider().write(provHandle, evt, ActivityIdPtr, RelatedActivityIdPtr, 1, encoding); }; - { - const std::lock_guard lock(scopes_mutex_); - // Use span_id as index - scopes_[ToLowerBase16(result->GetContext().span_id())] = WithActiveSpan(result); - } - return result; }; diff --git a/exporters/etw/test/etw_tracer_test.cc b/exporters/etw/test/etw_tracer_test.cc index 2f25a85bdb..45acd4f705 100644 --- a/exporters/etw/test/etw_tracer_test.cc +++ b/exporters/etw/test/etw_tracer_test.cc @@ -57,55 +57,62 @@ TEST(ETWTracer, TracerCheck) {"attrib1", 1}, {"attrib2", 2} }; + { + auto topSpan = tracer->StartSpan("MySpanTop"); + auto topScope = tracer->WithActiveSpan(topSpan); + std::this_thread::sleep_for (std::chrono::seconds(1)); + { + auto outerSpan = tracer->StartSpan("MySpanL2", attribs); + auto outerScope = tracer->WithActiveSpan(outerSpan); - auto topSpan = tracer->StartSpan("MySpanTop"); - std::this_thread::sleep_for (std::chrono::seconds(1)); - - auto outerSpan = tracer->StartSpan("MySpanL2", attribs); - - // Create nested span. Note how we share the attributes here. - // It is Okay to either reuse/share or have your own attributes. - auto innerSpan = tracer->StartSpan("MySpanL3", attribs); + // Create nested span. Note how we share the attributes here. + // It is Okay to either reuse/share or have your own attributes. + { + auto innerSpan = tracer->StartSpan("MySpanL3", attribs); + auto innerScope = tracer->WithActiveSpan(innerSpan); - // Add first event - std::string eventName1 = "MyEvent1"; - Properties event1 = - { - {"uint32Key", (uint32_t)1234}, - {"uint64Key", (uint64_t)1234567890}, - {"strKey", "someValue"} - }; - EXPECT_NO_THROW(outerSpan->AddEvent(eventName1, event1)); - std::this_thread::sleep_for (std::chrono::seconds(1)); + // Add first event + std::string eventName1 = "MyEvent1"; + Properties event1 = + { + {"uint32Key", (uint32_t)1234}, + {"uint64Key", (uint64_t)1234567890}, + {"strKey", "someValue"} + }; + EXPECT_NO_THROW(outerSpan->AddEvent(eventName1, event1)); + std::this_thread::sleep_for (std::chrono::seconds(1)); - // Add second event - std::string eventName2 = "MyEvent2"; - Properties event2 = - { - {"uint32Key", (uint32_t)9876}, - {"uint64Key", (uint64_t)987654321}, - {"strKey", "anotherValue"} - }; - EXPECT_NO_THROW(outerSpan->AddEvent(eventName2, event2)); - std::this_thread::sleep_for (std::chrono::seconds(2)); + // Add second event + std::string eventName2 = "MyEvent2"; + Properties event2 = + { + {"uint32Key", (uint32_t)9876}, + {"uint64Key", (uint64_t)987654321}, + {"strKey", "anotherValue"} + }; + EXPECT_NO_THROW(outerSpan->AddEvent(eventName2, event2)); + std::this_thread::sleep_for (std::chrono::seconds(2)); - std::string eventName3= "MyEvent3"; - Properties event3 = - { - /* Extra metadata that allows event to flow to A.I. pipeline */ - {"metadata", "ai_event"}, - {"uint32Key", (uint32_t)9876}, - {"uint64Key", (uint64_t)987654321}, - // {"int32array", {{-1,0,1,2,3}} }, - {"tempString", getTemporaryValue() } - }; - EXPECT_NO_THROW(innerSpan->AddEvent(eventName3, event3)); - std::this_thread::sleep_for (std::chrono::seconds(1)); + std::string eventName3= "MyEvent3"; + Properties event3 = + { + /* Extra metadata that allows event to flow to A.I. pipeline */ + {"metadata", "ai_event"}, + {"uint32Key", (uint32_t)9876}, + {"uint64Key", (uint64_t)987654321}, + // {"int32array", {{-1,0,1,2,3}} }, + {"tempString", getTemporaryValue() } + }; + EXPECT_NO_THROW(innerSpan->AddEvent(eventName3, event3)); + std::this_thread::sleep_for (std::chrono::seconds(1)); + EXPECT_NO_THROW(innerSpan->End()); - EXPECT_NO_THROW(innerSpan->End()); // end innerSpan + } + EXPECT_NO_THROW(outerSpan->End()); - EXPECT_NO_THROW(outerSpan->End()); // end outerSpan - EXPECT_NO_THROW(topSpan->End()); // end topSpan + } + EXPECT_NO_THROW(topSpan->End()); + } EXPECT_NO_THROW(tracer->CloseWithMicroseconds(0)); } @@ -141,12 +148,21 @@ TEST(ETWTracer, TracerCheckMinDecoration) {"enableAutoParent", false} }); auto tracer = tp.GetTracer(providerName, "TLD"); - auto aSpan = tracer->StartSpan("A.min"); - auto bSpan = tracer->StartSpan("B.min"); - auto cSpan = tracer->StartSpan("C.min"); - cSpan->End(); - bSpan->End(); - aSpan->End(); + { + auto aSpan = tracer->StartSpan("A.min"); + auto aScope = tracer->WithActiveSpan(aSpan); + { + auto bSpan = tracer->StartSpan("B.min"); + auto bScope = tracer->WithActiveSpan(bSpan); + { + auto cSpan = tracer->StartSpan("C.min"); + auto cScope = tracer->WithActiveSpan(cSpan); + EXPECT_NO_THROW(cSpan->End()); + } + EXPECT_NO_THROW(bSpan->End()); + } + EXPECT_NO_THROW(aSpan->End()); +} tracer->CloseWithMicroseconds(0); } @@ -183,12 +199,21 @@ TEST(ETWTracer, TracerCheckMaxDecoration) {"enableAutoParent", true} }); auto tracer = tp.GetTracer(providerName, "TLD" ); - auto aSpan = tracer->StartSpan("A.max"); - auto bSpan = tracer->StartSpan("B.max"); - auto cSpan = tracer->StartSpan("C.max"); - cSpan->End(); - bSpan->End(); - aSpan->End(); + { + auto aSpan = tracer->StartSpan("A.max"); + auto aScope = tracer->WithActiveSpan(aSpan); + { + auto bSpan = tracer->StartSpan("B.max"); + auto bScope = tracer->WithActiveSpan(bSpan); + { + auto cSpan = tracer->StartSpan("C.max"); + auto cScope = tracer->WithActiveSpan(cSpan); + EXPECT_NO_THROW(cSpan->End()); + } + EXPECT_NO_THROW(bSpan->End()); + } + EXPECT_NO_THROW(aSpan->End()); + } tracer->CloseWithMicroseconds(0); } @@ -206,10 +231,13 @@ TEST(ETWTracer, TracerCheckMsgPack) auto tracer = tp.GetTracer(providerName, "MsgPack" ); { auto aSpan = tracer->StartSpan("A.max"); + auto aScope = tracer->WithActiveSpan(aSpan); { auto bSpan = tracer->StartSpan("B.max"); + auto bScope = tracer->WithActiveSpan(bSpan); { auto cSpan = tracer->StartSpan("C.max"); + auto cScope = tracer->WithActiveSpan(cSpan); std::string eventName = "MyMsgPackEvent"; Properties event = { @@ -218,11 +246,11 @@ TEST(ETWTracer, TracerCheckMsgPack) {"strKey", "someValue"} }; cSpan->AddEvent(eventName, event); - cSpan->End(); + EXPECT_NO_THROW(cSpan->End()); } - bSpan->End(); + EXPECT_NO_THROW(bSpan->End()); } - aSpan->End(); + EXPECT_NO_THROW(aSpan->End()); } tracer->CloseWithMicroseconds(0); }