Logs supplied in Span::FinishWithOptions are silently discarded

[ringerc]

jaegertracing::Span::FinishWithOptions entirely ignores the options.log_records field of opentracing::FinishSpanOptions.

Any logs so passed are discarded.

Not ideal, because the Span::Log method only accepts a std::initializer_list so it's clumsy when you have a dynamic set of keys in your logs.

Will send a pull, this just documents the issue first.

[rnburn]

That logging interface was recently added and hasn't actually been put into an OT release yet.

But I can a release for it if it's something you're waiting on.

[ringerc]

@rnburn Good to know. Useful interface :)

No rush. To make any of this work requires running with my integration branch that currently merges in flush-on-close, save-finish-span-logs and local-dependencies-support topic branches, all pending PRs. So building a recent OT is hardly a hassle on top. @isaachier may have an opinion about the next jaeger cpp-client release that's relevant though.

(The mess with Thrift in #45 means Jaeger cpp-client is in practice confined to deployments that can compile their own local Thrift anyway, either via Hunter or direct local install.)

BTW, I went looking for a Span.Log(std::vector<LogRecord::Field>) first, but there's only the initializer list interface. It might make sense to add this as a compromise between the two: you can supply key/value logs constructed dynamically but don't have to keep track of them yourself until Finish time, you can still just append them to the span directly.

In any case the logs support added in #58 works for me.

[ringerc]

@rnburn (PR #58 can't pass CI until there's a newer OT to fetch, but it's not hurting me any)

[rnburn]

@ringerc -- The current interface was copied over from Go's API.

In the PR, we did discuss another interface like what you're suggesting
opentracing/opentracing-cpp#43 (comment)
but decided to support only the FinishSpanOptions initially to keep things simple.

You might put in an issue to OT-cpp for being able to add them directly to the span. One of the open questions is whether you should also be able to specify the timestamp in such an API.