search

tryfer / node-tryfer

A Node Zipkin Tracer Library.
Apache License 2.0
45 stars 27 forks source link

Added test and additional fix for https://github.com/racker/node-tryfer/pull/16 #17

Closed cyli closed 10 years ago

cyli commented 10 years ago

Added a test to verify https://github.com/racker/node-tryfer/pull/16 and make sure no regressions happen. Discovered an additional bug that may cause problems, namely that scribe.Scribe.send is not an asynchronous function - it does not take a callback.

async.waterfall requires that each function accept a callback, and cannot move on to the next task (or finish) if not callbacked.

So wrapped the call to scribe.Scribe.send in a function that takes a callback, and calls it immediately after send. From looking at the scribe code it doesn't seem like it can error? And it should return immediately.

wadey commented 10 years ago

Good catch, and the fix looks good. I wonder if it would be better to just get rid of the async.waterfall completely. Since there is only one actual asynchronous function call now, it seems like just extra complication at this point.

cyli commented 10 years ago

Sure, that makes sense, thanks.

cyli commented 10 years ago

@wadey Going to merge, tag, and npm publish if this looks ok to you! Thank you again for your contribution and review!

wadey commented 10 years ago

Looks good to me. :+1: Thanks for node-tryfer and your continued support.