Question: Promisified libraries

[luismendozamx]

I've successfully integrated zipkin on some of our services. In most of them we are using Bluebird to promisify the redis.RedisClient and we are using request-promise for external service request, which also uses Bluebird internally.

What is the best approach to wrapping a promisified redisClient and request-promise? Should we use the zipkin-instrumentation-redis and zipkin-instrumentation-request packages and then promisify them? Or should we create our own instrumentation packages that support Promises?

[eirslett]

I believe promisifying code shouldn't have any effect on zipkin instrumentation at all. Does it stop it from working?

[luismendozamx]

I have only tried it for redis @eirslett.

From going through the instrumentation code I'm pretty sure why it doesn't work.

In the case of redis, using Bluebird to promisify it creates new methods with the Async word appended to the method's name. This commands are not taken into account in the 'redis-command' list used to wrap methods in the zipkin-instrumentation-redis package.

When I call an async redis method zipkin throws an error:

/app/my_app/node_modules/zipkin-instrumentation-redis/lib/zipkinClient.js:28
      callback.apply(this, args);
               ^

TypeError: callback.apply is not a function
    at zipkinCallback (/app/my_app/node_modules/zipkin-instrumentation-redis/lib/zipkinClient.js:28:16)
    at Object.callbackOrEmit [as callback_or_emit] (/app/my_app/node_modules/redis/lib/utils.js:89:9)
    at RedisClient.return_error (/app/my_app/node_modules/redis/index.js:701:11)
    at JavascriptRedisParser.returnError (/app/my_app/node_modules/redis/index.js:196:18)
    at JavascriptRedisParser.execute (/app/my_app/node_modules/redis-parser/lib/parser.js:560:12)
    at Socket.<anonymous> (/app/my_app/node_modules/redis/index.js:274:27)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at TCP.onread [as _originalOnread] (net.js:547:20)
    at TCP.onread (/app/my_app/node_modules/async-listener/glue.js:188:31)

For the request-promise module I haven't tried it, but again from browsing through the zipkin-instrumentation-request code I'm pretty sure it won't work.

[eirslett]

It's correct that Bluebird adds promisified methods doXxxAsync() but these methods again call doXxx() which is instrumented by zipkin-instrumentation-redis. Isn't that correct?
If you use the console tracer, you can start your app and see if it logs traces to the console correctly?

[luismendozamx]

@eirslett That's correct for request-promise, I've successfully managed to trace calls using the same wrapper for request. But for redis, it's not working. The stack trace I posted above was from an error using zipkin-instrumentation-redis with a Bluebird promisified node-redis.

[eirslett]

:-(

Just to reproduce, can you add an integration test?
Fork this project and add packages/zipkin-instrumentation-redis/test/bluebird-integrationTest.js similar to the other example, but where the client is Bluebird-promisified.

[luismendozamx]

Sure, I'll add the tests.

[mafrisci]

I also see this zipkin error if a wrapped redis command does not have a callback. Even though the node_redis documentation explicitly says "the API is entirely asynchronous," there is still a check in there to see if a callback exists. Perhaps doing a similar thing here would be helpful to stay consistent with the node_redis api.