From 95d47a5ac2300735b27f096dd78897b699dc4b44 Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 15 Sep 2022 13:54:41 -0700 Subject: [PATCH 1/5] fix --- .../opentelemetry/exporters/etw/etw_tracer.h | 70 ++++++++++--------- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h index 0c828d7241..ea2863a832 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h @@ -64,9 +64,10 @@ class Span; template SpanType *new_span(TracerType *objPtr, nostd::string_view name, - const opentelemetry::trace::StartSpanOptions &options) + const opentelemetry::trace::StartSpanOptions &options, + const opentelemetry::trace::SpanContext &span_context) { - return new (std::nothrow) SpanType{*objPtr, name, options}; + return new (std::nothrow) SpanType{*objPtr, name, options, span_context}; } /** @@ -374,30 +375,6 @@ class Tracer : public opentelemetry::trace::Tracer, const opentelemetry::trace::SpanContextKeyValueIterable &links, const opentelemetry::trace::StartSpanOptions &options = {}) noexcept override { - const auto &cfg = GetConfiguration(tracerProvider_); - - // Parent Context: - // - either use current span - // - or attach to parent SpanContext specified in options - opentelemetry::trace::SpanContext parentContext = GetCurrentSpan()->GetContext(); - if (nostd::holds_alternative(options.parent)) - { - auto span_context = nostd::get(options.parent); - if (span_context.IsValid()) - { - parentContext = span_context; - } - } - auto sampling_result = - GetSampler(tracerProvider_) - .ShouldSample(parentContext, traceId_, name, options.kind, attributes, links); - if (sampling_result.decision == sdk::trace::Decision::DROP) - { - static nostd::shared_ptr noop_span( - new trace::NoopSpan{this->shared_from_this()}); - return noop_span; - } - #ifdef OPENTELEMETRY_RTTI_ENABLED common::KeyValueIterable &attribs = const_cast(attributes); Properties *evt = dynamic_cast(&attribs); @@ -433,12 +410,37 @@ class Tracer : public opentelemetry::trace::Tracer, opentelemetry::trace::SpanContext parentContext = GetCurrentSpan()->GetContext(); if (nostd::holds_alternative(options.parent)) { - auto span_context = nostd::get(options.parent); - if (span_context.IsValid()) + auto spanContext = nostd::get(options.parent); + if (spanContext.IsValid()) { - parentContext = span_context; + parentContext = spanContext; } } + auto sampling_result = + GetSampler(tracerProvider_) + .ShouldSample(parentContext, traceId_, name, options.kind, attributes, links); + + auto trace_flags = + sampling_result.decision == Decision::DROP + ? opentelemetry::trace::TraceFlags{} + : opentelemetry::trace::TraceFlags{opentelemetry::trace::TraceFlags::kIsSampled}; + + auto traceId = parentContext.IsValid() ? parentContext.trace_id() : traceId_; + + auto spanContext = + std::unique_ptr(new opentelemetry::trace::SpanContext( + traceId, GetIdGenerator(tracerProvider_).GenerateSpanId(), trace_flags, false, + sampling_result.trace_state + ? sampling_result.trace_state + : parentContext.IsValid ? parent_context.trace_state() + : opentelemetry::trace::TraceState::GetDefault())); + + if (sampling_result.decision == sdk::trace::Decision::DROP) + { + static nostd::shared_ptr noop_span( + new trace::NoopSpan{this->shared_from_this()}); + return noop_span; + } // Populate Etw.RelatedActivityId at envelope level if enabled GUID RelatedActivityId; @@ -456,10 +458,10 @@ class Tracer : public opentelemetry::trace::Tracer, // This template pattern allows us to forward-declare the etw::Span, // create an instance of it, then assign it to tracer::Span result. - auto currentSpan = new_span(this, name, options); + auto currentSpan = new_span(this, name, options, spanContext); nostd::shared_ptr result = to_span_ptr(currentSpan); - auto spanContext = result->GetContext(); + // auto spanContext = result->GetContext(); // Decorate with additional standard fields std::string eventName = name.data(); @@ -759,13 +761,13 @@ class Span : public opentelemetry::trace::Span Span(Tracer &owner, nostd::string_view name, const opentelemetry::trace::StartSpanOptions &options, + const opentelemetry::trace::SpanContext &span_context, Span *parent = nullptr) noexcept : opentelemetry::trace::Span(), start_time_(std::chrono::system_clock::now()), owner_(owner), - parent_(parent), - context_{owner.traceId_, GetIdGenerator(owner.tracerProvider_).GenerateSpanId(), - opentelemetry::trace::TraceFlags{0}, false} + context_(span_context), + parent_(parent) { name_ = name; UNREFERENCED_PARAMETER(options); From eab88dd48d2b1637a78df274cb4e317aa14a4cab Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 15 Sep 2022 14:38:07 -0700 Subject: [PATCH 2/5] fix --- .../opentelemetry/exporters/etw/etw_tracer.h | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h index ea2863a832..d516ad0f22 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h @@ -416,23 +416,24 @@ class Tracer : public opentelemetry::trace::Tracer, parentContext = spanContext; } } - auto sampling_result = + auto traceId = parentContext.IsValid() ? parentContext.trace_id() : traceId_; + + // Sampling based on attributes is not supported for now, so passing empty below. + opentelemetry::sdk::trace::SamplingResult sampling_result = GetSampler(tracerProvider_) - .ShouldSample(parentContext, traceId_, name, options.kind, attributes, links); + .ShouldSample(parentContext, traceId, name, options.kind, {}, links); - auto trace_flags = - sampling_result.decision == Decision::DROP + opentelemetry::trace::TraceFlags trace_flags = + sampling_result.decision == opentelemetry::sdk::trace::Decision::DROP ? opentelemetry::trace::TraceFlags{} : opentelemetry::trace::TraceFlags{opentelemetry::trace::TraceFlags::kIsSampled}; - auto traceId = parentContext.IsValid() ? parentContext.trace_id() : traceId_; - auto spanContext = std::unique_ptr(new opentelemetry::trace::SpanContext( traceId, GetIdGenerator(tracerProvider_).GenerateSpanId(), trace_flags, false, sampling_result.trace_state ? sampling_result.trace_state - : parentContext.IsValid ? parent_context.trace_state() + : parentContext.IsValid ? parentContext.trace_state() : opentelemetry::trace::TraceState::GetDefault())); if (sampling_result.decision == sdk::trace::Decision::DROP) From 8ed4dfe29bb097141a2d33336298b4b1f28e4029 Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 15 Sep 2022 15:53:20 -0700 Subject: [PATCH 3/5] fix --- .../opentelemetry/exporters/etw/etw_tracer.h | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h index d516ad0f22..b2780a005a 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h @@ -65,9 +65,9 @@ template SpanType *new_span(TracerType *objPtr, nostd::string_view name, const opentelemetry::trace::StartSpanOptions &options, - const opentelemetry::trace::SpanContext &span_context) + std::unique_ptr span_context) { - return new (std::nothrow) SpanType{*objPtr, name, options, span_context}; + return new (std::nothrow) SpanType{*objPtr, name, options, std::move(span_context)}; } /** @@ -419,9 +419,13 @@ class Tracer : public opentelemetry::trace::Tracer, auto traceId = parentContext.IsValid() ? parentContext.trace_id() : traceId_; // Sampling based on attributes is not supported for now, so passing empty below. + std::map empty_attributes = {{}}; opentelemetry::sdk::trace::SamplingResult sampling_result = GetSampler(tracerProvider_) - .ShouldSample(parentContext, traceId, name, options.kind, {}, links); + .ShouldSample(parentContext, traceId, name, options.kind, + opentelemetry::common::KeyValueIterableView>( + empty_attributes), + links); opentelemetry::trace::TraceFlags trace_flags = sampling_result.decision == opentelemetry::sdk::trace::Decision::DROP @@ -433,8 +437,8 @@ class Tracer : public opentelemetry::trace::Tracer, traceId, GetIdGenerator(tracerProvider_).GenerateSpanId(), trace_flags, false, sampling_result.trace_state ? sampling_result.trace_state - : parentContext.IsValid ? parentContext.trace_state() - : opentelemetry::trace::TraceState::GetDefault())); + : parentContext.IsValid() ? parentContext.trace_state() + : opentelemetry::trace::TraceState::GetDefault())); if (sampling_result.decision == sdk::trace::Decision::DROP) { @@ -459,11 +463,9 @@ class Tracer : public opentelemetry::trace::Tracer, // This template pattern allows us to forward-declare the etw::Span, // create an instance of it, then assign it to tracer::Span result. - auto currentSpan = new_span(this, name, options, spanContext); + auto currentSpan = new_span(this, name, options, std::move(spanContext)); nostd::shared_ptr result = to_span_ptr(currentSpan); - // auto spanContext = result->GetContext(); - // Decorate with additional standard fields std::string eventName = name.data(); @@ -478,13 +480,13 @@ class Tracer : public opentelemetry::trace::Tracer, { evt[ETW_FIELD_SPAN_PARENTID] = ToLowerBase16(parentContext.span_id()); } - evt[ETW_FIELD_SPAN_ID] = ToLowerBase16(spanContext.span_id()); + evt[ETW_FIELD_SPAN_ID] = ToLowerBase16(result.get()->GetContext().span_id()); } // Populate Etw.Payload["TraceId"] attribute if (cfg.enableTraceId) { - evt[ETW_FIELD_TRACE_ID] = ToLowerBase16(spanContext.trace_id()); + evt[ETW_FIELD_TRACE_ID] = ToLowerBase16(result.get()->GetContext().trace_id()); } // Populate Etw.ActivityId at envelope level if enabled @@ -708,7 +710,7 @@ class Span : public opentelemetry::trace::Span */ Span *GetParent() const { return parent_; } - opentelemetry::trace::SpanContext context_; + std::unique_ptr context_; public: /** @@ -762,12 +764,12 @@ class Span : public opentelemetry::trace::Span Span(Tracer &owner, nostd::string_view name, const opentelemetry::trace::StartSpanOptions &options, - const opentelemetry::trace::SpanContext &span_context, + std::unique_ptr span_context, Span *parent = nullptr) noexcept : opentelemetry::trace::Span(), start_time_(std::chrono::system_clock::now()), owner_(owner), - context_(span_context), + context_(std::move(span_context)), parent_(parent) { name_ = name; @@ -886,7 +888,7 @@ class Span : public opentelemetry::trace::Span * @brief Obtain SpanContext * @return */ - opentelemetry::trace::SpanContext GetContext() const noexcept override { return context_; } + opentelemetry::trace::SpanContext GetContext() const noexcept override { return *context_.get(); } /** * @brief Check if Span is recording data. From f4e00336eef6a9792e9475cd37a33c15a0b2e76b Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 16 Sep 2022 00:00:05 -0700 Subject: [PATCH 4/5] Fix --- .../opentelemetry/exporters/etw/etw_tracer.h | 4 +- exporters/etw/test/etw_tracer_test.cc | 70 ++++++++++++++++++- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h index b2780a005a..d4898d0345 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h @@ -442,8 +442,8 @@ class Tracer : public opentelemetry::trace::Tracer, if (sampling_result.decision == sdk::trace::Decision::DROP) { - static nostd::shared_ptr noop_span( - new trace::NoopSpan{this->shared_from_this()}); + auto noop_span = nostd::shared_ptr{ + new (std::nothrow) trace::NoopSpan(this->shared_from_this(), std::move(spanContext))}; return noop_span; } diff --git a/exporters/etw/test/etw_tracer_test.cc b/exporters/etw/test/etw_tracer_test.cc index 43b7d18698..d80323be22 100644 --- a/exporters/etw/test/etw_tracer_test.cc +++ b/exporters/etw/test/etw_tracer_test.cc @@ -7,6 +7,7 @@ # include # include +# include "opentelemetry//sdk/trace/sampler.h" # include "opentelemetry/exporters/etw/etw_tracer_exporter.h" # include "opentelemetry/sdk/trace/samplers/always_off.h" # include "opentelemetry/sdk/trace/simple_processor.h" @@ -14,6 +15,7 @@ using namespace OPENTELEMETRY_NAMESPACE; using namespace opentelemetry::exporter::etw; +using namespace opentelemetry::sdk::trace; const char *kGlobalProviderName = "OpenTelemetry-ETW-TLD"; @@ -40,6 +42,42 @@ class MockIdGenerator : public sdk::trace::IdGenerator uint8_t buf_trace[16] = {1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}; }; +/* A Custom Sampler, implementing parent based sampler*/ +class MockSampler : public sdk::trace::Sampler +{ +public: + MockSampler(std::shared_ptr delegate_sampler) noexcept + : delegate_sampler_(delegate_sampler) + {} + sdk::trace::SamplingResult ShouldSample( + const trace_api::SpanContext &parent_context, + trace_api::TraceId trace_id, + nostd::string_view name, + trace_api::SpanKind span_kind, + const opentelemetry::common::KeyValueIterable &attributes, + const trace_api::SpanContextKeyValueIterable &links) noexcept + { + if (!parent_context.IsValid()) + { + // If no parent (root span) exists returns the result of the delegateSampler + return delegate_sampler_->ShouldSample(parent_context, trace_id, name, span_kind, attributes, + links); + } + + // If parent exists: + if (parent_context.IsSampled()) + { + return {Decision::RECORD_AND_SAMPLE, nullptr, parent_context.trace_state()}; + } + return {Decision::DROP, nullptr, parent_context.trace_state()}; + } + + nostd::string_view GetDescription() const noexcept { return "Custom Sampler"; } + +private: + std::shared_ptr delegate_sampler_; +}; + /* clang-format off */ TEST(ETWTracer, TracerCheck) { @@ -417,7 +455,8 @@ TEST(ETWTracer, AlwayOffSampler) std::move(always_off)); auto tracer = tp.GetTracer(providerName); auto span = tracer->StartSpan("span_off"); - EXPECT_EQ(span->GetContext().IsValid(), false); + EXPECT_EQ(span->GetContext().IsValid(), true); + EXPECT_EQ(span->GetContext().IsSampled(), false); } TEST(ETWTracer, CustomIdGenerator) @@ -440,6 +479,35 @@ TEST(ETWTracer, CustomIdGenerator) EXPECT_EQ(span->GetContext().trace_id(), id_generator->GenerateTraceId()); } +TEST(ETWTracer, CustomSampler) +{ + std::string providerName = kGlobalProviderName; // supply unique instrumentation name here + auto parent_off = std::unique_ptr(new MockSampler(std::make_shared())); + exporter::etw::TracerProvider tp + ({ + {"enableTraceId", true}, + {"enableSpanId", true}, + {"enableActivityId", true}, + {"enableRelatedActivityId", true}, + {"enableAutoParent", true} + }, + std::move(parent_off)); + auto tracer = tp.GetTracer(providerName); + { + auto span = tracer->StartSpan("span_off"); + EXPECT_EQ(span->GetContext().IsValid(), true); + EXPECT_EQ(span->GetContext().IsSampled(), true); + auto scope = tracer->WithActiveSpan(span); + auto trace_id = span->GetContext().trace_id(); + { + auto child_span = tracer->StartSpan("span on"); + EXPECT_EQ(child_span->GetContext().IsValid(), true); + EXPECT_EQ(child_span->GetContext().IsSampled(), true); + EXPECT_EQ(child_span->GetContext().trace_id(), trace_id); + } + } +} + /* clang-format on */ #endif From 5fb17a09afd017409966ddc32ebb63207964d32a Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 19 Sep 2022 10:28:19 -0700 Subject: [PATCH 5/5] fix review comments --- .../opentelemetry/exporters/etw/etw_tracer.h | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h index d4898d0345..41ab741c1c 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h @@ -65,9 +65,9 @@ template SpanType *new_span(TracerType *objPtr, nostd::string_view name, const opentelemetry::trace::StartSpanOptions &options, - std::unique_ptr span_context) + std::unique_ptr spanContext) { - return new (std::nothrow) SpanType{*objPtr, name, options, std::move(span_context)}; + return new (std::nothrow) SpanType{*objPtr, name, options, std::move(spanContext)}; } /** @@ -419,22 +419,22 @@ class Tracer : public opentelemetry::trace::Tracer, auto traceId = parentContext.IsValid() ? parentContext.trace_id() : traceId_; // Sampling based on attributes is not supported for now, so passing empty below. - std::map empty_attributes = {{}}; + std::map emptyAttributes = {{}}; opentelemetry::sdk::trace::SamplingResult sampling_result = GetSampler(tracerProvider_) .ShouldSample(parentContext, traceId, name, options.kind, opentelemetry::common::KeyValueIterableView>( - empty_attributes), + emptyAttributes), links); - opentelemetry::trace::TraceFlags trace_flags = + opentelemetry::trace::TraceFlags traceFlags = sampling_result.decision == opentelemetry::sdk::trace::Decision::DROP ? opentelemetry::trace::TraceFlags{} : opentelemetry::trace::TraceFlags{opentelemetry::trace::TraceFlags::kIsSampled}; auto spanContext = std::unique_ptr(new opentelemetry::trace::SpanContext( - traceId, GetIdGenerator(tracerProvider_).GenerateSpanId(), trace_flags, false, + traceId, GetIdGenerator(tracerProvider_).GenerateSpanId(), traceFlags, false, sampling_result.trace_state ? sampling_result.trace_state : parentContext.IsValid() ? parentContext.trace_state() @@ -442,9 +442,9 @@ class Tracer : public opentelemetry::trace::Tracer, if (sampling_result.decision == sdk::trace::Decision::DROP) { - auto noop_span = nostd::shared_ptr{ + auto noopSpan = nostd::shared_ptr{ new (std::nothrow) trace::NoopSpan(this->shared_from_this(), std::move(spanContext))}; - return noop_span; + return noopSpan; } // Populate Etw.RelatedActivityId at envelope level if enabled @@ -764,12 +764,12 @@ class Span : public opentelemetry::trace::Span Span(Tracer &owner, nostd::string_view name, const opentelemetry::trace::StartSpanOptions &options, - std::unique_ptr span_context, + std::unique_ptr spanContext, Span *parent = nullptr) noexcept : opentelemetry::trace::Span(), start_time_(std::chrono::system_clock::now()), owner_(owner), - context_(std::move(span_context)), + context_(std::move(spanContext)), parent_(parent) { name_ = name;