From 82e92da3b06a22bdb7336015b7cd2eb13900cf2b Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Aug 2018 23:04:35 -0700 Subject: [PATCH 01/16] Tweak span. --- src/lightstep_span.cpp | 18 +++++++++--------- src/lightstep_span.h | 1 + 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 524d5aa0..485e7b2c 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -170,16 +170,16 @@ void LightStepSpan::FinishWithOptions( finish_timestamp = SteadyClock::now(); } - collector::Span span; + collector::Span span_; // Set timing information. auto duration = finish_timestamp - start_steady_; - span.set_duration_micros( + span_.set_duration_micros( std::chrono::duration_cast(duration).count()); - *span.mutable_start_timestamp() = ToTimestamp(start_timestamp_); + *span_.mutable_start_timestamp() = ToTimestamp(start_timestamp_); // Set references. - auto references = span.mutable_references(); + auto references = span_.mutable_references(); references->Reserve(static_cast(references_.size())); for (const auto& reference : references_) { *references->Add() = reference; @@ -188,8 +188,8 @@ void LightStepSpan::FinishWithOptions( // Set tags, logs, and operation name. { std::lock_guard lock_guard{mutex_}; - span.set_operation_name(std::move(operation_name_)); - auto tags = span.mutable_tags(); + span_.set_operation_name(std::move(operation_name_)); + auto tags = span_.mutable_tags(); tags->Reserve(static_cast(tags_.size())); for (const auto& tag : tags_) { try { @@ -199,7 +199,7 @@ void LightStepSpan::FinishWithOptions( R"(": )", e.what()); } } - auto logs = span.mutable_logs(); + auto logs = span_.mutable_logs(); logs->Reserve(static_cast(logs_.size()) + static_cast(options.log_records.size())); for (auto& log : logs_) { @@ -225,7 +225,7 @@ void LightStepSpan::FinishWithOptions( } // Set the span context. - auto span_context = span.mutable_span_context(); + auto span_context = span_.mutable_span_context(); span_context->set_trace_id(span_context_.trace_id()); span_context->set_span_id(span_context_.span_id()); auto baggage = span_context->mutable_baggage(); @@ -237,7 +237,7 @@ void LightStepSpan::FinishWithOptions( }); // Record the span - recorder_.RecordSpan(std::move(span)); + recorder_.RecordSpan(std::move(span_)); } catch (const std::exception& e) { logger_.Error("FinishWithOptions failed: ", e.what()); } diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 16d1c54d..5fa5ee79 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -63,6 +63,7 @@ class LightStepSpan : public opentracing::Span { // Mutex protects tags_, logs_, and operation_name_. std::mutex mutex_; + /* collector::Span span_; */ std::string operation_name_; std::unordered_map tags_; std::vector logs_; From fa7edf43d943307a6a1445ee4a4a9f64e82b73d0 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Aug 2018 23:13:19 -0700 Subject: [PATCH 02/16] Use protobuf operation name. --- src/lightstep_span.cpp | 13 +++++-------- src/lightstep_span.h | 3 +-- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 485e7b2c..1fc54a14 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -93,10 +93,10 @@ LightStepSpan::LightStepSpan( std::shared_ptr&& tracer, Logger& logger, Recorder& recorder, opentracing::string_view operation_name, const opentracing::StartSpanOptions& options) - : tracer_{std::move(tracer)}, - logger_{logger}, - recorder_{recorder}, - operation_name_{operation_name} { + : tracer_{std::move(tracer)}, logger_{logger}, recorder_{recorder} { + + span_.set_operation_name(operation_name.data(), operation_name.size()); + // Set the start timestamps. std::tie(start_timestamp_, start_steady_) = ComputeStartTimestamps( options.start_system_timestamp, options.start_steady_timestamp); @@ -170,8 +170,6 @@ void LightStepSpan::FinishWithOptions( finish_timestamp = SteadyClock::now(); } - collector::Span span_; - // Set timing information. auto duration = finish_timestamp - start_steady_; span_.set_duration_micros( @@ -188,7 +186,6 @@ void LightStepSpan::FinishWithOptions( // Set tags, logs, and operation name. { std::lock_guard lock_guard{mutex_}; - span_.set_operation_name(std::move(operation_name_)); auto tags = span_.mutable_tags(); tags->Reserve(static_cast(tags_.size())); for (const auto& tag : tags_) { @@ -248,7 +245,7 @@ void LightStepSpan::FinishWithOptions( void LightStepSpan::SetOperationName( opentracing::string_view name) noexcept try { std::lock_guard lock_guard{mutex_}; - operation_name_ = name; + span_.set_operation_name(name.data(), name.size()); } catch (const std::exception& e) { logger_.Error("SetOperationName failed: ", e.what()); } diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 5fa5ee79..696d98e4 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -63,8 +63,7 @@ class LightStepSpan : public opentracing::Span { // Mutex protects tags_, logs_, and operation_name_. std::mutex mutex_; - /* collector::Span span_; */ - std::string operation_name_; + collector::Span span_; std::unordered_map tags_; std::vector logs_; }; From b5df35eb8472e3e7bfe27d5afb12cbd9c2d78acb Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Aug 2018 23:23:32 -0700 Subject: [PATCH 03/16] Avoid storing start_timestamp_. --- src/lightstep_span.cpp | 5 +++-- src/lightstep_span.h | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 1fc54a14..03d229ae 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -98,8 +98,10 @@ LightStepSpan::LightStepSpan( span_.set_operation_name(operation_name.data(), operation_name.size()); // Set the start timestamps. - std::tie(start_timestamp_, start_steady_) = ComputeStartTimestamps( + std::chrono::system_clock::time_point start_timestamp; + std::tie(start_timestamp, start_steady_) = ComputeStartTimestamps( options.start_system_timestamp, options.start_steady_timestamp); + *span_.mutable_start_timestamp() = ToTimestamp(start_timestamp); // Set any span references. std::unordered_map baggage; @@ -174,7 +176,6 @@ void LightStepSpan::FinishWithOptions( auto duration = finish_timestamp - start_steady_; span_.set_duration_micros( std::chrono::duration_cast(duration).count()); - *span_.mutable_start_timestamp() = ToTimestamp(start_timestamp_); // Set references. auto references = span_.mutable_references(); diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 696d98e4..0988aaae 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -55,7 +55,6 @@ class LightStepSpan : public opentracing::Span { Logger& logger_; Recorder& recorder_; std::vector references_; - std::chrono::system_clock::time_point start_timestamp_; std::chrono::steady_clock::time_point start_steady_; LightStepSpanContext span_context_; From 33f0b652875fb997649263ecc0e5f87d3c9840b3 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 11:03:53 -0700 Subject: [PATCH 04/16] Don't use embedded span context. --- src/auto_recorder.cpp | 4 +- src/auto_recorder.h | 2 +- src/lightstep_span.cpp | 80 +++++---- src/lightstep_span.h | 54 +++++- src/lightstep_span_context.cpp | 6 +- src/lightstep_span_context.h | 67 ++++++-- src/lightstep_tracer_impl.cpp | 2 +- src/manual_recorder.cpp | 4 +- src/manual_recorder.h | 2 +- src/propagation.cpp | 293 +++++++++++++++++++++++++++++++++ src/propagation.h | 27 +++ src/recorder.h | 2 +- src/report_builder.cpp | 2 +- src/report_builder.h | 2 +- test/in_memory_recorder.h | 4 +- test/tracer_test.cpp | 293 +++++++++++++++++---------------- 16 files changed, 637 insertions(+), 207 deletions(-) diff --git a/src/auto_recorder.cpp b/src/auto_recorder.cpp index 238b381d..88df8170 100644 --- a/src/auto_recorder.cpp +++ b/src/auto_recorder.cpp @@ -40,7 +40,7 @@ AutoRecorder::~AutoRecorder() { //------------------------------------------------------------------------------ // RecordSpan //------------------------------------------------------------------------------ -void AutoRecorder::RecordSpan(collector::Span&& span) noexcept try { +void AutoRecorder::RecordSpan(const collector::Span& span) noexcept try { std::lock_guard lock_guard{write_mutex_}; if (builder_.num_pending_spans() >= max_buffered_spans_snapshot_ || write_exit_) { @@ -48,7 +48,7 @@ void AutoRecorder::RecordSpan(collector::Span&& span) noexcept try { options_.metrics_observer->OnSpansDropped(1); return; } - builder_.AddSpan(std::move(span)); + builder_.AddSpan(span); if (builder_.num_pending_spans() >= max_buffered_spans_snapshot_) { write_cond_->NotifyAll(); } diff --git a/src/auto_recorder.h b/src/auto_recorder.h index 295012ff..9472133c 100644 --- a/src/auto_recorder.h +++ b/src/auto_recorder.h @@ -32,7 +32,7 @@ class AutoRecorder : public Recorder { ~AutoRecorder() override; - void RecordSpan(collector::Span&& span) noexcept override; + void RecordSpan(const collector::Span& span) noexcept override; bool FlushWithTimeout( std::chrono::system_clock::duration timeout) noexcept override; diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 03d229ae..382a747e 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -1,5 +1,6 @@ #include "lightstep_span.h" #include +#include #include "utility.h" using opentracing::SystemTime; @@ -50,9 +51,8 @@ static bool SetSpanReference( Logger& logger, const std::pair& reference, - std::unordered_map& baggage, - collector::Reference& collector_reference, bool& sampled) { - collector_reference.Clear(); + BaggageMap& baggage, collector::Reference& collector_reference, + bool& sampled) { switch (reference.first) { case opentracing::SpanReferenceType::ChildOfRef: collector_reference.set_relationship(collector::Reference::CHILD_OF); @@ -66,11 +66,12 @@ static bool SetSpanReference( return false; } auto referenced_context = - dynamic_cast(reference.second); + dynamic_cast(reference.second); if (referenced_context == nullptr) { logger.Warn("Passed in span reference of unexpected type."); return false; } + std::cout << "trace_id = " << referenced_context->trace_id() << std::endl; collector_reference.mutable_span_context()->set_trace_id( referenced_context->trace_id()); collector_reference.mutable_span_context()->set_span_id( @@ -94,8 +95,9 @@ LightStepSpan::LightStepSpan( Recorder& recorder, opentracing::string_view operation_name, const opentracing::StartSpanOptions& options) : tracer_{std::move(tracer)}, logger_{logger}, recorder_{recorder} { - span_.set_operation_name(operation_name.data(), operation_name.size()); + auto& span_context = *span_.mutable_span_context(); + auto& baggage = *span_context.mutable_baggage(); // Set the start timestamps. std::chrono::system_clock::time_point start_timestamp; @@ -104,13 +106,12 @@ LightStepSpan::LightStepSpan( *span_.mutable_start_timestamp() = ToTimestamp(start_timestamp); // Set any span references. - std::unordered_map baggage; references_.reserve(options.references.size()); collector::Reference collector_reference; - bool sampled = false; + sampled_ = false; for (auto& reference : options.references) { if (!SetSpanReference(logger_, reference, baggage, collector_reference, - sampled)) { + sampled_)) { continue; } references_.push_back(collector_reference); @@ -119,7 +120,7 @@ LightStepSpan::LightStepSpan( // If there are any span references, sampled should be true if any of the // references are sampled; with no refences, we set sampled to true. if (references_.empty()) { - sampled = true; + sampled_ = true; } // Set tags. @@ -131,7 +132,7 @@ LightStepSpan::LightStepSpan( // derived from the referenced spans. auto sampling_priority_tag = tags_.find(opentracing::ext::sampling_priority); if (sampling_priority_tag != tags_.end()) { - sampled = is_sampled(sampling_priority_tag->second); + sampled_ = is_sampled(sampling_priority_tag->second); } // Set opentracing::SpanContext. @@ -139,8 +140,8 @@ LightStepSpan::LightStepSpan( ? GenerateId() : references_[0].span_context().trace_id(); auto span_id = GenerateId(); - span_context_ = - LightStepSpanContext{trace_id, span_id, sampled, std::move(baggage)}; + span_context.set_trace_id(trace_id); + span_context.set_span_id(span_id); } //------------------------------------------------------------------------------ @@ -162,8 +163,8 @@ void LightStepSpan::FinishWithOptions( return; } - // If the span isn't sampled do nothing. - if (!span_context_.sampled()) { + std::lock_guard lock_guard{mutex_}; + if (!sampled_) { return; } @@ -186,7 +187,6 @@ void LightStepSpan::FinishWithOptions( // Set tags, logs, and operation name. { - std::lock_guard lock_guard{mutex_}; auto tags = span_.mutable_tags(); tags->Reserve(static_cast(tags_.size())); for (const auto& tag : tags_) { @@ -222,20 +222,8 @@ void LightStepSpan::FinishWithOptions( } } - // Set the span context. - auto span_context = span_.mutable_span_context(); - span_context->set_trace_id(span_context_.trace_id()); - span_context->set_span_id(span_context_.span_id()); - auto baggage = span_context->mutable_baggage(); - span_context_.ForeachBaggageItem( - [baggage](const std::string& key, const std::string& value) { - using StringMap = google::protobuf::Map; - baggage->insert(StringMap::value_type(key, value)); - return true; - }); - // Record the span - recorder_.RecordSpan(std::move(span_)); + recorder_.RecordSpan(span_); } catch (const std::exception& e) { logger_.Error("FinishWithOptions failed: ", e.what()); } @@ -260,7 +248,7 @@ void LightStepSpan::SetTag(opentracing::string_view key, tags_[key] = value; if (key == opentracing::ext::sampling_priority) { - span_context_.set_sampled(is_sampled(value)); + sampled_ = is_sampled(value); } } catch (const std::exception& e) { logger_.Error("SetTag failed: ", e.what()); @@ -271,7 +259,9 @@ void LightStepSpan::SetTag(opentracing::string_view key, //------------------------------------------------------------------------------ void LightStepSpan::SetBaggageItem(opentracing::string_view restricted_key, opentracing::string_view value) noexcept { - span_context_.set_baggage_item(restricted_key, value); + std::lock_guard lock_guard{mutex_}; + auto& baggage = *span_.mutable_span_context()->mutable_baggage(); + baggage.insert(BaggageMap::value_type(restricted_key, value)); } //------------------------------------------------------------------------------ @@ -279,7 +269,13 @@ void LightStepSpan::SetBaggageItem(opentracing::string_view restricted_key, //------------------------------------------------------------------------------ std::string LightStepSpan::BaggageItem( opentracing::string_view restricted_key) const noexcept try { - return span_context_.baggage_item(restricted_key); + std::lock_guard lock_guard{mutex_}; + auto& baggage = span_.span_context().baggage(); + auto lookup = baggage.find(restricted_key); + if (lookup != baggage.end()) { + return lookup->second; + } + return {}; } catch (const std::exception& e) { logger_.Error("BaggageItem failed, returning empty string: ", e.what()); return {}; @@ -302,4 +298,26 @@ void LightStepSpan::Log(std::initializer_list< } catch (const std::exception& e) { logger_.Error("Log failed: ", e.what()); } + +//------------------------------------------------------------------------------ +// ForeachBaggageItem +//------------------------------------------------------------------------------ +void LightStepSpan::ForeachBaggageItem( + std::function f) + const { + std::lock_guard lock_guard{mutex_}; + for (const auto& baggage_item : span_.span_context().baggage()) { + if (!f(baggage_item.first, baggage_item.second)) { + return; + } + } +} + +//------------------------------------------------------------------------------ +// sampled +//------------------------------------------------------------------------------ +bool LightStepSpan::sampled() const noexcept { + std::lock_guard lock_guard{mutex_}; + return sampled_; +} } // namespace lightstep diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 0988aaae..79138a15 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -10,7 +10,8 @@ #include "recorder.h" namespace lightstep { -class LightStepSpan : public opentracing::Span { +class LightStepSpan final : public opentracing::Span, + public LightStepSpanContextBase { public: LightStepSpan(std::shared_ptr&& tracer, Logger& logger, Recorder& recorder, @@ -43,27 +44,70 @@ class LightStepSpan : public opentracing::Span { fields) noexcept override; const opentracing::SpanContext& context() const noexcept override { - return span_context_; + return *this; } const opentracing::Tracer& tracer() const noexcept override { return *tracer_; } + void ForeachBaggageItem( + std::function f) + const override; + + bool sampled() const noexcept override; + + uint64_t trace_id() const noexcept override { + return span_.span_context().trace_id(); + } + + uint64_t span_id() const noexcept override { + return span_.span_context().span_id(); + } + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + std::ostream& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::TextMapWriter& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::HTTPHeadersWriter& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + private: // Fields set in StartSpan() are not protected by a mutex. + collector::Span span_; + mutable std::mutex mutex_; + bool sampled_; std::shared_ptr tracer_; Logger& logger_; Recorder& recorder_; std::vector references_; std::chrono::steady_clock::time_point start_steady_; - LightStepSpanContext span_context_; + /* LightStepSpanContext span_context_; */ std::atomic is_finished_{false}; // Mutex protects tags_, logs_, and operation_name_. - std::mutex mutex_; - collector::Span span_; std::unordered_map tags_; std::vector logs_; + + template + opentracing::expected InjectImpl( + const PropagationOptions& propagation_options, Carrier& writer) const { + std::lock_guard lock_guard{mutex_}; + auto& span_context = span_.span_context(); + return InjectSpanContext(propagation_options, writer, + span_context.trace_id(), span_context.span_id(), + sampled_, span_context.baggage()); + } }; } // namespace lightstep diff --git a/src/lightstep_span_context.cpp b/src/lightstep_span_context.cpp index 751f96bc..07ab09db 100644 --- a/src/lightstep_span_context.cpp +++ b/src/lightstep_span_context.cpp @@ -86,9 +86,9 @@ void LightStepSpanContext::set_sampled(bool sampled) noexcept { //------------------------------------------------------------------------------ // operator== //------------------------------------------------------------------------------ -bool operator==(const LightStepSpanContext& lhs, - const LightStepSpanContext& rhs) { - auto extract_baggage = [](const LightStepSpanContext& span_context) { +bool operator==(const LightStepSpanContextBase& lhs, + const LightStepSpanContextBase& rhs) { + auto extract_baggage = [](const LightStepSpanContextBase& span_context) { std::unordered_map baggage; span_context.ForeachBaggageItem( [&](const std::string& key, const std::string& value) { diff --git a/src/lightstep_span_context.h b/src/lightstep_span_context.h index 12052383..7567bba9 100644 --- a/src/lightstep_span_context.h +++ b/src/lightstep_span_context.h @@ -8,7 +8,28 @@ #include "propagation.h" namespace lightstep { -class LightStepSpanContext : public opentracing::SpanContext { +class LightStepSpanContextBase : public opentracing::SpanContext { + public: + virtual bool sampled() const noexcept = 0; + + virtual uint64_t trace_id() const noexcept = 0; + + virtual uint64_t span_id() const noexcept = 0; + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + std::ostream& writer) const = 0; + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::TextMapWriter& writer) const = 0; + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::HTTPHeadersWriter& writer) const = 0; +}; + +class LightStepSpanContext final : public LightStepSpanContextBase { public: LightStepSpanContext() = default; @@ -37,12 +58,22 @@ class LightStepSpanContext : public opentracing::SpanContext { std::function f) const override; - template - opentracing::expected Inject( - const PropagationOptions& propagation_options, Carrier& writer) const { - std::lock_guard lock_guard{mutex_}; - return InjectSpanContext(propagation_options, writer, trace_id_, span_id_, - sampled_, baggage_); + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + std::ostream& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::TextMapWriter& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + virtual opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::HTTPHeadersWriter& writer) const override { + return this->InjectImpl(propagation_options, writer); } template @@ -53,10 +84,10 @@ class LightStepSpanContext : public opentracing::SpanContext { sampled_, baggage_); } - uint64_t trace_id() const noexcept { return trace_id_; } - uint64_t span_id() const noexcept { return span_id_; } + uint64_t trace_id() const noexcept override { return trace_id_; } + uint64_t span_id() const noexcept override { return span_id_; } - bool sampled() const noexcept; + bool sampled() const noexcept override; void set_sampled(bool sampled) noexcept; private: @@ -66,13 +97,21 @@ class LightStepSpanContext : public opentracing::SpanContext { mutable std::mutex mutex_; bool sampled_ = true; std::unordered_map baggage_; + + template + opentracing::expected InjectImpl( + const PropagationOptions& propagation_options, Carrier& writer) const { + std::lock_guard lock_guard{mutex_}; + return InjectSpanContext(propagation_options, writer, trace_id_, span_id_, + sampled_, baggage_); + } }; -bool operator==(const LightStepSpanContext& lhs, - const LightStepSpanContext& rhs); +bool operator==(const LightStepSpanContextBase& lhs, + const LightStepSpanContextBase& rhs); -inline bool operator!=(const LightStepSpanContext& lhs, - const LightStepSpanContext& rhs) { +inline bool operator!=(const LightStepSpanContextBase& lhs, + const LightStepSpanContextBase& rhs) { return !(lhs == rhs); } } // namespace lightstep diff --git a/src/lightstep_tracer_impl.cpp b/src/lightstep_tracer_impl.cpp index 44ba926a..5bfecfbe 100644 --- a/src/lightstep_tracer_impl.cpp +++ b/src/lightstep_tracer_impl.cpp @@ -12,7 +12,7 @@ static opentracing::expected InjectImpl( const PropagationOptions& propagation_options, const opentracing::SpanContext& span_context, Carrier& writer) { auto lightstep_span_context = - dynamic_cast(&span_context); + dynamic_cast(&span_context); if (lightstep_span_context == nullptr) { return opentracing::make_unexpected( opentracing::invalid_span_context_error); diff --git a/src/manual_recorder.cpp b/src/manual_recorder.cpp index d73137a6..2bc44650 100644 --- a/src/manual_recorder.cpp +++ b/src/manual_recorder.cpp @@ -20,7 +20,7 @@ ManualRecorder::ManualRecorder(Logger& logger, LightStepTracerOptions options, //------------------------------------------------------------------------------ // RecordSpan //------------------------------------------------------------------------------ -void ManualRecorder::RecordSpan(collector::Span&& span) noexcept try { +void ManualRecorder::RecordSpan(const collector::Span& span) noexcept try { if (disabled_) { dropped_spans_++; options_.metrics_observer->OnSpansDropped(1); @@ -41,7 +41,7 @@ void ManualRecorder::RecordSpan(collector::Span&& span) noexcept try { return; } } - builder_.AddSpan(std::move(span)); + builder_.AddSpan(span); if (builder_.num_pending_spans() >= max_buffered_spans) { FlushOne(); } diff --git a/src/manual_recorder.h b/src/manual_recorder.h index 01b7fa38..645b2a64 100644 --- a/src/manual_recorder.h +++ b/src/manual_recorder.h @@ -13,7 +13,7 @@ class ManualRecorder : public Recorder, private AsyncTransporter::Callback { ManualRecorder(Logger& logger, LightStepTracerOptions options, std::unique_ptr&& transporter); - void RecordSpan(collector::Span&& span) noexcept override; + void RecordSpan(const collector::Span& span) noexcept override; bool FlushWithTimeout( std::chrono::system_clock::duration timeout) noexcept override; diff --git a/src/propagation.cpp b/src/propagation.cpp index effd4640..fc3584f3 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -123,6 +123,50 @@ static opentracing::expected InjectSpanContextMultiKey( return {}; } +static opentracing::expected InjectSpanContextMultiKey( + const opentracing::TextMapWriter& carrier, uint64_t trace_id, + uint64_t span_id, bool sampled, const BaggageMap& baggage) { + std::string trace_id_hex, span_id_hex, baggage_key; + try { + trace_id_hex = Uint64ToHex(trace_id); + span_id_hex = Uint64ToHex(span_id); + baggage_key = PrefixBaggage; + } catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); + } + auto result = carrier.Set(FieldNameTraceID, trace_id_hex); + if (!result) { + return result; + } + result = carrier.Set(FieldNameSpanID, span_id_hex); + if (!result) { + return result; + } + if (sampled) { + result = carrier.Set(FieldNameSampled, "true"); + } else { + result = carrier.Set(FieldNameSampled, "false"); + } + if (!result) { + return result; + } + for (const auto& baggage_item : baggage) { + try { + baggage_key.replace(std::begin(baggage_key) + PrefixBaggage.size(), + std::end(baggage_key), baggage_item.first); + } catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); + } + result = carrier.Set(baggage_key, baggage_item.second); + if (!result) { + return result; + } + } + return {}; +} + //------------------------------------------------------------------------------ // InjectSpanContextSingleKey //------------------------------------------------------------------------------ @@ -153,6 +197,32 @@ static opentracing::expected InjectSpanContextSingleKey( return {}; } +static opentracing::expected InjectSpanContextSingleKey( + const opentracing::TextMapWriter& carrier, uint64_t trace_id, + uint64_t span_id, bool sampled, const BaggageMap& baggage) { + std::ostringstream ostream; + auto result = InjectSpanContext(PropagationOptions{}, ostream, trace_id, + span_id, sampled, baggage); + if (!result) { + return result; + } + std::string context_value; + try { + auto binary_encoding = ostream.str(); + context_value = + Base64::encode(binary_encoding.data(), binary_encoding.size()); + } catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); + } + + result = carrier.Set(PropagationSingleKey, context_value); + if (!result) { + return result; + } + return {}; +} + //------------------------------------------------------------------------------ // InjectSpanContext //------------------------------------------------------------------------------ @@ -213,6 +283,62 @@ opentracing::expected InjectSpanContext( baggage); } +static opentracing::expected InjectSpanContext( + BinaryCarrier& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, + const BaggageMap& baggage) noexcept try { + carrier.Clear(); + auto basic = carrier.mutable_basic_ctx(); + basic->set_trace_id(trace_id); + basic->set_span_id(span_id); + basic->set_sampled(sampled); + auto mutable_baggage = basic->mutable_baggage_items(); + for (auto& baggage_item : baggage) { + (*mutable_baggage)[baggage_item.first] = baggage_item.second; + } + return {}; +} catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); +} + +opentracing::expected InjectSpanContext( + const PropagationOptions& /*propagation_options*/, std::ostream& carrier, + uint64_t trace_id, uint64_t span_id, bool sampled, + const BaggageMap& baggage) { + BinaryCarrier binary_carrier; + auto result = + InjectSpanContext(binary_carrier, trace_id, span_id, sampled, baggage); + if (!result) { + return result; + } + if (!binary_carrier.SerializeToOstream(&carrier)) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::io_error)); + } + + // Flush so that when we call carrier.good(), we'll get an accurate view of + // the error state. + carrier.flush(); + if (!carrier.good()) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::io_error)); + } + + return {}; +} + +opentracing::expected InjectSpanContext( + const PropagationOptions& propagation_options, + const opentracing::TextMapWriter& carrier, uint64_t trace_id, + uint64_t span_id, bool sampled, const BaggageMap& baggage) { + if (propagation_options.use_single_key) { + return InjectSpanContextSingleKey(carrier, trace_id, span_id, sampled, + baggage); + } + return InjectSpanContextMultiKey(carrier, trace_id, span_id, sampled, + baggage); +} + //------------------------------------------------------------------------------ // ExtractSpanContextMultiKey //------------------------------------------------------------------------------ @@ -263,6 +389,52 @@ static opentracing::expected ExtractSpanContextMultiKey( return true; } +template +static opentracing::expected ExtractSpanContextMultiKey( + const opentracing::TextMapReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage, + KeyCompare key_compare) { + int count = 0; + auto result = carrier.ForeachKey([&](opentracing::string_view key, + opentracing::string_view value) + -> opentracing::expected { + try { + if (key_compare(key, FieldNameTraceID)) { + trace_id = HexToUint64(value); + count++; + } else if (key_compare(key, FieldNameSpanID)) { + span_id = HexToUint64(value); + count++; + } else if (key_compare(key, FieldNameSampled)) { + sampled = !(value == "false" || value == "0"); + count++; + } else if (key.length() > PrefixBaggage.size() && + key_compare( + opentracing::string_view{key.data(), PrefixBaggage.size()}, + PrefixBaggage)) { + baggage.insert(BaggageMap::value_type( + std::string{std::begin(key) + PrefixBaggage.size(), std::end(key)}, + value)); + } + return {}; + } catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); + } + }); + if (!result) { + return opentracing::make_unexpected(result.error()); + } + if (count == 0) { + return false; + } + if (count > 0 && count != FieldCount) { + return opentracing::make_unexpected( + opentracing::span_context_corrupted_error); + } + return true; +} + //------------------------------------------------------------------------------ // ExtractSpanContextSingleKey //------------------------------------------------------------------------------ @@ -296,6 +468,35 @@ static opentracing::expected ExtractSpanContextSingleKey( sampled, baggage); } +template +static opentracing::expected ExtractSpanContextSingleKey( + const opentracing::TextMapReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage, + KeyCompare key_compare) { + auto value_maybe = LookupKey(carrier, PropagationSingleKey, key_compare); + if (!value_maybe) { + if (value_maybe.error() == opentracing::key_not_found_error) { + return false; + } + return opentracing::make_unexpected(value_maybe.error()); + } + auto value = *value_maybe; + std::string base64_decoding; + try { + base64_decoding = Base64::decode(value.data(), value.size()); + } catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); + } + if (base64_decoding.empty()) { + return opentracing::make_unexpected( + opentracing::span_context_corrupted_error); + } + in_memory_stream istream{base64_decoding.data(), base64_decoding.size()}; + return ExtractSpanContext(PropagationOptions{}, istream, trace_id, span_id, + sampled, baggage); +} + //------------------------------------------------------------------------------ // ExtractSpanContext //------------------------------------------------------------------------------ @@ -396,4 +597,96 @@ opentracing::expected ExtractSpanContext( return ExtractSpanContext(propagation_options, carrier, trace_id, span_id, sampled, baggage, iequals); } + +static opentracing::expected ExtractSpanContext( + const BinaryCarrier& carrier, uint64_t& trace_id, uint64_t& span_id, + bool& sampled, BaggageMap& baggage) noexcept try { + auto& basic = carrier.basic_ctx(); + trace_id = basic.trace_id(); + span_id = basic.span_id(); + sampled = basic.sampled(); + baggage = basic.baggage_items(); + return true; +} catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); +} + +opentracing::expected ExtractSpanContext( + const PropagationOptions& /*propagation_options*/, std::istream& carrier, + uint64_t& trace_id, uint64_t& span_id, bool& sampled, + BaggageMap& baggage) try { + // istream::peek returns EOF if it's in an error state, so check for an error + // state first before checking for an empty stream. + if (!carrier.good()) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::io_error)); + } + + // Check for the case when no span is encoded. + if (carrier.peek() == EOF) { + return false; + } + + BinaryCarrier binary_carrier; + if (!binary_carrier.ParseFromIstream(&carrier)) { + return opentracing::make_unexpected( + opentracing::span_context_corrupted_error); + } + return ExtractSpanContext(binary_carrier, trace_id, span_id, sampled, + baggage); +} catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::not_enough_memory)); +} + +template +static opentracing::expected ExtractSpanContext( + const PropagationOptions& propagation_options, + const opentracing::TextMapReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage, + KeyCompare key_compare) { + if (propagation_options.use_single_key) { + auto span_context_maybe = ExtractSpanContextSingleKey( + carrier, trace_id, span_id, sampled, baggage, key_compare); + + // If no span context was found, fall back to multikey extraction so as to + // support interoperability with other tracers. + if (!span_context_maybe || *span_context_maybe) { + return span_context_maybe; + } + } + + return ExtractSpanContextMultiKey(carrier, trace_id, span_id, sampled, + baggage, key_compare); +} + +opentracing::expected ExtractSpanContext( + const PropagationOptions& propagation_options, + const opentracing::TextMapReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage) { + return ExtractSpanContext(propagation_options, carrier, trace_id, span_id, + sampled, baggage, + std::equal_to()); +} + +// HTTP header field names are case insensitive, so we need to ignore case when +// comparing against the OpenTracing field names. +// +// See https://stackoverflow.com/a/5259004/4447365 +opentracing::expected ExtractSpanContext( + const PropagationOptions& propagation_options, + const opentracing::HTTPHeadersReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage) { + auto iequals = [](opentracing::string_view lhs, + opentracing::string_view rhs) { + return lhs.length() == rhs.length() && + std::equal(std::begin(lhs), std::end(lhs), std::begin(rhs), + [](char a, char b) { + return std::tolower(a) == std::tolower(b); + }); + }; + return ExtractSpanContext(propagation_options, carrier, trace_id, span_id, + sampled, baggage, iequals); +} } // namespace lightstep diff --git a/src/propagation.h b/src/propagation.h index f4cb0372..96ac38dc 100644 --- a/src/propagation.h +++ b/src/propagation.h @@ -1,13 +1,26 @@ #pragma once +#include #include #include namespace lightstep { +using BaggageMap = google::protobuf::Map; + struct PropagationOptions { bool use_single_key = false; }; +opentracing::expected InjectSpanContext( + const PropagationOptions& propagation_options, std::ostream& carrier, + uint64_t trace_id, uint64_t span_id, bool sampled, + const BaggageMap& baggage); + +opentracing::expected InjectSpanContext( + const PropagationOptions& propagation_options, + const opentracing::TextMapWriter& carrier, uint64_t trace_id, + uint64_t span_id, bool sampled, const BaggageMap& baggage); + opentracing::expected InjectSpanContext( const PropagationOptions& propagation_options, std::ostream& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, @@ -35,4 +48,18 @@ opentracing::expected ExtractSpanContext( const opentracing::HTTPHeadersReader& carrier, uint64_t& trace_id, uint64_t& span_id, bool& sampled, std::unordered_map& baggage); + +opentracing::expected ExtractSpanContext( + const PropagationOptions& propagation_options, std::istream& carrier, + uint64_t& trace_id, uint64_t& span_id, bool& sampled, BaggageMap& baggage); + +opentracing::expected ExtractSpanContext( + const PropagationOptions& propagation_options, + const opentracing::TextMapReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage); + +opentracing::expected ExtractSpanContext( + const PropagationOptions& propagation_options, + const opentracing::HTTPHeadersReader& carrier, uint64_t& trace_id, + uint64_t& span_id, bool& sampled, BaggageMap& baggage); } // namespace lightstep diff --git a/src/recorder.h b/src/recorder.h index e094f7a4..5e6b5882 100644 --- a/src/recorder.h +++ b/src/recorder.h @@ -10,7 +10,7 @@ class Recorder { public: virtual ~Recorder() = default; - virtual void RecordSpan(collector::Span&& span) noexcept = 0; + virtual void RecordSpan(const collector::Span& span) noexcept = 0; virtual bool FlushWithTimeout( std::chrono::system_clock::duration /*timeout*/) noexcept { diff --git a/src/report_builder.cpp b/src/report_builder.cpp index eef06594..9f640958 100644 --- a/src/report_builder.cpp +++ b/src/report_builder.cpp @@ -20,7 +20,7 @@ ReportBuilder::ReportBuilder( //------------------------------------------------------------------------------ // AddSpan //------------------------------------------------------------------------------ -void ReportBuilder::AddSpan(collector::Span&& span) { +void ReportBuilder::AddSpan(const collector::Span& span) { if (reset_next_) { pending_.Clear(); pending_.CopyFrom(preamble_); diff --git a/src/report_builder.h b/src/report_builder.h index 9be75c2d..3a474e5f 100644 --- a/src/report_builder.h +++ b/src/report_builder.h @@ -16,7 +16,7 @@ class ReportBuilder { const std::unordered_map& tags); // AddSpan adds the span to the currently-building ReportRequest. - void AddSpan(collector::Span&& span); + void AddSpan(const collector::Span& span); // num_pending_spans() is the number of pending spans. size_t num_pending_spans() const { return pending_.spans_size(); } diff --git a/test/in_memory_recorder.h b/test/in_memory_recorder.h index 2ee8d974..519a99a7 100644 --- a/test/in_memory_recorder.h +++ b/test/in_memory_recorder.h @@ -9,9 +9,9 @@ namespace lightstep { // InMemoryRecorder is used for testing only. class InMemoryRecorder : public Recorder { public: - void RecordSpan(collector::Span&& span) noexcept override { + void RecordSpan(const collector::Span& span) noexcept override { std::lock_guard lock_guard{mutex_}; - spans_.emplace_back(std::move(span)); + spans_.emplace_back(span); } std::vector spans() const { diff --git a/test/tracer_test.cpp b/test/tracer_test.cpp index 13143797..a8e0fef8 100644 --- a/test/tracer_test.cpp +++ b/test/tracer_test.cpp @@ -16,55 +16,58 @@ TEST_CASE("tracer") { auto tracer = std::shared_ptr{new LightStepTracerImpl{ PropagationOptions{}, std::unique_ptr{recorder}}}; - SECTION("StartSpan applies the provided tags.") { - { - auto span = - tracer->StartSpan("a", {SetTag("abc", 123), SetTag("xyz", true)}); - CHECK(span); - span->Finish(); - } - auto span = recorder->top(); - CHECK(span.operation_name() == "a"); - CHECK(HasTag(span, "abc", 123)); - CHECK(HasTag(span, "xyz", true)); - } + /* SECTION("StartSpan applies the provided tags.") { */ + /* { */ + /* auto span = */ + /* tracer->StartSpan("a", {SetTag("abc", 123), SetTag("xyz", true)}); + */ + /* CHECK(span); */ + /* span->Finish(); */ + /* } */ + /* auto span = recorder->top(); */ + /* CHECK(span.operation_name() == "a"); */ + /* CHECK(HasTag(span, "abc", 123)); */ + /* CHECK(HasTag(span, "xyz", true)); */ + /* } */ - SECTION( - "Sampling of a span can be turned off by setting the sampling_priority " - "tag to 0.") { - { - auto span = tracer->StartSpan( - "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); - CHECK(span); - } - { - auto span = tracer->StartSpan("a"); - span->SetTag(opentracing::ext::sampling_priority, 0); - CHECK(span); - } - CHECK(recorder->size() == 0); - } + /* SECTION( */ + /* "Sampling of a span can be turned off by setting the sampling_priority + * " */ + /* "tag to 0.") { */ + /* { */ + /* auto span = tracer->StartSpan( */ + /* "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); */ + /* CHECK(span); */ + /* } */ + /* { */ + /* auto span = tracer->StartSpan("a"); */ + /* span->SetTag(opentracing::ext::sampling_priority, 0); */ + /* CHECK(span); */ + /* } */ + /* CHECK(recorder->size() == 0); */ + /* } */ - SECTION( - "If a span's parent isn't sampled, then it also isn't sampled unless " - "sampling priority is overwritten.") { - auto parent_span = tracer->StartSpan( - "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); - CHECK(parent_span); - { - auto child_span = tracer->StartSpan( - "b", {opentracing::ChildOf(&parent_span->context())}); - CHECK(child_span); - } - CHECK(recorder->size() == 0); - { - auto child_span = tracer->StartSpan( - "b", {opentracing::ChildOf(&parent_span->context())}); - CHECK(child_span); - child_span->SetTag(opentracing::ext::sampling_priority, 1); - } - CHECK(recorder->size() == 1); - } + /* SECTION( */ + /* "If a span's parent isn't sampled, then it also isn't sampled unless " + */ + /* "sampling priority is overwritten.") { */ + /* auto parent_span = tracer->StartSpan( */ + /* "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); */ + /* CHECK(parent_span); */ + /* { */ + /* auto child_span = tracer->StartSpan( */ + /* "b", {opentracing::ChildOf(&parent_span->context())}); */ + /* CHECK(child_span); */ + /* } */ + /* CHECK(recorder->size() == 0); */ + /* { */ + /* auto child_span = tracer->StartSpan( */ + /* "b", {opentracing::ChildOf(&parent_span->context())}); */ + /* CHECK(child_span); */ + /* child_span->SetTag(opentracing::ext::sampling_priority, 1); */ + /* } */ + /* CHECK(recorder->size() == 1); */ + /* } */ SECTION("You can set a single child-of reference when starting a span.") { auto span_a = tracer->StartSpan("a"); @@ -80,107 +83,113 @@ TEST_CASE("tracer") { spans.at(0))); } - SECTION("You can set a single follows-from reference when starting a span.") { - auto span_a = tracer->StartSpan("a"); - CHECK(span_a); - span_a->Finish(); - auto span_b = tracer->StartSpan("b", {FollowsFrom(&span_a->context())}); - CHECK(span_b); - span_b->Finish(); - auto spans = recorder->spans(); - CHECK(spans.at(0).span_context().trace_id() == - spans.at(1).span_context().trace_id()); - CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(1), - spans.at(0))); - } + /* SECTION("You can set a single follows-from reference when starting a + * span.") { */ + /* auto span_a = tracer->StartSpan("a"); */ + /* CHECK(span_a); */ + /* span_a->Finish(); */ + /* auto span_b = tracer->StartSpan("b", {FollowsFrom(&span_a->context())}); + */ + /* CHECK(span_b); */ + /* span_b->Finish(); */ + /* auto spans = recorder->spans(); */ + /* CHECK(spans.at(0).span_context().trace_id() == */ + /* spans.at(1).span_context().trace_id()); */ + /* CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(1), */ + /* spans.at(0))); */ + /* } */ - SECTION("Multiple references are supported when starting a span.") { - auto span_a = tracer->StartSpan("a"); - CHECK(span_a); - auto span_b = tracer->StartSpan("b"); - CHECK(span_b); - auto span_c = tracer->StartSpan( - "c", {ChildOf(&span_a->context()), FollowsFrom(&span_b->context())}); - span_a->Finish(); - span_b->Finish(); - span_c->Finish(); - auto spans = recorder->spans(); - CHECK(HasRelationship(SpanReferenceType::ChildOfRef, spans.at(2), - spans.at(0))); - CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(2), - spans.at(1))); - } + /* SECTION("Multiple references are supported when starting a span.") { */ + /* auto span_a = tracer->StartSpan("a"); */ + /* CHECK(span_a); */ + /* auto span_b = tracer->StartSpan("b"); */ + /* CHECK(span_b); */ + /* auto span_c = tracer->StartSpan( */ + /* "c", {ChildOf(&span_a->context()), FollowsFrom(&span_b->context())}); + */ + /* span_a->Finish(); */ + /* span_b->Finish(); */ + /* span_c->Finish(); */ + /* auto spans = recorder->spans(); */ + /* CHECK(HasRelationship(SpanReferenceType::ChildOfRef, spans.at(2), */ + /* spans.at(0))); */ + /* CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(2), */ + /* spans.at(1))); */ + /* } */ - SECTION( - "Baggage from the span references are copied over to a new span " - "context") { - auto span_a = tracer->StartSpan("a"); - CHECK(span_a); - span_a->SetBaggageItem("a", "1"); - auto span_b = tracer->StartSpan("b"); - CHECK(span_b); - span_b->SetBaggageItem("b", "2"); - auto span_c = tracer->StartSpan( - "c", {ChildOf(&span_a->context()), ChildOf(&span_b->context())}); - CHECK(span_c); - CHECK(span_c->BaggageItem("a") == "1"); - CHECK(span_c->BaggageItem("b") == "2"); - } + /* SECTION( */ + /* "Baggage from the span references are copied over to a new span " */ + /* "context") { */ + /* auto span_a = tracer->StartSpan("a"); */ + /* CHECK(span_a); */ + /* span_a->SetBaggageItem("a", "1"); */ + /* auto span_b = tracer->StartSpan("b"); */ + /* CHECK(span_b); */ + /* span_b->SetBaggageItem("b", "2"); */ + /* auto span_c = tracer->StartSpan( */ + /* "c", {ChildOf(&span_a->context()), ChildOf(&span_b->context())}); */ + /* CHECK(span_c); */ + /* CHECK(span_c->BaggageItem("a") == "1"); */ + /* CHECK(span_c->BaggageItem("b") == "2"); */ + /* } */ - SECTION("References to non-LightStep spans and null pointers are ignored.") { - auto noop_tracer = MakeNoopTracer(); - auto noop_span = noop_tracer->StartSpan("noop"); - CHECK(noop_span); - StartSpanOptions options; - options.references.push_back( - std::make_pair(SpanReferenceType::ChildOfRef, &noop_span->context())); - options.references.push_back( - std::make_pair(SpanReferenceType::ChildOfRef, nullptr)); - auto span = tracer->StartSpanWithOptions("a", options); - CHECK(span); - span->Finish(); - CHECK(recorder->top().references_size() == 0); - } + /* SECTION("References to non-LightStep spans and null pointers are ignored.") + * { */ + /* auto noop_tracer = MakeNoopTracer(); */ + /* auto noop_span = noop_tracer->StartSpan("noop"); */ + /* CHECK(noop_span); */ + /* StartSpanOptions options; */ + /* options.references.push_back( */ + /* std::make_pair(SpanReferenceType::ChildOfRef, + * &noop_span->context())); */ + /* options.references.push_back( */ + /* std::make_pair(SpanReferenceType::ChildOfRef, nullptr)); */ + /* auto span = tracer->StartSpanWithOptions("a", options); */ + /* CHECK(span); */ + /* span->Finish(); */ + /* CHECK(recorder->top().references_size() == 0); */ + /* } */ - SECTION("Calling Finish a second time does nothing.") { - auto span = tracer->StartSpan("a"); - CHECK(span); - span->Finish(); - CHECK(recorder->size() == 1); - span->Finish(); - CHECK(recorder->size() == 1); - } + /* SECTION("Calling Finish a second time does nothing.") { */ + /* auto span = tracer->StartSpan("a"); */ + /* CHECK(span); */ + /* span->Finish(); */ + /* CHECK(recorder->size() == 1); */ + /* span->Finish(); */ + /* CHECK(recorder->size() == 1); */ + /* } */ - SECTION("The operation name can be changed after the span is started.") { - auto span = tracer->StartSpan("a"); - CHECK(span); - span->SetOperationName("b"); - span->Finish(); - CHECK(recorder->top().operation_name() == "b"); - } + /* SECTION("The operation name can be changed after the span is started.") { + */ + /* auto span = tracer->StartSpan("a"); */ + /* CHECK(span); */ + /* span->SetOperationName("b"); */ + /* span->Finish(); */ + /* CHECK(recorder->top().operation_name() == "b"); */ + /* } */ - SECTION("Tags can be specified after a span is started.") { - auto span = tracer->StartSpan("a"); - CHECK(span); - span->SetTag("abc", 123); - span->Finish(); - CHECK(HasTag(recorder->top(), "abc", 123)); - } + /* SECTION("Tags can be specified after a span is started.") { */ + /* auto span = tracer->StartSpan("a"); */ + /* CHECK(span); */ + /* span->SetTag("abc", 123); */ + /* span->Finish(); */ + /* CHECK(HasTag(recorder->top(), "abc", 123)); */ + /* } */ - SECTION("Logs are appended and sent to the collector.") { - auto span = tracer->StartSpan("a"); - CHECK(span); - span->Log({{"abc", 123}}); - span->Finish(); - CHECK(recorder->top().logs().size() == 1); - } + /* SECTION("Logs are appended and sent to the collector.") { */ + /* auto span = tracer->StartSpan("a"); */ + /* CHECK(span); */ + /* span->Log({{"abc", 123}}); */ + /* span->Finish(); */ + /* CHECK(recorder->top().logs().size() == 1); */ + /* } */ - SECTION("Logs can be added with FinishSpanOptions.") { - auto span = tracer->StartSpan("a"); - CHECK(span); - opentracing::FinishSpanOptions options; - options.log_records = {{SystemClock::now(), {{"abc", 123}}}}; - span->FinishWithOptions(options); - CHECK(recorder->top().logs().size() == 1); - } + /* SECTION("Logs can be added with FinishSpanOptions.") { */ + /* auto span = tracer->StartSpan("a"); */ + /* CHECK(span); */ + /* opentracing::FinishSpanOptions options; */ + /* options.log_records = {{SystemClock::now(), {{"abc", 123}}}}; */ + /* span->FinishWithOptions(options); */ + /* CHECK(recorder->top().logs().size() == 1); */ + /* } */ } From 858220c1e3fe87f81b2a9103047982367d0bf052 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 11:36:38 -0700 Subject: [PATCH 05/16] Replace LightStepSpanContext with LightStepImmutableSpanContext. --- CMakeLists.txt | 1 + src/lightstep_immutable_span_context.cpp | 37 +++++++++++ src/lightstep_immutable_span_context.h | 56 ++++++++++++++++ src/lightstep_span_context.cpp | 82 ------------------------ src/lightstep_span_context.h | 78 ---------------------- src/lightstep_tracer_impl.cpp | 35 +++++----- src/tracer.cpp | 7 +- test/propagation_test.cpp | 21 +++--- 8 files changed, 127 insertions(+), 190 deletions(-) create mode 100644 src/lightstep_immutable_span_context.cpp create mode 100644 src/lightstep_immutable_span_context.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 182dbad6..1d53175c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -157,6 +157,7 @@ set(LIGHTSTEP_SRCS src/utility.cpp src/manual_recorder.cpp src/auto_recorder.cpp src/lightstep_span_context.cpp + src/lightstep_immutable_span_context.cpp src/lightstep_span.cpp src/lightstep_tracer_impl.cpp src/lightstep_tracer_factory.cpp diff --git a/src/lightstep_immutable_span_context.cpp b/src/lightstep_immutable_span_context.cpp new file mode 100644 index 00000000..d76c0747 --- /dev/null +++ b/src/lightstep_immutable_span_context.cpp @@ -0,0 +1,37 @@ +#include "lightstep_immutable_span_context.h" + +namespace lightstep { +//------------------------------------------------------------------------------ +// constructor +//------------------------------------------------------------------------------ +LightStepImmutableSpanContext::LightStepImmutableSpanContext( + uint64_t trace_id, uint64_t span_id, bool sampled, + std::unordered_map&& baggage) + : trace_id_{trace_id}, span_id_{span_id}, sampled_{sampled} { + for (auto& baggage_item : baggage) { + baggage_.insert(BaggageMap::value_type(std::move(baggage_item.first), + std::move(baggage_item.second))); + } +} + +LightStepImmutableSpanContext::LightStepImmutableSpanContext( + uint64_t trace_id, uint64_t span_id, bool sampled, + BaggageMap&& baggage) noexcept + : trace_id_{trace_id}, + span_id_{span_id}, + sampled_{sampled}, + baggage_{std::move(baggage)} {} + +//------------------------------------------------------------------------------ +// ForeachBaggageItem +//------------------------------------------------------------------------------ +void LightStepImmutableSpanContext::ForeachBaggageItem( + std::function f) + const { + for (const auto& baggage_item : baggage_) { + if (!f(baggage_item.first, baggage_item.second)) { + return; + } + } +} +} // namespace lightstep diff --git a/src/lightstep_immutable_span_context.h b/src/lightstep_immutable_span_context.h new file mode 100644 index 00000000..9204d4f1 --- /dev/null +++ b/src/lightstep_immutable_span_context.h @@ -0,0 +1,56 @@ +#pragma once + +#include "lightstep_span_context.h" + +namespace lightstep { +class LightStepImmutableSpanContext final : public LightStepSpanContextBase { + public: + LightStepImmutableSpanContext( + uint64_t trace_id, uint64_t span_id, bool sampled, + std::unordered_map&& baggage); + + LightStepImmutableSpanContext(uint64_t trace_id, uint64_t span_id, + bool sampled, BaggageMap&& baggage) noexcept; + + uint64_t trace_id() const noexcept override { return trace_id_; } + + uint64_t span_id() const noexcept override { return span_id_; } + + bool sampled() const noexcept override { return sampled_; } + + void ForeachBaggageItem( + std::function f) + const override; + + opentracing::expected Inject( + const PropagationOptions& propagation_options, + std::ostream& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::TextMapWriter& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + opentracing::expected Inject( + const PropagationOptions& propagation_options, + const opentracing::HTTPHeadersWriter& writer) const override { + return this->InjectImpl(propagation_options, writer); + } + + private: + uint64_t trace_id_; + uint64_t span_id_; + bool sampled_; + BaggageMap baggage_; + + template + opentracing::expected InjectImpl( + const PropagationOptions& propagation_options, Carrier& writer) const { + return InjectSpanContext(propagation_options, writer, trace_id_, span_id_, + sampled_, baggage_); + } +}; +} // namespace lightstep diff --git a/src/lightstep_span_context.cpp b/src/lightstep_span_context.cpp index 07ab09db..4f0fda7a 100644 --- a/src/lightstep_span_context.cpp +++ b/src/lightstep_span_context.cpp @@ -1,88 +1,6 @@ #include "lightstep_span_context.h" namespace lightstep { -//------------------------------------------------------------------------------ -// Constructor -//------------------------------------------------------------------------------ -LightStepSpanContext::LightStepSpanContext( - uint64_t trace_id, uint64_t span_id, - std::unordered_map&& baggage) noexcept - : trace_id_{trace_id}, span_id_{span_id}, baggage_{std::move(baggage)} {} - -LightStepSpanContext::LightStepSpanContext( - uint64_t trace_id, uint64_t span_id, bool sampled, - std::unordered_map&& baggage) noexcept - : trace_id_{trace_id}, - span_id_{span_id}, - sampled_{sampled}, - baggage_{std::move(baggage)} {} - -//------------------------------------------------------------------------------ -// operator= -//------------------------------------------------------------------------------ -LightStepSpanContext& LightStepSpanContext::operator=( - LightStepSpanContext&& other) noexcept { - trace_id_ = other.trace_id_; - span_id_ = other.span_id_; - sampled_ = other.sampled_; - baggage_ = std::move(other.baggage_); - return *this; -} - -//------------------------------------------------------------------------------ -// set_baggage_item -//------------------------------------------------------------------------------ -void LightStepSpanContext::set_baggage_item( - opentracing::string_view key, opentracing::string_view value) noexcept try { - std::lock_guard lock_guard{mutex_}; - baggage_.emplace(key, value); -} catch (const std::exception&) { - // Drop baggage item upon error. -} - -//------------------------------------------------------------------------------ -// baggage_item -//------------------------------------------------------------------------------ -std::string LightStepSpanContext::baggage_item( - opentracing::string_view key) const { - std::lock_guard lock_guard{mutex_}; - auto lookup = baggage_.find(key); - if (lookup != baggage_.end()) { - return lookup->second; - } - return {}; -} - -//------------------------------------------------------------------------------ -// ForeachBaggageItem -//------------------------------------------------------------------------------ -void LightStepSpanContext::ForeachBaggageItem( - std::function f) - const { - std::lock_guard lock_guard{mutex_}; - for (const auto& baggage_item : baggage_) { - if (!f(baggage_item.first, baggage_item.second)) { - return; - } - } -} - -//------------------------------------------------------------------------------ -// sampled -//------------------------------------------------------------------------------ -bool LightStepSpanContext::sampled() const noexcept { - std::lock_guard lock_guard{mutex_}; - return sampled_; -} - -//------------------------------------------------------------------------------ -// set_sampled -//------------------------------------------------------------------------------ -void LightStepSpanContext::set_sampled(bool sampled) noexcept { - std::lock_guard lock_guard{mutex_}; - sampled_ = sampled; -} - //------------------------------------------------------------------------------ // operator== //------------------------------------------------------------------------------ diff --git a/src/lightstep_span_context.h b/src/lightstep_span_context.h index 7567bba9..de34018b 100644 --- a/src/lightstep_span_context.h +++ b/src/lightstep_span_context.h @@ -29,84 +29,6 @@ class LightStepSpanContextBase : public opentracing::SpanContext { const opentracing::HTTPHeadersWriter& writer) const = 0; }; -class LightStepSpanContext final : public LightStepSpanContextBase { - public: - LightStepSpanContext() = default; - - LightStepSpanContext( - uint64_t trace_id, uint64_t span_id, - std::unordered_map&& baggage) noexcept; - - LightStepSpanContext( - uint64_t trace_id, uint64_t span_id, bool sampled, - std::unordered_map&& baggage) noexcept; - - LightStepSpanContext(const LightStepSpanContext&) = delete; - LightStepSpanContext(LightStepSpanContext&&) = delete; - - ~LightStepSpanContext() override = default; - - LightStepSpanContext& operator=(LightStepSpanContext&) = delete; - LightStepSpanContext& operator=(LightStepSpanContext&& other) noexcept; - - void set_baggage_item(opentracing::string_view key, - opentracing::string_view value) noexcept; - - std::string baggage_item(opentracing::string_view key) const; - - void ForeachBaggageItem( - std::function f) - const override; - - virtual opentracing::expected Inject( - const PropagationOptions& propagation_options, - std::ostream& writer) const override { - return this->InjectImpl(propagation_options, writer); - } - - virtual opentracing::expected Inject( - const PropagationOptions& propagation_options, - const opentracing::TextMapWriter& writer) const override { - return this->InjectImpl(propagation_options, writer); - } - - virtual opentracing::expected Inject( - const PropagationOptions& propagation_options, - const opentracing::HTTPHeadersWriter& writer) const override { - return this->InjectImpl(propagation_options, writer); - } - - template - opentracing::expected Extract( - const PropagationOptions& propagation_options, Carrier& reader) { - std::lock_guard lock_guard{mutex_}; - return ExtractSpanContext(propagation_options, reader, trace_id_, span_id_, - sampled_, baggage_); - } - - uint64_t trace_id() const noexcept override { return trace_id_; } - uint64_t span_id() const noexcept override { return span_id_; } - - bool sampled() const noexcept override; - void set_sampled(bool sampled) noexcept; - - private: - uint64_t trace_id_ = 0; - uint64_t span_id_ = 0; - - mutable std::mutex mutex_; - bool sampled_ = true; - std::unordered_map baggage_; - - template - opentracing::expected InjectImpl( - const PropagationOptions& propagation_options, Carrier& writer) const { - std::lock_guard lock_guard{mutex_}; - return InjectSpanContext(propagation_options, writer, trace_id_, span_id_, - sampled_, baggage_); - } -}; - bool operator==(const LightStepSpanContextBase& lhs, const LightStepSpanContextBase& rhs); diff --git a/src/lightstep_tracer_impl.cpp b/src/lightstep_tracer_impl.cpp index 5bfecfbe..161d8d37 100644 --- a/src/lightstep_tracer_impl.cpp +++ b/src/lightstep_tracer_impl.cpp @@ -1,6 +1,6 @@ #include "lightstep_tracer_impl.h" +#include "lightstep_immutable_span_context.h" #include "lightstep_span.h" -#include "lightstep_span_context.h" namespace lightstep { @@ -25,24 +25,25 @@ static opentracing::expected InjectImpl( //------------------------------------------------------------------------------ template opentracing::expected> ExtractImpl( - const PropagationOptions& propagation_options, Carrier& reader) { - LightStepSpanContext* lightstep_span_context; - try { - lightstep_span_context = new LightStepSpanContext{}; - } catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - make_error_code(std::errc::not_enough_memory)); - } - std::unique_ptr span_context( - lightstep_span_context); - auto result = lightstep_span_context->Extract(propagation_options, reader); - if (!result) { - return opentracing::make_unexpected(result.error()); + const PropagationOptions& propagation_options, Carrier& reader) try { + uint64_t trace_id, span_id; + bool sampled; + BaggageMap baggage; + auto extract_maybe = ExtractSpanContext(propagation_options, reader, trace_id, + span_id, sampled, baggage); + if (!extract_maybe) { + return opentracing::make_unexpected(extract_maybe.error()); } - if (!*result) { - span_context.reset(); + if (!*extract_maybe) { + return std::unique_ptr{nullptr}; } - return std::move(span_context); + std::unique_ptr result{ + new LightStepImmutableSpanContext{trace_id, span_id, sampled, + std::move(baggage)}}; + return std::move(result); +} catch (const std::bad_alloc&) { + return opentracing::make_unexpected( + make_error_code(std::errc::not_enough_memory)); } //------------------------------------------------------------------------------ diff --git a/src/tracer.cpp b/src/tracer.cpp index 2c3d0ba3..e562cdef 100644 --- a/src/tracer.cpp +++ b/src/tracer.cpp @@ -12,7 +12,7 @@ #include "auto_recorder.h" #include "grpc_transporter.h" #include "lightstep-tracer-common/collector.pb.h" -#include "lightstep_span_context.h" +#include "lightstep_immutable_span_context.h" #include "lightstep_tracer_impl.h" #include "logger.h" #include "manual_recorder.h" @@ -65,7 +65,7 @@ opentracing::expected> LightStepTracer::GetTraceSpanIdsSampled( const opentracing::SpanContext& span_context) const noexcept { auto lightstep_span_context = - dynamic_cast(&span_context); + dynamic_cast(&span_context); if (lightstep_span_context == nullptr) { return opentracing::make_unexpected( opentracing::invalid_span_context_error); @@ -84,7 +84,8 @@ LightStepTracer::MakeSpanContext( uint64_t trace_id, uint64_t span_id, bool sampled, std::unordered_map&& baggage) const noexcept try { std::unique_ptr result{ - new LightStepSpanContext{trace_id, span_id, sampled, std::move(baggage)}}; + new LightStepImmutableSpanContext{trace_id, span_id, sampled, + std::move(baggage)}}; return std::move(result); } catch (const std::bad_alloc&) { return opentracing::make_unexpected( diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp index 95691610..81c690af 100644 --- a/test/propagation_test.cpp +++ b/test/propagation_test.cpp @@ -7,7 +7,7 @@ #include #include #include -#include "../src/lightstep_span_context.h" +#include "../src/lightstep_immutable_span_context.h" #include "../src/lightstep_tracer_impl.h" #include "../src/utility.h" #include "in_memory_recorder.h" @@ -107,8 +107,8 @@ class LightStepBinaryReaderWriter : public LightStepBinaryReader, static bool AreSpanContextsEquivalent( const opentracing::SpanContext& context1, const opentracing::SpanContext& context2) { - return dynamic_cast(context1) == - dynamic_cast(context2); + return dynamic_cast(context1) == + dynamic_cast(context2); } //------------------------------------------------------------------------------ @@ -119,21 +119,22 @@ static std::vector> MakeTestSpanContexts() { std::vector> result; // most basic span context - result.push_back( - std::unique_ptr{new LightStepSpanContext{ - 123, 456, std::unordered_map{}}}); + result.push_back(std::unique_ptr{ + new LightStepImmutableSpanContext{ + 123, 456, true, std::unordered_map{}}}); // span context with single baggage item result.push_back(std::unique_ptr{ - new LightStepSpanContext{123, 456, {{"abc", "123"}}}}); + new LightStepImmutableSpanContext{123, 456, true, {{"abc", "123"}}}}); // span context with multiple baggage items result.push_back(std::unique_ptr{ - new LightStepSpanContext{123, 456, {{"abc", "123"}, {"xyz", "qrz"}}}}); + new LightStepImmutableSpanContext{ + 123, 456, true, {{"abc", "123"}, {"xyz", "qrz"}}}}); // unsampled span context - result.push_back( - std::unique_ptr{new LightStepSpanContext{ + result.push_back(std::unique_ptr{ + new LightStepImmutableSpanContext{ 123, 456, false, std::unordered_map{}}}); return result; From a218b399b18d65e062b23d4b8deac49b16d4caf1 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 11:38:17 -0700 Subject: [PATCH 06/16] s/LightStepSpanContextBase/LightStepSpanContext --- src/lightstep_immutable_span_context.h | 2 +- src/lightstep_span.cpp | 2 +- src/lightstep_span.h | 2 +- src/lightstep_span_context.cpp | 6 +++--- src/lightstep_span_context.h | 10 +++++----- src/lightstep_tracer_impl.cpp | 2 +- src/tracer.cpp | 2 +- test/propagation_test.cpp | 4 ++-- 8 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/lightstep_immutable_span_context.h b/src/lightstep_immutable_span_context.h index 9204d4f1..aca2d775 100644 --- a/src/lightstep_immutable_span_context.h +++ b/src/lightstep_immutable_span_context.h @@ -3,7 +3,7 @@ #include "lightstep_span_context.h" namespace lightstep { -class LightStepImmutableSpanContext final : public LightStepSpanContextBase { +class LightStepImmutableSpanContext final : public LightStepSpanContext { public: LightStepImmutableSpanContext( uint64_t trace_id, uint64_t span_id, bool sampled, diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 382a747e..e30bd4c4 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -66,7 +66,7 @@ static bool SetSpanReference( return false; } auto referenced_context = - dynamic_cast(reference.second); + dynamic_cast(reference.second); if (referenced_context == nullptr) { logger.Warn("Passed in span reference of unexpected type."); return false; diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 79138a15..94cdbaeb 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -11,7 +11,7 @@ namespace lightstep { class LightStepSpan final : public opentracing::Span, - public LightStepSpanContextBase { + public LightStepSpanContext { public: LightStepSpan(std::shared_ptr&& tracer, Logger& logger, Recorder& recorder, diff --git a/src/lightstep_span_context.cpp b/src/lightstep_span_context.cpp index 4f0fda7a..76e6a962 100644 --- a/src/lightstep_span_context.cpp +++ b/src/lightstep_span_context.cpp @@ -4,9 +4,9 @@ namespace lightstep { //------------------------------------------------------------------------------ // operator== //------------------------------------------------------------------------------ -bool operator==(const LightStepSpanContextBase& lhs, - const LightStepSpanContextBase& rhs) { - auto extract_baggage = [](const LightStepSpanContextBase& span_context) { +bool operator==(const LightStepSpanContext& lhs, + const LightStepSpanContext& rhs) { + auto extract_baggage = [](const LightStepSpanContext& span_context) { std::unordered_map baggage; span_context.ForeachBaggageItem( [&](const std::string& key, const std::string& value) { diff --git a/src/lightstep_span_context.h b/src/lightstep_span_context.h index de34018b..d0a44967 100644 --- a/src/lightstep_span_context.h +++ b/src/lightstep_span_context.h @@ -8,7 +8,7 @@ #include "propagation.h" namespace lightstep { -class LightStepSpanContextBase : public opentracing::SpanContext { +class LightStepSpanContext : public opentracing::SpanContext { public: virtual bool sampled() const noexcept = 0; @@ -29,11 +29,11 @@ class LightStepSpanContextBase : public opentracing::SpanContext { const opentracing::HTTPHeadersWriter& writer) const = 0; }; -bool operator==(const LightStepSpanContextBase& lhs, - const LightStepSpanContextBase& rhs); +bool operator==(const LightStepSpanContext& lhs, + const LightStepSpanContext& rhs); -inline bool operator!=(const LightStepSpanContextBase& lhs, - const LightStepSpanContextBase& rhs) { +inline bool operator!=(const LightStepSpanContext& lhs, + const LightStepSpanContext& rhs) { return !(lhs == rhs); } } // namespace lightstep diff --git a/src/lightstep_tracer_impl.cpp b/src/lightstep_tracer_impl.cpp index 161d8d37..76700e2c 100644 --- a/src/lightstep_tracer_impl.cpp +++ b/src/lightstep_tracer_impl.cpp @@ -12,7 +12,7 @@ static opentracing::expected InjectImpl( const PropagationOptions& propagation_options, const opentracing::SpanContext& span_context, Carrier& writer) { auto lightstep_span_context = - dynamic_cast(&span_context); + dynamic_cast(&span_context); if (lightstep_span_context == nullptr) { return opentracing::make_unexpected( opentracing::invalid_span_context_error); diff --git a/src/tracer.cpp b/src/tracer.cpp index e562cdef..32ab68a9 100644 --- a/src/tracer.cpp +++ b/src/tracer.cpp @@ -65,7 +65,7 @@ opentracing::expected> LightStepTracer::GetTraceSpanIdsSampled( const opentracing::SpanContext& span_context) const noexcept { auto lightstep_span_context = - dynamic_cast(&span_context); + dynamic_cast(&span_context); if (lightstep_span_context == nullptr) { return opentracing::make_unexpected( opentracing::invalid_span_context_error); diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp index 81c690af..8f629ede 100644 --- a/test/propagation_test.cpp +++ b/test/propagation_test.cpp @@ -107,8 +107,8 @@ class LightStepBinaryReaderWriter : public LightStepBinaryReader, static bool AreSpanContextsEquivalent( const opentracing::SpanContext& context1, const opentracing::SpanContext& context2) { - return dynamic_cast(context1) == - dynamic_cast(context2); + return dynamic_cast(context1) == + dynamic_cast(context2); } //------------------------------------------------------------------------------ From 258ad1f6d6745232f0b1c452ee86ebc8670d67ee Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 11:48:58 -0700 Subject: [PATCH 07/16] Remove unused propagation code. --- src/propagation.cpp | 304 -------------------------------------------- src/propagation.h | 28 ---- 2 files changed, 332 deletions(-) diff --git a/src/propagation.cpp b/src/propagation.cpp index fc3584f3..e79c799e 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -78,51 +78,6 @@ static opentracing::expected LookupKey( //------------------------------------------------------------------------------ // InjectSpanContextMultiKey //------------------------------------------------------------------------------ -static opentracing::expected InjectSpanContextMultiKey( - const opentracing::TextMapWriter& carrier, uint64_t trace_id, - uint64_t span_id, bool sampled, - const std::unordered_map& baggage) { - std::string trace_id_hex, span_id_hex, baggage_key; - try { - trace_id_hex = Uint64ToHex(trace_id); - span_id_hex = Uint64ToHex(span_id); - baggage_key = PrefixBaggage; - } catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); - } - auto result = carrier.Set(FieldNameTraceID, trace_id_hex); - if (!result) { - return result; - } - result = carrier.Set(FieldNameSpanID, span_id_hex); - if (!result) { - return result; - } - if (sampled) { - result = carrier.Set(FieldNameSampled, "true"); - } else { - result = carrier.Set(FieldNameSampled, "false"); - } - if (!result) { - return result; - } - for (const auto& baggage_item : baggage) { - try { - baggage_key.replace(std::begin(baggage_key) + PrefixBaggage.size(), - std::end(baggage_key), baggage_item.first); - } catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); - } - result = carrier.Set(baggage_key, baggage_item.second); - if (!result) { - return result; - } - } - return {}; -} - static opentracing::expected InjectSpanContextMultiKey( const opentracing::TextMapWriter& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, const BaggageMap& baggage) { @@ -170,33 +125,6 @@ static opentracing::expected InjectSpanContextMultiKey( //------------------------------------------------------------------------------ // InjectSpanContextSingleKey //------------------------------------------------------------------------------ -static opentracing::expected InjectSpanContextSingleKey( - const opentracing::TextMapWriter& carrier, uint64_t trace_id, - uint64_t span_id, bool sampled, - const std::unordered_map& baggage) { - std::ostringstream ostream; - auto result = InjectSpanContext(PropagationOptions{}, ostream, trace_id, - span_id, sampled, baggage); - if (!result) { - return result; - } - std::string context_value; - try { - auto binary_encoding = ostream.str(); - context_value = - Base64::encode(binary_encoding.data(), binary_encoding.size()); - } catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); - } - - result = carrier.Set(PropagationSingleKey, context_value); - if (!result) { - return result; - } - return {}; -} - static opentracing::expected InjectSpanContextSingleKey( const opentracing::TextMapWriter& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, const BaggageMap& baggage) { @@ -226,63 +154,6 @@ static opentracing::expected InjectSpanContextSingleKey( //------------------------------------------------------------------------------ // InjectSpanContext //------------------------------------------------------------------------------ -static opentracing::expected InjectSpanContext( - BinaryCarrier& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, - const std::unordered_map& baggage) noexcept try { - carrier.Clear(); - auto basic = carrier.mutable_basic_ctx(); - basic->set_trace_id(trace_id); - basic->set_span_id(span_id); - basic->set_sampled(sampled); - auto mutable_baggage = basic->mutable_baggage_items(); - for (auto& baggage_item : baggage) { - (*mutable_baggage)[baggage_item.first] = baggage_item.second; - } - return {}; -} catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); -} - -opentracing::expected InjectSpanContext( - const PropagationOptions& /*propagation_options*/, std::ostream& carrier, - uint64_t trace_id, uint64_t span_id, bool sampled, - const std::unordered_map& baggage) { - BinaryCarrier binary_carrier; - auto result = - InjectSpanContext(binary_carrier, trace_id, span_id, sampled, baggage); - if (!result) { - return result; - } - if (!binary_carrier.SerializeToOstream(&carrier)) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::io_error)); - } - - // Flush so that when we call carrier.good(), we'll get an accurate view of - // the error state. - carrier.flush(); - if (!carrier.good()) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::io_error)); - } - - return {}; -} - -opentracing::expected InjectSpanContext( - const PropagationOptions& propagation_options, - const opentracing::TextMapWriter& carrier, uint64_t trace_id, - uint64_t span_id, bool sampled, - const std::unordered_map& baggage) { - if (propagation_options.use_single_key) { - return InjectSpanContextSingleKey(carrier, trace_id, span_id, sampled, - baggage); - } - return InjectSpanContextMultiKey(carrier, trace_id, span_id, sampled, - baggage); -} - static opentracing::expected InjectSpanContext( BinaryCarrier& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, const BaggageMap& baggage) noexcept try { @@ -342,53 +213,6 @@ opentracing::expected InjectSpanContext( //------------------------------------------------------------------------------ // ExtractSpanContextMultiKey //------------------------------------------------------------------------------ -template -static opentracing::expected ExtractSpanContextMultiKey( - const opentracing::TextMapReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage, - KeyCompare key_compare) { - int count = 0; - auto result = carrier.ForeachKey( - [&](opentracing::string_view key, - opentracing::string_view value) -> opentracing::expected { - try { - if (key_compare(key, FieldNameTraceID)) { - trace_id = HexToUint64(value); - count++; - } else if (key_compare(key, FieldNameSpanID)) { - span_id = HexToUint64(value); - count++; - } else if (key_compare(key, FieldNameSampled)) { - sampled = !(value == "false" || value == "0"); - count++; - } else if (key.length() > PrefixBaggage.size() && - key_compare(opentracing::string_view{key.data(), - PrefixBaggage.size()}, - PrefixBaggage)) { - baggage.emplace(std::string{std::begin(key) + PrefixBaggage.size(), - std::end(key)}, - value); - } - return {}; - } catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); - } - }); - if (!result) { - return opentracing::make_unexpected(result.error()); - } - if (count == 0) { - return false; - } - if (count > 0 && count != FieldCount) { - return opentracing::make_unexpected( - opentracing::span_context_corrupted_error); - } - return true; -} - template static opentracing::expected ExtractSpanContextMultiKey( const opentracing::TextMapReader& carrier, uint64_t& trace_id, @@ -438,36 +262,6 @@ static opentracing::expected ExtractSpanContextMultiKey( //------------------------------------------------------------------------------ // ExtractSpanContextSingleKey //------------------------------------------------------------------------------ -template -static opentracing::expected ExtractSpanContextSingleKey( - const opentracing::TextMapReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage, - KeyCompare key_compare) { - auto value_maybe = LookupKey(carrier, PropagationSingleKey, key_compare); - if (!value_maybe) { - if (value_maybe.error() == opentracing::key_not_found_error) { - return false; - } - return opentracing::make_unexpected(value_maybe.error()); - } - auto value = *value_maybe; - std::string base64_decoding; - try { - base64_decoding = Base64::decode(value.data(), value.size()); - } catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); - } - if (base64_decoding.empty()) { - return opentracing::make_unexpected( - opentracing::span_context_corrupted_error); - } - in_memory_stream istream{base64_decoding.data(), base64_decoding.size()}; - return ExtractSpanContext(PropagationOptions{}, istream, trace_id, span_id, - sampled, baggage); -} - template static opentracing::expected ExtractSpanContextSingleKey( const opentracing::TextMapReader& carrier, uint64_t& trace_id, @@ -500,104 +294,6 @@ static opentracing::expected ExtractSpanContextSingleKey( //------------------------------------------------------------------------------ // ExtractSpanContext //------------------------------------------------------------------------------ -static opentracing::expected ExtractSpanContext( - const BinaryCarrier& carrier, uint64_t& trace_id, uint64_t& span_id, - bool& sampled, - std::unordered_map& baggage) noexcept try { - auto& basic = carrier.basic_ctx(); - trace_id = basic.trace_id(); - span_id = basic.span_id(); - sampled = basic.sampled(); - for (const auto& entry : basic.baggage_items()) { - baggage.emplace(entry.first, entry.second); - } - return true; -} catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); -} - -opentracing::expected ExtractSpanContext( - const PropagationOptions& /*propagation_options*/, std::istream& carrier, - uint64_t& trace_id, uint64_t& span_id, bool& sampled, - std::unordered_map& baggage) try { - // istream::peek returns EOF if it's in an error state, so check for an error - // state first before checking for an empty stream. - if (!carrier.good()) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::io_error)); - } - - // Check for the case when no span is encoded. - if (carrier.peek() == EOF) { - return false; - } - - BinaryCarrier binary_carrier; - if (!binary_carrier.ParseFromIstream(&carrier)) { - return opentracing::make_unexpected( - opentracing::span_context_corrupted_error); - } - return ExtractSpanContext(binary_carrier, trace_id, span_id, sampled, - baggage); -} catch (const std::bad_alloc&) { - return opentracing::make_unexpected( - std::make_error_code(std::errc::not_enough_memory)); -} - -template -static opentracing::expected ExtractSpanContext( - const PropagationOptions& propagation_options, - const opentracing::TextMapReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage, - KeyCompare key_compare) { - if (propagation_options.use_single_key) { - auto span_context_maybe = ExtractSpanContextSingleKey( - carrier, trace_id, span_id, sampled, baggage, key_compare); - - // If no span context was found, fall back to multikey extraction so as to - // support interoperability with other tracers. - if (!span_context_maybe || *span_context_maybe) { - return span_context_maybe; - } - } - - return ExtractSpanContextMultiKey(carrier, trace_id, span_id, sampled, - baggage, key_compare); -} - -opentracing::expected ExtractSpanContext( - const PropagationOptions& propagation_options, - const opentracing::TextMapReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage) { - return ExtractSpanContext(propagation_options, carrier, trace_id, span_id, - sampled, baggage, - std::equal_to()); -} - -// HTTP header field names are case insensitive, so we need to ignore case when -// comparing against the OpenTracing field names. -// -// See https://stackoverflow.com/a/5259004/4447365 -opentracing::expected ExtractSpanContext( - const PropagationOptions& propagation_options, - const opentracing::HTTPHeadersReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage) { - auto iequals = [](opentracing::string_view lhs, - opentracing::string_view rhs) { - return lhs.length() == rhs.length() && - std::equal(std::begin(lhs), std::end(lhs), std::begin(rhs), - [](char a, char b) { - return std::tolower(a) == std::tolower(b); - }); - }; - return ExtractSpanContext(propagation_options, carrier, trace_id, span_id, - sampled, baggage, iequals); -} - static opentracing::expected ExtractSpanContext( const BinaryCarrier& carrier, uint64_t& trace_id, uint64_t& span_id, bool& sampled, BaggageMap& baggage) noexcept try { diff --git a/src/propagation.h b/src/propagation.h index 96ac38dc..514b405c 100644 --- a/src/propagation.h +++ b/src/propagation.h @@ -21,34 +21,6 @@ opentracing::expected InjectSpanContext( const opentracing::TextMapWriter& carrier, uint64_t trace_id, uint64_t span_id, bool sampled, const BaggageMap& baggage); -opentracing::expected InjectSpanContext( - const PropagationOptions& propagation_options, std::ostream& carrier, - uint64_t trace_id, uint64_t span_id, bool sampled, - const std::unordered_map& baggage); - -opentracing::expected InjectSpanContext( - const PropagationOptions& propagation_options, - const opentracing::TextMapWriter& carrier, uint64_t trace_id, - uint64_t span_id, bool sampled, - const std::unordered_map& baggage); - -opentracing::expected ExtractSpanContext( - const PropagationOptions& propagation_options, std::istream& carrier, - uint64_t& trace_id, uint64_t& span_id, bool& sampled, - std::unordered_map& baggage); - -opentracing::expected ExtractSpanContext( - const PropagationOptions& propagation_options, - const opentracing::TextMapReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage); - -opentracing::expected ExtractSpanContext( - const PropagationOptions& propagation_options, - const opentracing::HTTPHeadersReader& carrier, uint64_t& trace_id, - uint64_t& span_id, bool& sampled, - std::unordered_map& baggage); - opentracing::expected ExtractSpanContext( const PropagationOptions& propagation_options, std::istream& carrier, uint64_t& trace_id, uint64_t& span_id, bool& sampled, BaggageMap& baggage); From ef9a257d4c15a0e0f6a8e5ce7ce91a1e90174d0f Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 12:06:19 -0700 Subject: [PATCH 08/16] Set tags directly on protobuf object. --- src/lightstep_span.cpp | 27 +++++++++------------------ src/lightstep_span.h | 2 -- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index e30bd4c4..7476f98f 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -124,15 +124,16 @@ LightStepSpan::LightStepSpan( } // Set tags. + auto& tags = *span_.mutable_tags(); + tags.Reserve(static_cast(options.tags.size())); for (auto& tag : options.tags) { - tags_[tag.first] = tag.second; - } + *tags.Add() = ToKeyValue(tag.first, tag.second); - // If sampling_priority is set, it overrides whatever sampling decision was - // derived from the referenced spans. - auto sampling_priority_tag = tags_.find(opentracing::ext::sampling_priority); - if (sampling_priority_tag != tags_.end()) { - sampled_ = is_sampled(sampling_priority_tag->second); + // If sampling_priority is set, it overrides whatever sampling decision was + // derived from the referenced spans. + if (tag.first == opentracing::ext::sampling_priority) { + sampled_ = is_sampled(tag.second); + } } // Set opentracing::SpanContext. @@ -187,16 +188,6 @@ void LightStepSpan::FinishWithOptions( // Set tags, logs, and operation name. { - auto tags = span_.mutable_tags(); - tags->Reserve(static_cast(tags_.size())); - for (const auto& tag : tags_) { - try { - *tags->Add() = ToKeyValue(tag.first, tag.second); - } catch (const std::exception& e) { - logger_.Error(R"(Dropping tag for key ")", tag.first, - R"(": )", e.what()); - } - } auto logs = span_.mutable_logs(); logs->Reserve(static_cast(logs_.size()) + static_cast(options.log_records.size())); @@ -245,7 +236,7 @@ void LightStepSpan::SetOperationName( void LightStepSpan::SetTag(opentracing::string_view key, const opentracing::Value& value) noexcept try { std::lock_guard lock_guard{mutex_}; - tags_[key] = value; + *span_.mutable_tags()->Add() = ToKeyValue(key, value); if (key == opentracing::ext::sampling_priority) { sampled_ = is_sampled(value); diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 94cdbaeb..75e540f6 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -92,12 +92,10 @@ class LightStepSpan final : public opentracing::Span, Recorder& recorder_; std::vector references_; std::chrono::steady_clock::time_point start_steady_; - /* LightStepSpanContext span_context_; */ std::atomic is_finished_{false}; // Mutex protects tags_, logs_, and operation_name_. - std::unordered_map tags_; std::vector logs_; template From 37b7056e64b4bfa8186ce935b35995cceef9faf9 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 12:16:05 -0700 Subject: [PATCH 09/16] Add reference benchmark. --- benchmark/span_operations_benchmark.cpp | 14 ++++++++++++++ src/lightstep_span.cpp | 2 -- src/lightstep_span.h | 4 ++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/benchmark/span_operations_benchmark.cpp b/benchmark/span_operations_benchmark.cpp index 5f536418..ca539698 100644 --- a/benchmark/span_operations_benchmark.cpp +++ b/benchmark/span_operations_benchmark.cpp @@ -29,6 +29,20 @@ static void BM_SpanCreation(benchmark::State& state) { } BENCHMARK(BM_SpanCreation); +static void BM_SpanCreationWithParent(benchmark::State& state) { + auto tracer = MakeTracer(); + assert(tracer != nullptr); + auto parent_span = tracer->StartSpan("parent"); + parent_span->Finish(); + opentracing::StartSpanOptions options; + options.references.emplace_back(opentracing::SpanReferenceType::ChildOfRef, + &parent_span->context()); + for (auto _ : state) { + auto span = tracer->StartSpanWithOptions("abc123", options); + } +} +BENCHMARK(BM_SpanCreationWithParent); + static void BM_SpanSetTag1(benchmark::State& state) { auto tracer = MakeTracer(); assert(tracer != nullptr); diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 7476f98f..b0fafcfd 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -1,6 +1,5 @@ #include "lightstep_span.h" #include -#include #include "utility.h" using opentracing::SystemTime; @@ -71,7 +70,6 @@ static bool SetSpanReference( logger.Warn("Passed in span reference of unexpected type."); return false; } - std::cout << "trace_id = " << referenced_context->trace_id() << std::endl; collector_reference.mutable_span_context()->set_trace_id( referenced_context->trace_id()); collector_reference.mutable_span_context()->set_span_id( diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 75e540f6..2d86f755 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -85,13 +85,13 @@ class LightStepSpan final : public opentracing::Span, private: // Fields set in StartSpan() are not protected by a mutex. collector::Span span_; - mutable std::mutex mutex_; + std::chrono::steady_clock::time_point start_steady_; bool sampled_; + mutable std::mutex mutex_; std::shared_ptr tracer_; Logger& logger_; Recorder& recorder_; std::vector references_; - std::chrono::steady_clock::time_point start_steady_; std::atomic is_finished_{false}; From 1119b6a2162557b45f9bb261b82ed9c58d4dfda2 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 12:27:08 -0700 Subject: [PATCH 10/16] Set reference directly on protobuf object. --- src/lightstep_immutable_span_context.cpp | 6 +++--- src/lightstep_immutable_span_context.h | 2 +- src/lightstep_span.cpp | 21 +++++++-------------- src/lightstep_span.h | 1 - 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/src/lightstep_immutable_span_context.cpp b/src/lightstep_immutable_span_context.cpp index d76c0747..c9d54ac2 100644 --- a/src/lightstep_immutable_span_context.cpp +++ b/src/lightstep_immutable_span_context.cpp @@ -6,11 +6,11 @@ namespace lightstep { //------------------------------------------------------------------------------ LightStepImmutableSpanContext::LightStepImmutableSpanContext( uint64_t trace_id, uint64_t span_id, bool sampled, - std::unordered_map&& baggage) + const std::unordered_map& baggage) : trace_id_{trace_id}, span_id_{span_id}, sampled_{sampled} { for (auto& baggage_item : baggage) { - baggage_.insert(BaggageMap::value_type(std::move(baggage_item.first), - std::move(baggage_item.second))); + baggage_.insert( + BaggageMap::value_type(baggage_item.first, baggage_item.second)); } } diff --git a/src/lightstep_immutable_span_context.h b/src/lightstep_immutable_span_context.h index aca2d775..41768785 100644 --- a/src/lightstep_immutable_span_context.h +++ b/src/lightstep_immutable_span_context.h @@ -7,7 +7,7 @@ class LightStepImmutableSpanContext final : public LightStepSpanContext { public: LightStepImmutableSpanContext( uint64_t trace_id, uint64_t span_id, bool sampled, - std::unordered_map&& baggage); + const std::unordered_map& baggage); LightStepImmutableSpanContext(uint64_t trace_id, uint64_t span_id, bool sampled, BaggageMap&& baggage) noexcept; diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index b0fafcfd..d0003f8e 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -104,20 +104,21 @@ LightStepSpan::LightStepSpan( *span_.mutable_start_timestamp() = ToTimestamp(start_timestamp); // Set any span references. - references_.reserve(options.references.size()); - collector::Reference collector_reference; sampled_ = false; + collector::Reference collector_reference; + auto& references = *span_.mutable_references(); + references.Reserve(static_cast(options.references.size())); for (auto& reference : options.references) { if (!SetSpanReference(logger_, reference, baggage, collector_reference, sampled_)) { continue; } - references_.push_back(collector_reference); + *references.Add() = collector_reference; } // If there are any span references, sampled should be true if any of the // references are sampled; with no refences, we set sampled to true. - if (references_.empty()) { + if (references.empty()) { sampled_ = true; } @@ -135,9 +136,8 @@ LightStepSpan::LightStepSpan( } // Set opentracing::SpanContext. - auto trace_id = references_.empty() - ? GenerateId() - : references_[0].span_context().trace_id(); + auto trace_id = references.empty() ? GenerateId() + : references[0].span_context().trace_id(); auto span_id = GenerateId(); span_context.set_trace_id(trace_id); span_context.set_span_id(span_id); @@ -177,13 +177,6 @@ void LightStepSpan::FinishWithOptions( span_.set_duration_micros( std::chrono::duration_cast(duration).count()); - // Set references. - auto references = span_.mutable_references(); - references->Reserve(static_cast(references_.size())); - for (const auto& reference : references_) { - *references->Add() = reference; - } - // Set tags, logs, and operation name. { auto logs = span_.mutable_logs(); diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 2d86f755..e3adddf4 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -91,7 +91,6 @@ class LightStepSpan final : public opentracing::Span, std::shared_ptr tracer_; Logger& logger_; Recorder& recorder_; - std::vector references_; std::atomic is_finished_{false}; From 003eab55091ca96c1b9a30e2dd7345f3f0652e99 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 13:46:35 -0700 Subject: [PATCH 11/16] Set logs on protobuf object. --- src/lightstep_span.cpp | 60 ++++++++++++++++++++---------------------- src/lightstep_span.h | 3 --- 2 files changed, 29 insertions(+), 34 deletions(-) diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index d0003f8e..0df08e8e 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -8,6 +8,23 @@ using opentracing::SteadyClock; using opentracing::SteadyTime; namespace lightstep { +//------------------------------------------------------------------------------ +// ToLog +//------------------------------------------------------------------------------ +template +static collector::Log ToLog(std::chrono::system_clock::time_point timestamp, + Iterator field_first, Iterator field_last) { + collector::Log result; + *result.mutable_timestamp() = ToTimestamp(timestamp); + auto& key_values = *result.mutable_fields(); + key_values.Reserve(static_cast(std::distance(field_first, field_last))); + for (Iterator field_iter = field_first; field_iter != field_last; + ++field_iter) { + *key_values.Add() = ToKeyValue(field_iter->first, field_iter->second); + } + return result; +} + //------------------------------------------------------------------------------ // is_sampled //------------------------------------------------------------------------------ @@ -177,30 +194,15 @@ void LightStepSpan::FinishWithOptions( span_.set_duration_micros( std::chrono::duration_cast(duration).count()); - // Set tags, logs, and operation name. - { - auto logs = span_.mutable_logs(); - logs->Reserve(static_cast(logs_.size()) + - static_cast(options.log_records.size())); - for (auto& log : logs_) { - try { - *logs->Add() = std::move(log); - } catch (const std::exception& e) { - logger_.Error("Dropping log record: ", e.what()); - } - } - for (auto& log_record : options.log_records) { - try { - collector::Log log; - *log.mutable_timestamp() = ToTimestamp(log_record.timestamp); - auto key_values = log.mutable_fields(); - for (auto& field : log_record.fields) { - *key_values->Add() = ToKeyValue(field.first, field.second); - } - *logs->Add() = std::move(log); - } catch (const std::exception& e) { - logger_.Error("Dropping log record: ", e.what()); - } + // Set logs + auto& logs = *span_.mutable_logs(); + logs.Reserve(logs.size() + static_cast(options.log_records.size())); + for (auto& log_record : options.log_records) { + try { + *logs.Add() = ToLog(log_record.timestamp, std::begin(log_record.fields), + std::end(log_record.fields)); + } catch (const std::exception& e) { + logger_.Error("Dropping log record: ", e.what()); } } @@ -270,13 +272,9 @@ void LightStepSpan::Log(std::initializer_list< std::pair> fields) noexcept try { auto timestamp = SystemClock::now(); - collector::Log log; - *log.mutable_timestamp() = ToTimestamp(timestamp); - auto key_values = log.mutable_fields(); - for (const auto& field : fields) { - *key_values->Add() = ToKeyValue(field.first, field.second); - } - logs_.emplace_back(std::move(log)); + std::lock_guard lock_guard{mutex_}; + *span_.mutable_logs()->Add() = + ToLog(timestamp, std::begin(fields), std::end(fields)); } catch (const std::exception& e) { logger_.Error("Log failed: ", e.what()); } diff --git a/src/lightstep_span.h b/src/lightstep_span.h index e3adddf4..7707d31e 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -94,9 +94,6 @@ class LightStepSpan final : public opentracing::Span, std::atomic is_finished_{false}; - // Mutex protects tags_, logs_, and operation_name_. - std::vector logs_; - template opentracing::expected InjectImpl( const PropagationOptions& propagation_options, Carrier& writer) const { From 532478ef88f78533b1a9dcb6893326495a023dd4 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 14:37:34 -0700 Subject: [PATCH 12/16] Fix clang-tidy warning. --- src/tracer.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tracer.cpp b/src/tracer.cpp index 32ab68a9..6122fef5 100644 --- a/src/tracer.cpp +++ b/src/tracer.cpp @@ -84,8 +84,7 @@ LightStepTracer::MakeSpanContext( uint64_t trace_id, uint64_t span_id, bool sampled, std::unordered_map&& baggage) const noexcept try { std::unique_ptr result{ - new LightStepImmutableSpanContext{trace_id, span_id, sampled, - std::move(baggage)}}; + new LightStepImmutableSpanContext{trace_id, span_id, sampled, baggage}}; return std::move(result); } catch (const std::bad_alloc&) { return opentracing::make_unexpected( From f6d54f7105801c3b8f71033defbd31e39c7f2409 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 14:43:50 -0700 Subject: [PATCH 13/16] Change include order. --- src/lightstep_span.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lightstep_span.h b/src/lightstep_span.h index 7707d31e..78f1c1dc 100644 --- a/src/lightstep_span.h +++ b/src/lightstep_span.h @@ -1,14 +1,16 @@ #pragma once -#include -#include -#include -#include #include "lightstep-tracer-common/collector.pb.h" #include "lightstep_span_context.h" #include "logger.h" #include "recorder.h" +#include + +#include +#include +#include + namespace lightstep { class LightStepSpan final : public opentracing::Span, public LightStepSpanContext { From 22cf1a1e01eb0ab4226ab17635aee4e0f4916a9e Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 14:47:03 -0700 Subject: [PATCH 14/16] Clear reference before setting. --- src/lightstep_span.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lightstep_span.cpp b/src/lightstep_span.cpp index 0df08e8e..b6fac63e 100644 --- a/src/lightstep_span.cpp +++ b/src/lightstep_span.cpp @@ -69,6 +69,7 @@ static bool SetSpanReference( const opentracing::SpanContext*>& reference, BaggageMap& baggage, collector::Reference& collector_reference, bool& sampled) { + collector_reference.Clear(); switch (reference.first) { case opentracing::SpanReferenceType::ChildOfRef: collector_reference.set_relationship(collector::Reference::CHILD_OF); From 83a55010f61c40150a9c0caa4574460a5dda2ad6 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 14:54:36 -0700 Subject: [PATCH 15/16] Use final specifier. --- src/auto_recorder.h | 2 +- src/lightstep_tracer_factory.h | 2 +- src/lightstep_tracer_impl.h | 2 +- src/manual_recorder.h | 3 ++- test/in_memory_recorder.h | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/auto_recorder.h b/src/auto_recorder.h index 9472133c..372ed17e 100644 --- a/src/auto_recorder.h +++ b/src/auto_recorder.h @@ -16,7 +16,7 @@ namespace lightstep { // AutoRecorder buffers spans finished by a tracer and sends them over to // the provided SyncTransporter. It uses an internal thread to regularly send // the reports according to the rate specified by LightStepTracerOptions. -class AutoRecorder : public Recorder { +class AutoRecorder final : public Recorder { public: AutoRecorder(Logger& logger, LightStepTracerOptions&& options, std::unique_ptr&& transporter); diff --git a/src/lightstep_tracer_factory.h b/src/lightstep_tracer_factory.h index 90b00948..7e7df050 100644 --- a/src/lightstep_tracer_factory.h +++ b/src/lightstep_tracer_factory.h @@ -3,7 +3,7 @@ #include namespace lightstep { -class LightStepTracerFactory : public opentracing::TracerFactory { +class LightStepTracerFactory final : public opentracing::TracerFactory { public: opentracing::expected> MakeTracer( const char* configuration, std::string& error_message) const diff --git a/src/lightstep_tracer_impl.h b/src/lightstep_tracer_impl.h index d9f44d1e..4dfa4d83 100644 --- a/src/lightstep_tracer_impl.h +++ b/src/lightstep_tracer_impl.h @@ -6,7 +6,7 @@ #include "recorder.h" namespace lightstep { -class LightStepTracerImpl +class LightStepTracerImpl final : public LightStepTracer, public std::enable_shared_from_this { public: diff --git a/src/manual_recorder.h b/src/manual_recorder.h index 645b2a64..9daf4ede 100644 --- a/src/manual_recorder.h +++ b/src/manual_recorder.h @@ -8,7 +8,8 @@ namespace lightstep { // ManualRecorder buffers spans finished by a tracer and sends them over to // the provided AsyncTransporter when FlushWithTimeout is called. -class ManualRecorder : public Recorder, private AsyncTransporter::Callback { +class ManualRecorder final : public Recorder, + private AsyncTransporter::Callback { public: ManualRecorder(Logger& logger, LightStepTracerOptions options, std::unique_ptr&& transporter); diff --git a/test/in_memory_recorder.h b/test/in_memory_recorder.h index 519a99a7..5fd4f778 100644 --- a/test/in_memory_recorder.h +++ b/test/in_memory_recorder.h @@ -7,7 +7,7 @@ namespace lightstep { // InMemoryRecorder is used for testing only. -class InMemoryRecorder : public Recorder { +class InMemoryRecorder final : public Recorder { public: void RecordSpan(const collector::Span& span) noexcept override { std::lock_guard lock_guard{mutex_}; From fbc87e17e8436e02d009419ac5ae1b9b4562a94d Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Aug 2018 15:06:59 -0700 Subject: [PATCH 16/16] Uncomment tests. --- test/tracer_test.cpp | 293 +++++++++++++++++++++---------------------- 1 file changed, 142 insertions(+), 151 deletions(-) diff --git a/test/tracer_test.cpp b/test/tracer_test.cpp index a8e0fef8..13143797 100644 --- a/test/tracer_test.cpp +++ b/test/tracer_test.cpp @@ -16,58 +16,55 @@ TEST_CASE("tracer") { auto tracer = std::shared_ptr{new LightStepTracerImpl{ PropagationOptions{}, std::unique_ptr{recorder}}}; - /* SECTION("StartSpan applies the provided tags.") { */ - /* { */ - /* auto span = */ - /* tracer->StartSpan("a", {SetTag("abc", 123), SetTag("xyz", true)}); - */ - /* CHECK(span); */ - /* span->Finish(); */ - /* } */ - /* auto span = recorder->top(); */ - /* CHECK(span.operation_name() == "a"); */ - /* CHECK(HasTag(span, "abc", 123)); */ - /* CHECK(HasTag(span, "xyz", true)); */ - /* } */ + SECTION("StartSpan applies the provided tags.") { + { + auto span = + tracer->StartSpan("a", {SetTag("abc", 123), SetTag("xyz", true)}); + CHECK(span); + span->Finish(); + } + auto span = recorder->top(); + CHECK(span.operation_name() == "a"); + CHECK(HasTag(span, "abc", 123)); + CHECK(HasTag(span, "xyz", true)); + } - /* SECTION( */ - /* "Sampling of a span can be turned off by setting the sampling_priority - * " */ - /* "tag to 0.") { */ - /* { */ - /* auto span = tracer->StartSpan( */ - /* "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); */ - /* CHECK(span); */ - /* } */ - /* { */ - /* auto span = tracer->StartSpan("a"); */ - /* span->SetTag(opentracing::ext::sampling_priority, 0); */ - /* CHECK(span); */ - /* } */ - /* CHECK(recorder->size() == 0); */ - /* } */ + SECTION( + "Sampling of a span can be turned off by setting the sampling_priority " + "tag to 0.") { + { + auto span = tracer->StartSpan( + "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); + CHECK(span); + } + { + auto span = tracer->StartSpan("a"); + span->SetTag(opentracing::ext::sampling_priority, 0); + CHECK(span); + } + CHECK(recorder->size() == 0); + } - /* SECTION( */ - /* "If a span's parent isn't sampled, then it also isn't sampled unless " - */ - /* "sampling priority is overwritten.") { */ - /* auto parent_span = tracer->StartSpan( */ - /* "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); */ - /* CHECK(parent_span); */ - /* { */ - /* auto child_span = tracer->StartSpan( */ - /* "b", {opentracing::ChildOf(&parent_span->context())}); */ - /* CHECK(child_span); */ - /* } */ - /* CHECK(recorder->size() == 0); */ - /* { */ - /* auto child_span = tracer->StartSpan( */ - /* "b", {opentracing::ChildOf(&parent_span->context())}); */ - /* CHECK(child_span); */ - /* child_span->SetTag(opentracing::ext::sampling_priority, 1); */ - /* } */ - /* CHECK(recorder->size() == 1); */ - /* } */ + SECTION( + "If a span's parent isn't sampled, then it also isn't sampled unless " + "sampling priority is overwritten.") { + auto parent_span = tracer->StartSpan( + "a", {SetTag(opentracing::ext::sampling_priority, 0u)}); + CHECK(parent_span); + { + auto child_span = tracer->StartSpan( + "b", {opentracing::ChildOf(&parent_span->context())}); + CHECK(child_span); + } + CHECK(recorder->size() == 0); + { + auto child_span = tracer->StartSpan( + "b", {opentracing::ChildOf(&parent_span->context())}); + CHECK(child_span); + child_span->SetTag(opentracing::ext::sampling_priority, 1); + } + CHECK(recorder->size() == 1); + } SECTION("You can set a single child-of reference when starting a span.") { auto span_a = tracer->StartSpan("a"); @@ -83,113 +80,107 @@ TEST_CASE("tracer") { spans.at(0))); } - /* SECTION("You can set a single follows-from reference when starting a - * span.") { */ - /* auto span_a = tracer->StartSpan("a"); */ - /* CHECK(span_a); */ - /* span_a->Finish(); */ - /* auto span_b = tracer->StartSpan("b", {FollowsFrom(&span_a->context())}); - */ - /* CHECK(span_b); */ - /* span_b->Finish(); */ - /* auto spans = recorder->spans(); */ - /* CHECK(spans.at(0).span_context().trace_id() == */ - /* spans.at(1).span_context().trace_id()); */ - /* CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(1), */ - /* spans.at(0))); */ - /* } */ + SECTION("You can set a single follows-from reference when starting a span.") { + auto span_a = tracer->StartSpan("a"); + CHECK(span_a); + span_a->Finish(); + auto span_b = tracer->StartSpan("b", {FollowsFrom(&span_a->context())}); + CHECK(span_b); + span_b->Finish(); + auto spans = recorder->spans(); + CHECK(spans.at(0).span_context().trace_id() == + spans.at(1).span_context().trace_id()); + CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(1), + spans.at(0))); + } - /* SECTION("Multiple references are supported when starting a span.") { */ - /* auto span_a = tracer->StartSpan("a"); */ - /* CHECK(span_a); */ - /* auto span_b = tracer->StartSpan("b"); */ - /* CHECK(span_b); */ - /* auto span_c = tracer->StartSpan( */ - /* "c", {ChildOf(&span_a->context()), FollowsFrom(&span_b->context())}); - */ - /* span_a->Finish(); */ - /* span_b->Finish(); */ - /* span_c->Finish(); */ - /* auto spans = recorder->spans(); */ - /* CHECK(HasRelationship(SpanReferenceType::ChildOfRef, spans.at(2), */ - /* spans.at(0))); */ - /* CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(2), */ - /* spans.at(1))); */ - /* } */ + SECTION("Multiple references are supported when starting a span.") { + auto span_a = tracer->StartSpan("a"); + CHECK(span_a); + auto span_b = tracer->StartSpan("b"); + CHECK(span_b); + auto span_c = tracer->StartSpan( + "c", {ChildOf(&span_a->context()), FollowsFrom(&span_b->context())}); + span_a->Finish(); + span_b->Finish(); + span_c->Finish(); + auto spans = recorder->spans(); + CHECK(HasRelationship(SpanReferenceType::ChildOfRef, spans.at(2), + spans.at(0))); + CHECK(HasRelationship(SpanReferenceType::FollowsFromRef, spans.at(2), + spans.at(1))); + } - /* SECTION( */ - /* "Baggage from the span references are copied over to a new span " */ - /* "context") { */ - /* auto span_a = tracer->StartSpan("a"); */ - /* CHECK(span_a); */ - /* span_a->SetBaggageItem("a", "1"); */ - /* auto span_b = tracer->StartSpan("b"); */ - /* CHECK(span_b); */ - /* span_b->SetBaggageItem("b", "2"); */ - /* auto span_c = tracer->StartSpan( */ - /* "c", {ChildOf(&span_a->context()), ChildOf(&span_b->context())}); */ - /* CHECK(span_c); */ - /* CHECK(span_c->BaggageItem("a") == "1"); */ - /* CHECK(span_c->BaggageItem("b") == "2"); */ - /* } */ + SECTION( + "Baggage from the span references are copied over to a new span " + "context") { + auto span_a = tracer->StartSpan("a"); + CHECK(span_a); + span_a->SetBaggageItem("a", "1"); + auto span_b = tracer->StartSpan("b"); + CHECK(span_b); + span_b->SetBaggageItem("b", "2"); + auto span_c = tracer->StartSpan( + "c", {ChildOf(&span_a->context()), ChildOf(&span_b->context())}); + CHECK(span_c); + CHECK(span_c->BaggageItem("a") == "1"); + CHECK(span_c->BaggageItem("b") == "2"); + } - /* SECTION("References to non-LightStep spans and null pointers are ignored.") - * { */ - /* auto noop_tracer = MakeNoopTracer(); */ - /* auto noop_span = noop_tracer->StartSpan("noop"); */ - /* CHECK(noop_span); */ - /* StartSpanOptions options; */ - /* options.references.push_back( */ - /* std::make_pair(SpanReferenceType::ChildOfRef, - * &noop_span->context())); */ - /* options.references.push_back( */ - /* std::make_pair(SpanReferenceType::ChildOfRef, nullptr)); */ - /* auto span = tracer->StartSpanWithOptions("a", options); */ - /* CHECK(span); */ - /* span->Finish(); */ - /* CHECK(recorder->top().references_size() == 0); */ - /* } */ + SECTION("References to non-LightStep spans and null pointers are ignored.") { + auto noop_tracer = MakeNoopTracer(); + auto noop_span = noop_tracer->StartSpan("noop"); + CHECK(noop_span); + StartSpanOptions options; + options.references.push_back( + std::make_pair(SpanReferenceType::ChildOfRef, &noop_span->context())); + options.references.push_back( + std::make_pair(SpanReferenceType::ChildOfRef, nullptr)); + auto span = tracer->StartSpanWithOptions("a", options); + CHECK(span); + span->Finish(); + CHECK(recorder->top().references_size() == 0); + } - /* SECTION("Calling Finish a second time does nothing.") { */ - /* auto span = tracer->StartSpan("a"); */ - /* CHECK(span); */ - /* span->Finish(); */ - /* CHECK(recorder->size() == 1); */ - /* span->Finish(); */ - /* CHECK(recorder->size() == 1); */ - /* } */ + SECTION("Calling Finish a second time does nothing.") { + auto span = tracer->StartSpan("a"); + CHECK(span); + span->Finish(); + CHECK(recorder->size() == 1); + span->Finish(); + CHECK(recorder->size() == 1); + } - /* SECTION("The operation name can be changed after the span is started.") { - */ - /* auto span = tracer->StartSpan("a"); */ - /* CHECK(span); */ - /* span->SetOperationName("b"); */ - /* span->Finish(); */ - /* CHECK(recorder->top().operation_name() == "b"); */ - /* } */ + SECTION("The operation name can be changed after the span is started.") { + auto span = tracer->StartSpan("a"); + CHECK(span); + span->SetOperationName("b"); + span->Finish(); + CHECK(recorder->top().operation_name() == "b"); + } - /* SECTION("Tags can be specified after a span is started.") { */ - /* auto span = tracer->StartSpan("a"); */ - /* CHECK(span); */ - /* span->SetTag("abc", 123); */ - /* span->Finish(); */ - /* CHECK(HasTag(recorder->top(), "abc", 123)); */ - /* } */ + SECTION("Tags can be specified after a span is started.") { + auto span = tracer->StartSpan("a"); + CHECK(span); + span->SetTag("abc", 123); + span->Finish(); + CHECK(HasTag(recorder->top(), "abc", 123)); + } - /* SECTION("Logs are appended and sent to the collector.") { */ - /* auto span = tracer->StartSpan("a"); */ - /* CHECK(span); */ - /* span->Log({{"abc", 123}}); */ - /* span->Finish(); */ - /* CHECK(recorder->top().logs().size() == 1); */ - /* } */ + SECTION("Logs are appended and sent to the collector.") { + auto span = tracer->StartSpan("a"); + CHECK(span); + span->Log({{"abc", 123}}); + span->Finish(); + CHECK(recorder->top().logs().size() == 1); + } - /* SECTION("Logs can be added with FinishSpanOptions.") { */ - /* auto span = tracer->StartSpan("a"); */ - /* CHECK(span); */ - /* opentracing::FinishSpanOptions options; */ - /* options.log_records = {{SystemClock::now(), {{"abc", 123}}}}; */ - /* span->FinishWithOptions(options); */ - /* CHECK(recorder->top().logs().size() == 1); */ - /* } */ + SECTION("Logs can be added with FinishSpanOptions.") { + auto span = tracer->StartSpan("a"); + CHECK(span); + opentracing::FinishSpanOptions options; + options.log_records = {{SystemClock::now(), {{"abc", 123}}}}; + span->FinishWithOptions(options); + CHECK(recorder->top().logs().size() == 1); + } }