From 56254224552a5b02f18ae5de6fb7b221f2de995e Mon Sep 17 00:00:00 2001 From: Jean-Marie Comets Date: Fri, 13 Apr 2018 16:04:45 +0200 Subject: [PATCH 1/3] Implement `Span::Log()` using annotations --- zipkin_opentracing/src/opentracing.cc | 28 ++++++++++++--- zipkin_opentracing/src/utility.cc | 49 ++++++++++++++++----------- zipkin_opentracing/src/utility.h | 5 +++ 3 files changed, 59 insertions(+), 23 deletions(-) diff --git a/zipkin_opentracing/src/opentracing.cc b/zipkin_opentracing/src/opentracing.cc index 771c331..52613d1 100644 --- a/zipkin_opentracing/src/opentracing.cc +++ b/zipkin_opentracing/src/opentracing.cc @@ -186,7 +186,7 @@ class OtSpan : public ot::Span { .count()); span_->setDuration(duration_microsecs); - // Set tags and finish + // Set tags, log records and finish std::lock_guard lock{mutex_}; // Set appropriate CS/SR/SS/CR annotations if span.kind is set. @@ -218,6 +218,23 @@ class OtSpan : public ot::Span { for (const auto &tag : tags_) { span_->addBinaryAnnotation(toBinaryAnnotation(tag.first, tag.second)); } + + std::vector log_records; + log_records.swap(log_records_); + log_records.reserve(options.log_records.size()); + std::copy(options.log_records.begin(), options.log_records.end(), + std::back_inserter(log_records)); + + for (const auto &lr : log_records) { + const auto timestamp = std::chrono::duration_cast( + lr.timestamp.time_since_epoch()).count(); + for (const auto& field : lr.fields) { + Annotation annotation = toAnnotation(field.first, field.second); + annotation.setTimestamp(timestamp); + span_->addAnnotation(annotation); + } + } + span_->finish(); } catch (const std::bad_alloc &) { // Do nothing if memory allocation fails. @@ -246,8 +263,10 @@ class OtSpan : public ot::Span { return span_context_.baggageItem(restricted_key); } - void Log(std::initializer_list> - fields) noexcept override {} + void Log(std::initializer_list> fields) noexcept override { + std::lock_guard lock_guard{mutex_}; + log_records_.push_back({ SystemClock::now(), { fields.begin(), fields.end() } }); + } const ot::SpanContext &context() const noexcept override { return span_context_; @@ -261,10 +280,11 @@ class OtSpan : public ot::Span { OtSpanContext span_context_; SteadyTime start_steady_timestamp_; - // Mutex protects tags_ and span_ + // Mutex protects tags_, log_records_ and span_ std::atomic is_finished_{false}; std::mutex mutex_; std::unordered_map tags_; + std::vector log_records_; SpanPtr span_; }; diff --git a/zipkin_opentracing/src/utility.cc b/zipkin_opentracing/src/utility.cc index e666764..cdc1565 100644 --- a/zipkin_opentracing/src/utility.cc +++ b/zipkin_opentracing/src/utility.cc @@ -78,48 +78,59 @@ static std::string toJson(const Value &value) { } namespace { -struct ValueVisitor { - BinaryAnnotation &annotation; + +struct ToStringValueVisitor { const Value &original_value; - void operator()(bool value) const { - annotation.setValue(std::to_string(value)); + std::string operator()(bool value) const { + return std::to_string(value); } - void operator()(double value) const { - annotation.setValue(std::to_string(value)); + std::string operator()(double value) const { + return std::to_string(value); } - void operator()(int64_t value) const { - annotation.setValue(std::to_string(value)); + std::string operator()(int64_t value) const { + return std::to_string(value); } - void operator()(uint64_t value) const { + std::string operator()(uint64_t value) const { // There's no uint64_t value type so cast to an int64_t. - annotation.setValue(std::to_string(value)); + return std::to_string(value); } - void operator()(const std::string &s) const { annotation.setValue(s); } + std::string operator()(const std::string &s) const { return s; } - void operator()(std::nullptr_t) const { annotation.setValue("0"); } + std::string operator()(std::nullptr_t) const { return "0"; } - void operator()(const char *s) const { annotation.setValue(std::string{s}); } + std::string operator()(const char *s) const { return s; } - void operator()(const Values & /*unused*/) const { - annotation.setValue(toJson(original_value)); + std::string operator()(const Values & /*unused*/) const { + return toJson(original_value); } - void operator()(const Dictionary & /*unused*/) const { - annotation.setValue(toJson(original_value)); + std::string operator()(const Dictionary & /*unused*/) const { + return toJson(original_value); } }; + } // anonymous namespace BinaryAnnotation toBinaryAnnotation(string_view key, const Value &value) { + ToStringValueVisitor value_visitor{value}; + const std::string str = apply_visitor(value_visitor, value); BinaryAnnotation annotation; annotation.setKey(key); - ValueVisitor value_visitor{annotation, value}; - apply_visitor(value_visitor, value); + annotation.setValue(str); return annotation; } + +Annotation toAnnotation(string_view key, const Value &value) { + ToStringValueVisitor value_visitor{value}; + const std::string str = apply_visitor(value_visitor, value); + Annotation annotation; + annotation.setValue(std::string(key) + "=" + str); + return annotation; +} + } // namespace zipkin diff --git a/zipkin_opentracing/src/utility.h b/zipkin_opentracing/src/utility.h index fb64597..09980f3 100644 --- a/zipkin_opentracing/src/utility.h +++ b/zipkin_opentracing/src/utility.h @@ -5,6 +5,11 @@ #include namespace zipkin { + BinaryAnnotation toBinaryAnnotation(opentracing::string_view key, const opentracing::Value &value); + +Annotation toAnnotation(opentracing::string_view key, + const opentracing::Value &value); + } // namespace zipkin From 2d6b197b2e38ea9807289cb7c15e5a7f600ce488 Mon Sep 17 00:00:00 2001 From: Jean-Marie Comets Date: Tue, 17 Apr 2018 12:18:34 +0200 Subject: [PATCH 2/3] fixup! Review fixes --- zipkin_opentracing/src/opentracing.cc | 26 +++++++++++++------------- zipkin_opentracing/src/utility.cc | 14 ++++++++++---- zipkin_opentracing/src/utility.h | 3 +-- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/zipkin_opentracing/src/opentracing.cc b/zipkin_opentracing/src/opentracing.cc index 52613d1..43737d5 100644 --- a/zipkin_opentracing/src/opentracing.cc +++ b/zipkin_opentracing/src/opentracing.cc @@ -119,6 +119,17 @@ static const OtSpanContext *findSpanContext( return nullptr; } +static void appendAnnotations(Span& span, const std::vector& log_records, Endpoint endpoint) { + for (const auto &lr : log_records) { + const auto timestamp = std::chrono::duration_cast( + lr.timestamp.time_since_epoch()).count(); + Annotation annotation = toAnnotation({ lr.fields.begin(), lr.fields.end() }); + annotation.setTimestamp(timestamp); + annotation.setEndpoint(endpoint); + span.addAnnotation(annotation); + } +} + class OtSpan : public ot::Span { public: OtSpan(std::shared_ptr &&tracer_owner, SpanPtr &&span_owner, @@ -221,19 +232,8 @@ class OtSpan : public ot::Span { std::vector log_records; log_records.swap(log_records_); - log_records.reserve(options.log_records.size()); - std::copy(options.log_records.begin(), options.log_records.end(), - std::back_inserter(log_records)); - - for (const auto &lr : log_records) { - const auto timestamp = std::chrono::duration_cast( - lr.timestamp.time_since_epoch()).count(); - for (const auto& field : lr.fields) { - Annotation annotation = toAnnotation(field.first, field.second); - annotation.setTimestamp(timestamp); - span_->addAnnotation(annotation); - } - } + appendAnnotations(*span_, log_records, endpoint_); + appendAnnotations(*span_, options.log_records, endpoint_); span_->finish(); } catch (const std::bad_alloc &) { diff --git a/zipkin_opentracing/src/utility.cc b/zipkin_opentracing/src/utility.cc index cdc1565..6450fde 100644 --- a/zipkin_opentracing/src/utility.cc +++ b/zipkin_opentracing/src/utility.cc @@ -125,11 +125,17 @@ BinaryAnnotation toBinaryAnnotation(string_view key, const Value &value) { return annotation; } -Annotation toAnnotation(string_view key, const Value &value) { - ToStringValueVisitor value_visitor{value}; - const std::string str = apply_visitor(value_visitor, value); +Annotation toAnnotation(const std::vector>& fields) { + rapidjson::StringBuffer buffer; + JsonWriter writer(buffer); + writer.StartObject(); + for (auto & field : fields) { + writer.Key(field.first.data()); + toJson(writer, field.second); + } + writer.EndObject(); Annotation annotation; - annotation.setValue(std::string(key) + "=" + str); + annotation.setValue(buffer.GetString()); return annotation; } diff --git a/zipkin_opentracing/src/utility.h b/zipkin_opentracing/src/utility.h index 09980f3..6185f04 100644 --- a/zipkin_opentracing/src/utility.h +++ b/zipkin_opentracing/src/utility.h @@ -9,7 +9,6 @@ namespace zipkin { BinaryAnnotation toBinaryAnnotation(opentracing::string_view key, const opentracing::Value &value); -Annotation toAnnotation(opentracing::string_view key, - const opentracing::Value &value); +Annotation toAnnotation(const std::vector>& fields); } // namespace zipkin From 428be6c674f2b168f9bba5e76722db27cfa4c794 Mon Sep 17 00:00:00 2001 From: Jean-Marie Comets Date: Fri, 20 Apr 2018 11:04:22 +0200 Subject: [PATCH 3/3] fixup! setValue shouldn't write only strings --- zipkin_opentracing/src/utility.cc | 51 +++++++++++++++++++------------ 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/zipkin_opentracing/src/utility.cc b/zipkin_opentracing/src/utility.cc index 6450fde..673ecba 100644 --- a/zipkin_opentracing/src/utility.cc +++ b/zipkin_opentracing/src/utility.cc @@ -79,49 +79,60 @@ static std::string toJson(const Value &value) { namespace { -struct ToStringValueVisitor { +struct SetValueVisitor { + BinaryAnnotation& annotation; const Value &original_value; - std::string operator()(bool value) const { - return std::to_string(value); + // natively handled types + + void operator()(bool value) const { + annotation.setValue(value); } - std::string operator()(double value) const { - return std::to_string(value); + void operator()(double value) const { + annotation.setValue(value); } - std::string operator()(int64_t value) const { - return std::to_string(value); + void operator()(int64_t value) const { + annotation.setValue(value); } - std::string operator()(uint64_t value) const { - // There's no uint64_t value type so cast to an int64_t. - return std::to_string(value); + void operator()(const std::string &s) const { + annotation.setValue(s); } - std::string operator()(const std::string &s) const { return s; } + // overrides - std::string operator()(std::nullptr_t) const { return "0"; } + void operator()(uint64_t value) const { + // There's no uint64_t value type so cast to an int64_t. + int64_t cast = value; + (*this)(cast); + } - std::string operator()(const char *s) const { return s; } + void operator()(std::nullptr_t) const { + (*this)("0"); + } + + void operator()(const char *s) const { + (*this)(std::string(s)); + } - std::string operator()(const Values & /*unused*/) const { - return toJson(original_value); + void operator()(const Values & /*unused*/) const { + (*this)(toJson(original_value)); } - std::string operator()(const Dictionary & /*unused*/) const { - return toJson(original_value); + void operator()(const Dictionary & /*unused*/) const { + (*this)(toJson(original_value)); } }; } // anonymous namespace BinaryAnnotation toBinaryAnnotation(string_view key, const Value &value) { - ToStringValueVisitor value_visitor{value}; - const std::string str = apply_visitor(value_visitor, value); BinaryAnnotation annotation; annotation.setKey(key); - annotation.setValue(str); + SetValueVisitor visitor{annotation, value}; + apply_visitor(visitor, value); return annotation; }