Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Span::Log() using annotations #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions zipkin_opentracing/src/opentracing.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::mutex> lock{mutex_};

// Set appropriate CS/SR/SS/CR annotations if span.kind is set.
Expand Down Expand Up @@ -218,6 +218,23 @@ class OtSpan : public ot::Span {
for (const auto &tag : tags_) {
span_->addBinaryAnnotation(toBinaryAnnotation(tag.first, tag.second));
}

std::vector<ot::LogRecord> 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));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need to be making copies like this. Could we instead add a static function called something like appendLogAnnotations and then replace this with something like

appendLogAnnotations(log_records_, span_);
appendLogAnnotations(options.log_records, span_);


for (const auto &lr : log_records) {
const auto timestamp = std::chrono::duration_cast<std::chrono::microseconds>(
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.
Expand Down Expand Up @@ -246,8 +263,10 @@ class OtSpan : public ot::Span {
return span_context_.baggageItem(restricted_key);
}

void Log(std::initializer_list<std::pair<string_view, Value>>
fields) noexcept override {}
void Log(std::initializer_list<std::pair<string_view, Value>> fields) noexcept override {
std::lock_guard<std::mutex> lock_guard{mutex_};
log_records_.push_back({ SystemClock::now(), { fields.begin(), fields.end() } });
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can potentially throw std::bad_alloc, so you need a try/catch block here like what's done in SetTag.


const ot::SpanContext &context() const noexcept override {
return span_context_;
Expand All @@ -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<bool> is_finished_{false};
std::mutex mutex_;
std::unordered_map<std::string, Value> tags_;
std::vector<ot::LogRecord> log_records_;
SpanPtr span_;
};

Expand Down
49 changes: 30 additions & 19 deletions zipkin_opentracing/src/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to

annotate.setValue(apply_visitor(value_visitor, value));

or

std::string str = apply_visitor(value_visitor, value);
annotate.setValue(std::move(str));

so that we can potentially avoid the copy with move semantics.

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
5 changes: 5 additions & 0 deletions zipkin_opentracing/src/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
#include <zipkin/zipkin_core_types.h>

namespace zipkin {

BinaryAnnotation toBinaryAnnotation(opentracing::string_view key,
const opentracing::Value &value);

Annotation toAnnotation(opentracing::string_view key,
const opentracing::Value &value);

} // namespace zipkin