uber / tchannel-node

MIT License
203 stars 40 forks source link

introducing openTracer without removing old-style tracing #334

Closed badiib closed 5 years ago

badiib commented 8 years ago

adding a subTracer in the tracer object in channel called openTracer. This means each request needs to have a secondary span object that's called openSpan.

This diff is to define the cut-points for openTracer without removing any parts of the old style tracing.

Still pending:

  1. Proper creation of spans given the head k,v map.
  2. New tests for thrift and json tracing.
yurishkuro commented 8 years ago

Main suggestion is to minimize the exposure outside of the new methods on the channel, i.e. pass request to them, that way the code outside doesn't even need to know about openSpan field.

Raynos commented 8 years ago

The opentracing library still needs to meet our module requirements. It should be authored in ES5 instead of ES6.

We will also need to benchmark performance.

I recommend you land this onto a "tracing" branch instead of "master" branch.

We used to have a tracing benchmark ( https://github.com/uber/tchannel-node/blob/master/benchmarks/Makefile#L46 ) that may be broken on master / need some parameters tweaked, but it should allow us to verify performance. The benchmark has tracing support ( https://github.com/uber/tchannel-node/blob/master/benchmarks/multi_bench.js#L161-L176 ) and will need tweaking to support writing to jeager / opentracing.

CLAassistant commented 5 years ago

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.