nodejs solution of opentracing tutorial

[junminstorage]

@TODO: README file per lesson has minimum information may need to be completed, but python solutions have complete README file which can be referred to.

[junminstorage]

Just saw your comment. I will make changes based on the comment. One minor comment is that it seems to be strange to use GET for two actions type requests, maybe POST is more appropriate.

I need to dig into jaegar client a little bit to see the issue you mentioned in the question. I feel starting a server is more closer to the real usage for OpenTracing. If you just want it to be a script, how about ditching express and writing it as nodejs script instead?

But to be consistent with other solutions I will align the nodejs code as closer to others as possible.

I will get back to you on both tomorrow.

[junminstorage]

Sorry, too busy with daily work this week since returning from the conference, also was busy with organizing the conference notes so I can prepare for give a talk on velocity.

I took a deep dive to the span.finish(), it invokes this._tracer._report(), then a list of reporters will be used to report(span), one of them, the remote_reporter uses the udp_sender to send out thrift data, it does appending until the buffer is over limit of UDP_PACKET_MAX_LENGTH, then it will flush(), if I set limit lower or remove the buffer size checking, I can see the buffer being flushed.

So it may sound like a bug in the client library. But I don't know why this bug doesn't exist for express app.

To solve this problem if true, we might need to listen to node.js process exit event to auto flush the buffer or expose a method on tracer for app to flush out the any remain bytes.

I am working on solutions this weekend to make it align with python solution.

[yurishkuro]

we might need to listen to node.js process exit event to auto flush the buffer or expose a method on tracer for app to flush out the any remain bytes.

I think hooking into the process exit will be a very useful feature regardless. But for a simple js script I was still calling tracer.close(), but one thing that is missing for sure is the support for a callback in the tracer.close -> reporter.close -> sender.flush chain, i.e. sender.flush does not accept a callback, it always fires socket.send asynchronously, and it seems like on the very first call it might take some time. I confirmed it when I was experimenting with a simple js script, I logged to console just before socket.send was invoked, but its callback was never invoked prior to the app exiting.

[junminstorage]

I made couple changes:

1. bring the nodejs code closer to python solution
2. add process exit event handler to the lib/tracer.js so when nodejs process such as express exits, we can close tracer
3. add 12s wait time in hello.js to make sure the UDP message is sent out.

Per nodejs doc:

"The only way to know for sure that the datagram has been sent is by using a callback. If an error occurs and a callback is given, the error will be passed as the first argument to the callback. If a callback is not given, the error is emitted as an 'error' event on the socket object."

So there are a few ways to solve this flush issue:

1. add timeout like in hello.js, not a decent solution, but if we set the timeout interval long enough, it seems to work for me. Simple but ad hoc.

2. change the udp_sender.flush() fn to emit a tracer event like 'UDP_DELIVERED', this allow us to attach listener and callback to it. decent and require library code change.

[yurishkuro]

change the udp_sender.flush() fn to emit a tracer event like 'UDP_DELIVERED'

I booked an issue (#157) about flush() method supporting callback, it might be better than implementing events in the tracer (we don't do that anywhere).

[junminstorage]

The reason I like to emitting events is because it makes code loose-coupled and allow sender to report back its status so in app we know when to close tracer.

tracer.on('UDP_FLUSHED', ()=>{tracer.close();})

I read the issue #157, i guess you want to do something like below in remote_reporter?

        value: function close(callback) {
            console.log('close at remote_reporter ....')
            clearInterval(this._intervalHandle);

            let compositeCb = function () {
                   this._sender.close();
                    if (callback) {
                        callback();
                     }
            }
            this._sender.flush(compositeCb);
        }

I found tracing callbacks is hard, especially callback inside callback e.g. https://github.com/jaegertracing/jaeger-client-node/blob/master/src/tracer.js#L321

Another way is to promisify some of the async functions like flush(), I found short promise chains more readable and easy to reason than callbacks.

        value: function close(callback) {
            console.log('close at remote_reporter ....')
            clearInterval(this._intervalHandle);

            this._sender.flush()
             .then(()=> {this._sender.close();})
             .then(()=>{if(callback) callback();})
        }

We can even promisify the remote_reporter.close() method, in that way the last then() can be removed to make the close() fn clean and simple.

This may be a big change to ask. Just being curious any reasons you guys don't use promise at all in the client library?

[junminstorage]

Anyway, just tested out your callback idea and also made the below change to remote_reporter.close() fn, tracer.close() without using setTimeout() will flush out the last buffer.

    close(callback: ?Function): void {
        clearInterval(this._intervalHandle);
        let cb = this._sender.close.bind(this._sender);
        this._sender.flush(cb);

        if (callback) {
            callback();
        }
    }

If it is what you ask for and without any side effects, I might do a PR on the library.

[yurishkuro]

Thanks for looking into this. I will take a look at the rest of your changes and maybe merge it anyway, with the timeout for now, the callback improvement can be done separately.

Regarding the code snippet, I was actually thinking of something like this:

    close(callback: ?Function): void {
        clearInterval(this._intervalHandle);
        this._sender.flush(function flushCallback() {
            this._sender.close();
            if (callback) {
                callback();
            }
        });
    }

It's similar to your snippet, but yours still calls the flush() async, while this version doesn't invoke the top level callback until we know we flushed the buffer.

[junminstorage]

that sounds great! one minor improvement in your snippet is to bind the this inside the flushCallback with this._sender or use arrow function, anyway i got your idea.

[junminstorage]

@yurishkuro , there is a user asked about this nodejs solution today. do you think it can be merged as we agree? Let me know if there are anything missing.

[yurishkuro]

@junminstorage the code solutions look ok to merge. The READMEs are unfinished and have some stray code-block beginning lines. I left a couple of minor comments, let's apply them and we can merge. Also, can you put some warnings in the individual READMEs at the top saying "This README is currently incomplete / unfinished. Please refer to respective README in tutorials for one of the other languages".

[junminstorage]

@yurishkuro I made the changes based on your review comments. Please take a look. Thanks

[yurishkuro]

@junminstorage great, thank you very much for finishing it up!

[junminstorage]

I will give a talk to the engineering department here at Bloomberg tomorrow I will refer to this github project and other resources related to OpenTracing in github and OpenTracing.io as well.

[yurishkuro]

the HotROD demo https://medium.com/opentracing/take-opentracing-for-a-hotrod-ride-f6e3141f7941 is quite useful for general purpose talks.

I had one of my talks recorded https://blog.openshift.com/openshift-commons-briefing-82-distributed-tracing-with-jaeger-prometheus-on-kubernetes/

[junminstorage]

Thanks! very interesting demo on openshift.com, I will refer to the video in my slides.