Add a wrapper to log spans and span interactions

[vprithvi]

Part of #501

• Add zap fields to encode spans
• Add a wrapped Tracer

Signed-off-by: Prithvi Raj p.r@uber.com

[codecov]

Codecov Report

Merging #502 into master will increase coverage by 0.18%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   88.43%   88.62%   +0.18%     
==========================================
  Files          59       60       +1     
  Lines        3581     3745     +164     
==========================================
+ Hits         3167     3319     +152     
- Misses        303      309       +6     
- Partials      111      117       +6     

Impacted Files	Coverage Δ
log/zap/field.go	87.36% <92.30%> (+22.66%)	⬆️
log/zap/tracer.go	93.02% <93.02%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef8f5ed...af4c167. Read the comment docs.

[yurishkuro]

suggest keeping consistent with the rest and use type span opentracing.Span

Also might consider type-checking for *jaeger.Span in the following function rather than in MarshalLogObject

[vprithvi]

opentracing.Span is an interface. We can typedef it, but go doesn't allow implementing methods on interfaces

[vprithvi]

The approach of declaring a new type, and then converting it to use the old type seems pretty contrived to me. What are the benefits of doing this, as opposed to a plain or embedded struct?

[yurishkuro]

New structs cause memory allocations as they are converted to interface{} when passing through Zap API. stdlib also usually uses type aliases instead of creating wrappers, so you could say it's more idiomatic.

[vprithvi]

ty for the explanation!

[yurishkuro]

I strongly prefer not to introduce mock dependencies into this library. The goal of the logging wrapper is to log, the tests should be checking for log content, rather than mock expectations. Zap provides very good testing facilities, including checking for structured JSON.

[vprithvi]

The tests in this file already verify the log message and in cases where appropriate whether the logger contains the specific log field. e.g., context. They don't look at the contents of the the key because that logic is tested in field_test.go.

I agree that the purpose of the logging wrapper is to log - but it is crucial that it forwards calls and responses to and from the delegate. I think it is important to test this functionality.

I don't see a way of using purely the logs in the loggingWrapper to verify that the delegate methods have been called with the correct arguments. What are you proposing?

[yurishkuro]

ok