From 82e92da3b06a22bdb7336015b7cd2eb13900cf2b Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Aug 2018 23:04:35 -0700 Subject: [PATCH 01/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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); + } } From 4ae88ed3538c380b24f15c5b03ccf68d1531823c Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 3 Sep 2018 11:35:19 -0700 Subject: [PATCH 17/30] Add benchmark for multikey injection. --- benchmark/span_operations_benchmark.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/benchmark/span_operations_benchmark.cpp b/benchmark/span_operations_benchmark.cpp index ca539698..aa694d7f 100644 --- a/benchmark/span_operations_benchmark.cpp +++ b/benchmark/span_operations_benchmark.cpp @@ -13,6 +13,19 @@ class NullTransporter final : public lightstep::SyncTransporter { }; } // namespace +namespace { +class NullTextMapWriter final : public opentracing::TextMapWriter { + public: + opentracing::expected Set( + opentracing::string_view key, + opentracing::string_view value) const override { + benchmark::DoNotOptimize(key.data()); + benchmark::DoNotOptimize(value.data()); + return {}; + } +}; +} // namespace + static std::shared_ptr MakeTracer() { lightstep::LightStepTracerOptions options; options.access_token = "abc123"; @@ -94,4 +107,16 @@ static void BM_SpanLog2(benchmark::State& state) { } BENCHMARK(BM_SpanLog2); +static void BM_SpanMultikeyInjection(benchmark::State& state) { + auto tracer = MakeTracer(); + assert(tracer != nullptr); + auto span = tracer->StartSpan("abc123"); + assert(span != nullptr); + NullTextMapWriter carrier; + for (auto _ : state) { + tracer->Inject(span->context(), carrier); + } +} +BENCHMARK(BM_SpanMultikeyInjection); + BENCHMARK_MAIN(); From 8477732039b3cbff3105bd559a8d07ab0669a8cd Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 3 Sep 2018 11:52:58 -0700 Subject: [PATCH 18/30] Add span context extraction benchmark. --- benchmark/span_operations_benchmark.cpp | 63 ++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/benchmark/span_operations_benchmark.cpp b/benchmark/span_operations_benchmark.cpp index aa694d7f..e2212267 100644 --- a/benchmark/span_operations_benchmark.cpp +++ b/benchmark/span_operations_benchmark.cpp @@ -26,6 +26,50 @@ class NullTextMapWriter final : public opentracing::TextMapWriter { }; } // namespace +namespace { +class TextMapWriter final : public opentracing::TextMapWriter { + public: + explicit TextMapWriter( + std::vector>& key_values) + : key_values_{key_values} {} + + opentracing::expected Set( + opentracing::string_view key, + opentracing::string_view value) const override { + key_values_.emplace_back(key, value); + return {}; + } + + private: + std::vector>& key_values_; +}; +} // namespace + +namespace { +class TextMapReader final : public opentracing::TextMapReader { + public: + explicit TextMapReader( + const std::vector>& key_values) + : key_values_{key_values} {} + + opentracing::expected ForeachKey( + std::function(opentracing::string_view key, + opentracing::string_view value)> + f) const override { + for (auto key_value : key_values_) { + auto was_successful = f(key_value.first, key_value.second); + if (!was_successful) { + return was_successful; + } + } + return {}; + } + + private: + const std::vector>& key_values_; +}; +} // namespace + static std::shared_ptr MakeTracer() { lightstep::LightStepTracerOptions options; options.access_token = "abc123"; @@ -114,9 +158,26 @@ static void BM_SpanMultikeyInjection(benchmark::State& state) { assert(span != nullptr); NullTextMapWriter carrier; for (auto _ : state) { - tracer->Inject(span->context(), carrier); + auto was_successful = tracer->Inject(span->context(), carrier); + assert(was_successful); } } BENCHMARK(BM_SpanMultikeyInjection); +static void BM_SpanMultikeyExtraction(benchmark::State& state) { + auto tracer = MakeTracer(); + assert(tracer != nullptr); + auto span = tracer->StartSpan("abc123"); + std::vector> key_values; + auto was_successful = tracer->Inject(span->context(), TextMapWriter{key_values}); + assert(was_successful); + TextMapReader carrier{key_values}; + for (auto _ : state) { + auto span_context_maybe = tracer->Extract(carrier); + assert(span_context_maybe); + benchmark::DoNotOptimize(span_context_maybe); + } +} +BENCHMARK(BM_SpanMultikeyExtraction); + BENCHMARK_MAIN(); From 6928726ca524c769e0d952b7a7c11c31d96161ea Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 3 Sep 2018 11:53:11 -0700 Subject: [PATCH 19/30] Run clang-format. --- benchmark/span_operations_benchmark.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/benchmark/span_operations_benchmark.cpp b/benchmark/span_operations_benchmark.cpp index e2212267..85d80f88 100644 --- a/benchmark/span_operations_benchmark.cpp +++ b/benchmark/span_operations_benchmark.cpp @@ -169,7 +169,8 @@ static void BM_SpanMultikeyExtraction(benchmark::State& state) { assert(tracer != nullptr); auto span = tracer->StartSpan("abc123"); std::vector> key_values; - auto was_successful = tracer->Inject(span->context(), TextMapWriter{key_values}); + auto was_successful = + tracer->Inject(span->context(), TextMapWriter{key_values}); assert(was_successful); TextMapReader carrier{key_values}; for (auto _ : state) { From 5602822215cfe995f000cd0bc67a9338b58de1fc Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 3 Sep 2018 14:31:32 -0700 Subject: [PATCH 20/30] Add benhmark for empty span context extraction. --- benchmark/span_operations_benchmark.cpp | 27 +++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/benchmark/span_operations_benchmark.cpp b/benchmark/span_operations_benchmark.cpp index 85d80f88..48ef1d21 100644 --- a/benchmark/span_operations_benchmark.cpp +++ b/benchmark/span_operations_benchmark.cpp @@ -151,7 +151,7 @@ static void BM_SpanLog2(benchmark::State& state) { } BENCHMARK(BM_SpanLog2); -static void BM_SpanMultikeyInjection(benchmark::State& state) { +static void BM_SpanContextMultikeyInjection(benchmark::State& state) { auto tracer = MakeTracer(); assert(tracer != nullptr); auto span = tracer->StartSpan("abc123"); @@ -162,9 +162,9 @@ static void BM_SpanMultikeyInjection(benchmark::State& state) { assert(was_successful); } } -BENCHMARK(BM_SpanMultikeyInjection); +BENCHMARK(BM_SpanContextMultikeyInjection); -static void BM_SpanMultikeyExtraction(benchmark::State& state) { +static void BM_SpanContextMultikeyExtraction(benchmark::State& state) { auto tracer = MakeTracer(); assert(tracer != nullptr); auto span = tracer->StartSpan("abc123"); @@ -179,6 +179,25 @@ static void BM_SpanMultikeyExtraction(benchmark::State& state) { benchmark::DoNotOptimize(span_context_maybe); } } -BENCHMARK(BM_SpanMultikeyExtraction); +BENCHMARK(BM_SpanContextMultikeyExtraction); + +static void BM_SpanContextMultikeyEmptyExtraction(benchmark::State& state) { + auto tracer = MakeTracer(); + assert(tracer != nullptr); + std::vector> key_values = { + {"Accept", "text/html"}, + {"Content-Length", "300"}, + {"User-Agent", "Mozilla/5.0"}, + {"Pragma", "no-cache"}, + {"Host", "www.memyselfandi.com"}}; + TextMapReader carrier{key_values}; + for (auto _ : state) { + auto span_context_maybe = tracer->Extract(carrier); + assert(span_context_maybe); + assert(*span_context_maybe == nullptr); + benchmark::DoNotOptimize(span_context_maybe); + } +} +BENCHMARK(BM_SpanContextMultikeyEmptyExtraction); BENCHMARK_MAIN(); From 19923473167dab460b640c1bb6294347eeeba09f Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 3 Sep 2018 15:23:02 -0700 Subject: [PATCH 21/30] Fix TextMapReader. --- benchmark/span_operations_benchmark.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmark/span_operations_benchmark.cpp b/benchmark/span_operations_benchmark.cpp index 48ef1d21..fbd82da5 100644 --- a/benchmark/span_operations_benchmark.cpp +++ b/benchmark/span_operations_benchmark.cpp @@ -56,7 +56,7 @@ class TextMapReader final : public opentracing::TextMapReader { std::function(opentracing::string_view key, opentracing::string_view value)> f) const override { - for (auto key_value : key_values_) { + for (auto& key_value : key_values_) { auto was_successful = f(key_value.first, key_value.second); if (!was_successful) { return was_successful; From 7be7c68dd3a053ee18292ce55a50d2bb1b6facde Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 3 Sep 2018 23:24:14 -0700 Subject: [PATCH 22/30] Rewrite Uint64ToHex function. --- src/propagation.cpp | 69 ++++++++++++++++++++++--------------------- src/utility.cpp | 32 ++++++++++++++++++++ src/utility.h | 4 +++ test/utility_test.cpp | 22 ++++++++++++++ 4 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/propagation.cpp b/src/propagation.cpp index e79c799e..df78a179 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -9,6 +9,7 @@ #include #include "in_memory_stream.h" #include "lightstep-tracer-common/lightstep_carrier.pb.h" +#include "utility.h" namespace lightstep { #define PREFIX_TRACER_STATE "ot-tracer-" @@ -22,16 +23,6 @@ const opentracing::string_view FieldNameSampled = PREFIX_TRACER_STATE "sampled"; #undef PREFIX_TRACER_STATE const opentracing::string_view PropagationSingleKey = "x-ot-span-context"; - -//------------------------------------------------------------------------------ -// Uint64ToHex -//------------------------------------------------------------------------------ -static std::string Uint64ToHex(uint64_t u) { - std::ostringstream stream; - stream << std::setfill('0') << std::setw(16) << std::hex << u; - return stream.str(); -} - //------------------------------------------------------------------------------ // HexToUint64 //------------------------------------------------------------------------------ @@ -76,48 +67,60 @@ static opentracing::expected LookupKey( } //------------------------------------------------------------------------------ -// InjectSpanContextMultiKey +// InjectSpanContextBaggage //------------------------------------------------------------------------------ -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; +static opentracing::expected InjectSpanContextBaggage( + const opentracing::TextMapWriter& carrier, const BaggageMap& baggage) { + std::string 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); + 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)); + } + auto result = carrier.Set(baggage_key, baggage_item.second); + if (!result) { + return result; + } + } + return {}; +} + +//------------------------------------------------------------------------------ +// InjectSpanContextMultiKey +//------------------------------------------------------------------------------ +static opentracing::expected InjectSpanContextMultiKey( + const opentracing::TextMapWriter& carrier, uint64_t trace_id, + uint64_t span_id, bool sampled, const BaggageMap& baggage) { + char data[16]; + auto result = carrier.Set(FieldNameTraceID, Uint64ToHex(trace_id, data)); if (!result) { return result; } - result = carrier.Set(FieldNameSpanID, span_id_hex); + result = carrier.Set(FieldNameSpanID, Uint64ToHex(span_id, data)); if (!result) { return result; } + static opentracing::string_view true_str{"true"}; + static opentracing::string_view false_str{"false"}; if (sampled) { - result = carrier.Set(FieldNameSampled, "true"); + result = carrier.Set(FieldNameSampled, true_str); } else { - result = carrier.Set(FieldNameSampled, "false"); + result = carrier.Set(FieldNameSampled, false_str); } 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; - } + if (!baggage.empty()) { + return InjectSpanContextBaggage(carrier, baggage); } return {}; } diff --git a/src/utility.cpp b/src/utility.cpp index 9b23851e..4b0f3855 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -264,4 +264,36 @@ void LogReportResponse(Logger& logger, bool verbose, } } } + +//------------------------------------------------------------------------------ +// Uint64ToHex +//------------------------------------------------------------------------------ +// This uses the lookup table solution described on this blog post +// https://johnnylee-sde.github.io/Fast-unsigned-integer-to-hex-string/ +opentracing::string_view Uint64ToHex(uint64_t x, char* output) { + static const char digits[513] = + "000102030405060708090A0B0C0D0E0F" + "101112131415161718191A1B1C1D1E1F" + "202122232425262728292A2B2C2D2E2F" + "303132333435363738393A3B3C3D3E3F" + "404142434445464748494A4B4C4D4E4F" + "505152535455565758595A5B5C5D5E5F" + "606162636465666768696A6B6C6D6E6F" + "707172737475767778797A7B7C7D7E7F" + "808182838485868788898A8B8C8D8E8F" + "909192939495969798999A9B9C9D9E9F" + "A0A1A2A3A4A5A6A7A8A9AAABACADAEAF" + "B0B1B2B3B4B5B6B7B8B9BABBBCBDBEBF" + "C0C1C2C3C4C5C6C7C8C9CACBCCCDCECF" + "D0D1D2D3D4D5D6D7D8D9DADBDCDDDEDF" + "E0E1E2E3E4E5E6E7E8E9EAEBECEDEEEF" + "F0F1F2F3F4F5F6F7F8F9FAFBFCFDFEFF"; + for (int i = 8; i-- > 0;) { + auto lookup_index = (x & 0xFF) * 2; + output[i * 2] = digits[lookup_index]; + output[i * 2 + 1] = digits[lookup_index + 1]; + x >>= 8; + } + return {output, 16}; +} } // namespace lightstep diff --git a/src/utility.h b/src/utility.h index ffa078ad..545b278a 100644 --- a/src/utility.h +++ b/src/utility.h @@ -28,4 +28,8 @@ collector::KeyValue ToKeyValue(opentracing::string_view key, // Logs any information returned by the collector. void LogReportResponse(Logger& logger, bool verbose, const collector::ReportResponse& response); + +// Converts `x` to a hexidecimal, writes the results into `output` and returns +// a string_view of the number. +opentracing::string_view Uint64ToHex(uint64_t x, char* output); } // namespace lightstep diff --git a/test/utility_test.cpp b/test/utility_test.cpp index 37fc5304..62440d82 100644 --- a/test/utility_test.cpp +++ b/test/utility_test.cpp @@ -1,6 +1,7 @@ #include "../src/utility.h" #include #include +#include #define CATCH_CONFIG_MAIN #include @@ -55,3 +56,24 @@ TEST_CASE("Json") { CHECK(key_value1.json_value() == "[null]"); } } + +TEST_CASE("Uint64ToHex") { + auto f = [](const std::string& s) { + std::istringstream stream{s}; + uint64_t x; + stream >> std::setw(16) >> std::hex >> x; + return x; + }; + char data[16]; + + SECTION("Verify hex conversion and back against a range of values.") { + for (uint64_t x = 0; x < 1000; ++x) { + CHECK(x == f(Uint64ToHex(x, data))); + } + } + + SECTION("Verify hex conversion against the maximum possible value.") { + auto x = std::numeric_limits::max(); + CHECK(x == f(Uint64ToHex(x, data))); + } +} From 87b6bb73d250bb4b32c56d8b6829c9384d4f7d1f Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 4 Sep 2018 15:49:17 -0700 Subject: [PATCH 23/30] Improve extraction performance. --- src/propagation.cpp | 24 +++++++-------- src/utility.cpp | 72 ++++++++++++++++++++++++++++++++++++++++++- src/utility.h | 2 ++ test/utility_test.cpp | 51 +++++++++++++++++++++++------- 4 files changed, 125 insertions(+), 24 deletions(-) diff --git a/src/propagation.cpp b/src/propagation.cpp index df78a179..d539752f 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -23,16 +23,6 @@ const opentracing::string_view FieldNameSampled = PREFIX_TRACER_STATE "sampled"; #undef PREFIX_TRACER_STATE const opentracing::string_view PropagationSingleKey = "x-ot-span-context"; -//------------------------------------------------------------------------------ -// HexToUint64 -//------------------------------------------------------------------------------ -static uint64_t HexToUint64(opentracing::string_view s) { - in_memory_stream stream{s.data(), s.size()}; - uint64_t x; - stream >> std::setw(16) >> std::hex >> x; - return x; -} - //------------------------------------------------------------------------------ // LookupKey //------------------------------------------------------------------------------ @@ -227,10 +217,20 @@ static opentracing::expected ExtractSpanContextMultiKey( -> opentracing::expected { try { if (key_compare(key, FieldNameTraceID)) { - trace_id = HexToUint64(value); + auto trace_id_maybe = ToUint64(value); + if (!trace_id_maybe) { + return opentracing::make_unexpected( + opentracing::span_context_corrupted_error); + } + trace_id = *trace_id_maybe; count++; } else if (key_compare(key, FieldNameSpanID)) { - span_id = HexToUint64(value); + auto span_id_maybe = ToUint64(value); + if (!span_id_maybe) { + return opentracing::make_unexpected( + opentracing::span_context_corrupted_error); + } + span_id = *span_id_maybe; count++; } else if (key_compare(key, FieldNameSampled)) { sampled = !(value == "false" || value == "0"); diff --git a/src/utility.cpp b/src/utility.cpp index 4b0f3855..fb980215 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -5,11 +5,14 @@ #include "lightstep-tracer-common/collector.pb.h" #include +#include +#include #include #include #include #include #include +#include #include @@ -271,7 +274,7 @@ void LogReportResponse(Logger& logger, bool verbose, // This uses the lookup table solution described on this blog post // https://johnnylee-sde.github.io/Fast-unsigned-integer-to-hex-string/ opentracing::string_view Uint64ToHex(uint64_t x, char* output) { - static const char digits[513] = + static const unsigned char digits[513] = "000102030405060708090A0B0C0D0E0F" "101112131415161718191A1B1C1D1E1F" "202122232425262728292A2B2C2D2E2F" @@ -296,4 +299,71 @@ opentracing::string_view Uint64ToHex(uint64_t x, char* output) { } return {output, 16}; } + +//------------------------------------------------------------------------------ +// ToUint64 +//------------------------------------------------------------------------------ +// Adopted from https://stackoverflow.com/a/11068850/4447365 +opentracing::expected ToUint64(opentracing::string_view s) { + static const unsigned char nil = std::numeric_limits::max(); + static const std::array hextable = { + {nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, 0, 1, 2, 3, 4, 5, 6, 7, + 8, 9, nil, nil, nil, nil, nil, nil, nil, 10, 11, 12, 13, 14, + 15, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 10, + 11, 12, 13, 14, 15, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + nil, nil, nil, nil}}; + + auto i = s.data(); + auto last = s.data() + s.size(); + + // Remove any leading spaces or 0's + while (i != last && std::isspace(*i)) { + ++i; + } + + // Remove any trailing spaces + while (i != last && std::isspace(*(last - 1))) { + --last; + } + + auto length = std::distance(i, last); + + // Check for overflow + if (length > 16) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::value_too_large)); + } + + // Check for an empty string + if (length == 0) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::invalid_argument)); + } + + uint64_t result = 0; + for (; i != last; ++i) { + auto value = hextable[*i]; + if (value == nil) { + return opentracing::make_unexpected( + std::make_error_code(std::errc::invalid_argument)); + } + result = (result << 4) | value; + } + + return result; +} } // namespace lightstep diff --git a/src/utility.h b/src/utility.h index 545b278a..c072dc51 100644 --- a/src/utility.h +++ b/src/utility.h @@ -32,4 +32,6 @@ void LogReportResponse(Logger& logger, bool verbose, // Converts `x` to a hexidecimal, writes the results into `output` and returns // a string_view of the number. opentracing::string_view Uint64ToHex(uint64_t x, char* output); + +opentracing::expected ToUint64(opentracing::string_view s); } // namespace lightstep diff --git a/test/utility_test.cpp b/test/utility_test.cpp index 62440d82..64fee257 100644 --- a/test/utility_test.cpp +++ b/test/utility_test.cpp @@ -57,23 +57,52 @@ TEST_CASE("Json") { } } -TEST_CASE("Uint64ToHex") { - auto f = [](const std::string& s) { - std::istringstream stream{s}; - uint64_t x; - stream >> std::setw(16) >> std::hex >> x; - return x; - }; +TEST_CASE("hex-integer conversions") { char data[16]; SECTION("Verify hex conversion and back against a range of values.") { for (uint64_t x = 0; x < 1000; ++x) { - CHECK(x == f(Uint64ToHex(x, data))); + CHECK(x == *ToUint64(Uint64ToHex(x, data))); + auto y = std::numeric_limits::max() - x; + CHECK(y == *ToUint64(Uint64ToHex(y, data))); } } - SECTION("Verify hex conversion against the maximum possible value.") { - auto x = std::numeric_limits::max(); - CHECK(x == f(Uint64ToHex(x, data))); + SECTION("Verify a few special values.") { + CHECK(Uint64ToHex(0, data) == "0000000000000000"); + CHECK(Uint64ToHex(1, data) == "0000000000000001"); + CHECK(Uint64ToHex(std::numeric_limits::max(), data) == + "FFFFFFFFFFFFFFFF"); + + CHECK(*ToUint64("0") == 0); + CHECK(*ToUint64("1") == 1); + CHECK(*ToUint64("FFFFFFFFFFFFFFFF") == + std::numeric_limits::max()); + } + + SECTION("Leading or trailing spaces are ignored when converting from hex.") { + CHECK(*ToUint64(" \tabc") == 0xabc); + CHECK(*ToUint64("abc \t") == 0xabc); + CHECK(*ToUint64(" \tabc \t") == 0xabc); + } + + SECTION("Hex conversion works with both upper and lower case digits.") { + CHECK(*ToUint64("ABCDEF") == 0xABCDEF); + CHECK(*ToUint64("abcdef") == 0xABCDEF); + } + + SECTION("Hex conversion with an empty string gives an error.") { + CHECK(!ToUint64("")); + CHECK(!ToUint64(" ")); + } + + SECTION( + "Hex conversion of a number bigger than " + "std::numeric_limits::max() gives an error.") { + CHECK(!ToUint64("0123456789ABCDEF1")); + } + + SECTION("Hex conversion with invalid digits gives an error.") { + CHECK(!ToUint64("abcHef")); } } From ddc5ff062bd67e99948890aa448c03e944ffc740 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 4 Sep 2018 22:34:27 -0700 Subject: [PATCH 24/30] s/ToUint64/HexToUint64/ --- src/propagation.cpp | 4 ++-- src/utility.cpp | 4 ++-- src/utility.h | 2 +- test/utility_test.cpp | 28 ++++++++++++++-------------- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/propagation.cpp b/src/propagation.cpp index d539752f..6b8b1160 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -217,7 +217,7 @@ static opentracing::expected ExtractSpanContextMultiKey( -> opentracing::expected { try { if (key_compare(key, FieldNameTraceID)) { - auto trace_id_maybe = ToUint64(value); + auto trace_id_maybe = HexToUint64(value); if (!trace_id_maybe) { return opentracing::make_unexpected( opentracing::span_context_corrupted_error); @@ -225,7 +225,7 @@ static opentracing::expected ExtractSpanContextMultiKey( trace_id = *trace_id_maybe; count++; } else if (key_compare(key, FieldNameSpanID)) { - auto span_id_maybe = ToUint64(value); + auto span_id_maybe = HexToUint64(value); if (!span_id_maybe) { return opentracing::make_unexpected( opentracing::span_context_corrupted_error); diff --git a/src/utility.cpp b/src/utility.cpp index fb980215..8d082e1a 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -301,10 +301,10 @@ opentracing::string_view Uint64ToHex(uint64_t x, char* output) { } //------------------------------------------------------------------------------ -// ToUint64 +// HexToUint64 //------------------------------------------------------------------------------ // Adopted from https://stackoverflow.com/a/11068850/4447365 -opentracing::expected ToUint64(opentracing::string_view s) { +opentracing::expected HexToUint64(opentracing::string_view s) { static const unsigned char nil = std::numeric_limits::max(); static const std::array hextable = { {nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, diff --git a/src/utility.h b/src/utility.h index c072dc51..75a05151 100644 --- a/src/utility.h +++ b/src/utility.h @@ -33,5 +33,5 @@ void LogReportResponse(Logger& logger, bool verbose, // a string_view of the number. opentracing::string_view Uint64ToHex(uint64_t x, char* output); -opentracing::expected ToUint64(opentracing::string_view s); +opentracing::expected HexToUint64(opentracing::string_view s); } // namespace lightstep diff --git a/test/utility_test.cpp b/test/utility_test.cpp index 64fee257..c153e088 100644 --- a/test/utility_test.cpp +++ b/test/utility_test.cpp @@ -62,9 +62,9 @@ TEST_CASE("hex-integer conversions") { SECTION("Verify hex conversion and back against a range of values.") { for (uint64_t x = 0; x < 1000; ++x) { - CHECK(x == *ToUint64(Uint64ToHex(x, data))); + CHECK(x == *HexToUint64(Uint64ToHex(x, data))); auto y = std::numeric_limits::max() - x; - CHECK(y == *ToUint64(Uint64ToHex(y, data))); + CHECK(y == *HexToUint64(Uint64ToHex(y, data))); } } @@ -74,35 +74,35 @@ TEST_CASE("hex-integer conversions") { CHECK(Uint64ToHex(std::numeric_limits::max(), data) == "FFFFFFFFFFFFFFFF"); - CHECK(*ToUint64("0") == 0); - CHECK(*ToUint64("1") == 1); - CHECK(*ToUint64("FFFFFFFFFFFFFFFF") == + CHECK(*HexToUint64("0") == 0); + CHECK(*HexToUint64("1") == 1); + CHECK(*HexToUint64("FFFFFFFFFFFFFFFF") == std::numeric_limits::max()); } SECTION("Leading or trailing spaces are ignored when converting from hex.") { - CHECK(*ToUint64(" \tabc") == 0xabc); - CHECK(*ToUint64("abc \t") == 0xabc); - CHECK(*ToUint64(" \tabc \t") == 0xabc); + CHECK(*HexToUint64(" \tabc") == 0xabc); + CHECK(*HexToUint64("abc \t") == 0xabc); + CHECK(*HexToUint64(" \tabc \t") == 0xabc); } SECTION("Hex conversion works with both upper and lower case digits.") { - CHECK(*ToUint64("ABCDEF") == 0xABCDEF); - CHECK(*ToUint64("abcdef") == 0xABCDEF); + CHECK(*HexToUint64("ABCDEF") == 0xABCDEF); + CHECK(*HexToUint64("abcdef") == 0xABCDEF); } SECTION("Hex conversion with an empty string gives an error.") { - CHECK(!ToUint64("")); - CHECK(!ToUint64(" ")); + CHECK(!HexToUint64("")); + CHECK(!HexToUint64(" ")); } SECTION( "Hex conversion of a number bigger than " "std::numeric_limits::max() gives an error.") { - CHECK(!ToUint64("0123456789ABCDEF1")); + CHECK(!HexToUint64("0123456789ABCDEF1")); } SECTION("Hex conversion with invalid digits gives an error.") { - CHECK(!ToUint64("abcHef")); + CHECK(!HexToUint64("abcHef")); } } From 3f7b8ecc145d39beff03554011b65d87ac67c812 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 4 Sep 2018 22:38:30 -0700 Subject: [PATCH 25/30] Add documentation. --- src/utility.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/utility.h b/src/utility.h index 75a05151..d89186d1 100644 --- a/src/utility.h +++ b/src/utility.h @@ -33,5 +33,7 @@ void LogReportResponse(Logger& logger, bool verbose, // a string_view of the number. opentracing::string_view Uint64ToHex(uint64_t x, char* output); +// Converts a hexidecimal number to a 64-bit integer. Either returns the number +// or an error code. opentracing::expected HexToUint64(opentracing::string_view s); } // namespace lightstep From e7f34d0d6a6e78578713cd775683b8c15c862985 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 4 Sep 2018 23:06:17 -0700 Subject: [PATCH 26/30] Clang-tidy fixes. --- cmake/Modules/LightStepClangTidy.cmake | 5 +++++ src/utility.cpp | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cmake/Modules/LightStepClangTidy.cmake b/cmake/Modules/LightStepClangTidy.cmake index ccee7535..ca34a60f 100644 --- a/cmake/Modules/LightStepClangTidy.cmake +++ b/cmake/Modules/LightStepClangTidy.cmake @@ -13,9 +13,14 @@ else() -google-build-using-namespace,\ -modernize-make-unique,\ -hicpp-vararg,\ +-hicpp-signed-bitwise,\ +-hicpp-no-array-decay,\ -cppcoreguidelines-owning-memory,\ -cppcoreguidelines-pro-type-reinterpret-cast,\ -cppcoreguidelines-pro-type-const-cast,\ +-cppcoreguidelines-pro-bounds-array-to-pointer-decay,\ +-ppcoreguidelines-pro-bounds-constant-array-index,\ +-cppcoreguidelines-pro-bounds-pointer-arithmetic,\ -cppcoreguidelines-pro-type-vararg;\ -warnings-as-errors=*") endif() diff --git a/src/utility.cpp b/src/utility.cpp index 8d082e1a..5539bcd1 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -331,12 +331,12 @@ opentracing::expected HexToUint64(opentracing::string_view s) { auto last = s.data() + s.size(); // Remove any leading spaces or 0's - while (i != last && std::isspace(*i)) { + while (i != last && static_cast(std::isspace(*i))) { ++i; } // Remove any trailing spaces - while (i != last && std::isspace(*(last - 1))) { + while (i != last && static_cast(std::isspace(*(last - 1)))) { --last; } From b98e7c5fb93abdfd39122cce6e0949f0564c68c8 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 4 Sep 2018 23:14:56 -0700 Subject: [PATCH 27/30] Fix comment. --- src/utility.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utility.cpp b/src/utility.cpp index 5539bcd1..1bb0baca 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -330,7 +330,7 @@ opentracing::expected HexToUint64(opentracing::string_view s) { auto i = s.data(); auto last = s.data() + s.size(); - // Remove any leading spaces or 0's + // Remove any leading spaces while (i != last && static_cast(std::isspace(*i))) { ++i; } From 9205df2ebefee5f0861dc7faabf1676460552a28 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 4 Sep 2018 23:18:15 -0700 Subject: [PATCH 28/30] Remove unused include. --- test/utility_test.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/utility_test.cpp b/test/utility_test.cpp index c153e088..40c23490 100644 --- a/test/utility_test.cpp +++ b/test/utility_test.cpp @@ -1,7 +1,6 @@ #include "../src/utility.h" #include #include -#include #define CATCH_CONFIG_MAIN #include From 7824be075ffb11169b80fff217e241f384aecfd0 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 5 Sep 2018 08:20:42 -0700 Subject: [PATCH 29/30] Use string_view constants instead of literals. --- cmake/Modules/LightStepClangTidy.cmake | 2 +- src/propagation.cpp | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cmake/Modules/LightStepClangTidy.cmake b/cmake/Modules/LightStepClangTidy.cmake index ca34a60f..2f28508b 100644 --- a/cmake/Modules/LightStepClangTidy.cmake +++ b/cmake/Modules/LightStepClangTidy.cmake @@ -19,7 +19,7 @@ else() -cppcoreguidelines-pro-type-reinterpret-cast,\ -cppcoreguidelines-pro-type-const-cast,\ -cppcoreguidelines-pro-bounds-array-to-pointer-decay,\ --ppcoreguidelines-pro-bounds-constant-array-index,\ +-cppcoreguidelines-pro-bounds-constant-array-index,\ -cppcoreguidelines-pro-bounds-pointer-arithmetic,\ -cppcoreguidelines-pro-type-vararg;\ -warnings-as-errors=*") diff --git a/src/propagation.cpp b/src/propagation.cpp index 6b8b1160..e11f9978 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -23,6 +23,9 @@ const opentracing::string_view FieldNameSampled = PREFIX_TRACER_STATE "sampled"; #undef PREFIX_TRACER_STATE const opentracing::string_view PropagationSingleKey = "x-ot-span-context"; +const opentracing::string_view TrueStr = "true"; +const opentracing::string_view FalseStr = "false"; +const opentracing::string_view ZeroStr = "0"; //------------------------------------------------------------------------------ // LookupKey //------------------------------------------------------------------------------ @@ -99,12 +102,10 @@ static opentracing::expected InjectSpanContextMultiKey( if (!result) { return result; } - static opentracing::string_view true_str{"true"}; - static opentracing::string_view false_str{"false"}; if (sampled) { - result = carrier.Set(FieldNameSampled, true_str); + result = carrier.Set(FieldNameSampled, TrueStr); } else { - result = carrier.Set(FieldNameSampled, false_str); + result = carrier.Set(FieldNameSampled, FalseStr); } if (!result) { return result; @@ -233,7 +234,7 @@ static opentracing::expected ExtractSpanContextMultiKey( span_id = *span_id_maybe; count++; } else if (key_compare(key, FieldNameSampled)) { - sampled = !(value == "false" || value == "0"); + sampled = !(value == FalseStr || value == ZeroStr); count++; } else if (key.length() > PrefixBaggage.size() && key_compare( From e22b40e41fadc7c1817d881a7688e746fdd35908 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 5 Sep 2018 08:24:56 -0700 Subject: [PATCH 30/30] Fix overflow issue. --- src/utility.cpp | 11 +++++++++++ test/utility_test.cpp | 8 +++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/utility.cpp b/src/utility.cpp index 1bb0baca..017da90a 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -340,6 +340,13 @@ opentracing::expected HexToUint64(opentracing::string_view s) { --last; } + auto first = i; + + // Remove leading zeros + while (i != last && *i == '0') { + ++i; + } + auto length = std::distance(i, last); // Check for overflow @@ -350,6 +357,10 @@ opentracing::expected HexToUint64(opentracing::string_view s) { // Check for an empty string if (length == 0) { + // Handle the case of the string being all zeros + if (first != i) { + return 0; + } return opentracing::make_unexpected( std::make_error_code(std::errc::invalid_argument)); } diff --git a/test/utility_test.cpp b/test/utility_test.cpp index 40c23490..79c29da4 100644 --- a/test/utility_test.cpp +++ b/test/utility_test.cpp @@ -98,7 +98,13 @@ TEST_CASE("hex-integer conversions") { SECTION( "Hex conversion of a number bigger than " "std::numeric_limits::max() gives an error.") { - CHECK(!HexToUint64("0123456789ABCDEF1")); + CHECK(!HexToUint64("1123456789ABCDEF1")); + } + + SECTION( + "Hex conversion of a number within valid limits but with leading zeros " + "past 16 digits is successful.") { + CHECK(HexToUint64("0123456789ABCDEF1")); } SECTION("Hex conversion with invalid digits gives an error.") {