Implement Span::Log() using annotations
#18
Conversation
|
Thanks @jmcomets - I'd be glad to have this functionality added in. I'll try to review this soon. |
| 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)); |
There was a problem hiding this comment.
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_);
|
Could you add some coverage for this into https://github.com/rnburn/zipkin-cpp-opentracing/blob/master/zipkin_opentracing/test/ot_tracer_test.cc#L129? |
zipkin_opentracing/src/utility.cc
Outdated
| BinaryAnnotation annotation; | ||
| annotation.setKey(key); | ||
| ValueVisitor value_visitor{annotation, value}; | ||
| apply_visitor(value_visitor, value); | ||
| annotation.setValue(str); |
There was a problem hiding this comment.
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.
| 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() } }); | ||
| } |
There was a problem hiding this comment.
This can potentially throw std::bad_alloc, so you need a try/catch block here like what's done in SetTag.
|
@jmcomets I actually had setValue use non-string values at one point, but I changed it to strings because the non-strings broke jaeger's zipkin compatibility mode. According to openzipkin/zipkin#939 (comment), non-string binary annotations aren't really supported. |
|
Thanks for the info, I wasn't sure when writing it. I'm off for the next
few weeks and won't be able to look more into it.
…On Fri, Apr 20, 2018, 18:18 Ryan ***@***.***> wrote:
@jmcomets <https://github.com/jmcomets> I actually had setValue use
non-string values at one point, but I changed it to strings because the
non-strings broke jaeger's zipkin compatibility mode.
According to openzipkin/zipkin#939 (comment)
<openzipkin/zipkin#939 (comment)>,
non-string binary annotations aren't really supported.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzmg4cWTMMCIQ26NStKazdWByOGpy2Rks5tqgo9gaJpZM4TTgtw>
.
|
I noticed no field logging was implemented and therefore implemented it using
the same ideas as zipkin-go-opentracing.