Add "STATISTICS" command to zmq_proxy_steerable()

[f18m]

Issue description

Currently there is no visibility on how many packets / bytes a ZMQ proxy has processed.
A way to retrieve such information would be important, at the very least, for debugging purpose.
Attached is the proposed patch to ZMQ master branch to add such feature.

Patch description

On libzmq/src/proxy.cpp the following changes are done:

• the forward() function keeps track of how many packets/bytes are forwarded through the proxy
• the proxy control socket now handles an additional "STATISTICS" command; a reply is sent over the control socket (REP/REQ type should be used in such case for the control socket) with both frontend and backend socket stats.

On the tests_proxy.cpp file:

• small bugfix: when verbose = 1 the control-socket commands were not NUL-terminated. The patch adds NUL-termination that makes stdout more clear
• the control sockets for CLIENTS and SERVER WORKERS are split in 2 sockets; this allows to stop all threads except the proxy; this way the test can be more reliable since we can ensure that the proxy has processed all messages before assert()ing on the number of messages that went through it.
• a "STOP" command is added for CLIENTS and SERVER WORKERS: when the STOP command is received everybody stops doing zmq_send() and just listen on its sockets. In this way the chattering stops and we can reliably assert that the number on the number of processed packets.
• each CLIENT and SERVER WORKER keeps track using zmq atomics of how many requests/replies has sent
• the main() will use the STATISTICS command of the proxy to verify that all messages sent by clients and server workers were correctly processed by the proxy.

What's the expected result?

"make check" should pass all tests.

[f18m]

Attached patch file:

libzmq_proxy_statistics.patch.txt

[bluca]

Don't record the stats unconditionally - add an ON/OFF command (eg: STATS_ON STATS_OFF). Measurements have impact on performance, and if somebody is not interested there is no point in wasting resources.

Also the statistics message, instead of packing everything into a single buffer which makes it very akward toe extract, use one frame per statistics in a multipart msg.

Otherwise looks good, thansk!

[bluca]

Also maybe rather than packets, call them messages in the stats, since packets might be misleading given it's counting messages

[f18m]

Hi Luca, thanks for taking a look at the patch so quickly!

Don't record the stats unconditionally - add an ON/OFF command (eg:

STATS_ON STATS_OFF). Measurements have impact on performance, and if somebody is not interested there is no point in wasting resources. Generally speaking I agree but in this case I think the ON/OFF check itself would be overhead: in practice we would add an "if branch" before doing ++ and += operations. However I think that it's faster to simply do the add operations always rather than doing the "if" check and then skip the add operations. Nowadays ADD ops are almost free in any modern CPU while check-and-jump operations can be dramatic for performance if the CPU branch predictor fails. Moreover the counters should be small enough (64B) to stay in CPU cache at all times.

Also the statistics message, instead of packing everything into a single

buffer which makes it very akward toe extract, use one frame per statistics in a multipart msg. OK

Also maybe rather than packets, call them messages in the stats, since

packets might be misleading given it's counting messages OK sure Francesco 2017-09-05 12:45 GMT+02:00 Luca Boccassi <notifications@github.com>:

…

Also maybe rather than packets, call them messages in the stats, since packets might be misleading given it's counting messages — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2736 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTAcx_8pqMOn33ueUuIDfm-bsU38MhVks5sfSZngaJpZM4PMvD6> .

[bluca]

Generally speaking I agree but in this case I think the ON/OFF check itself
would be overhead: in practice we would add an "if branch" before doing ++
and += operations. However I think that it's faster to simply do the add
operations always rather than doing the "if" check and then skip the add
operations. Nowadays ADD ops are almost free in any modern CPU while
check-and-jump operations can be dramatic for performance if the CPU branch
predictor fails. Moreover the counters should be small enough (64B) to stay
in CPU cache at all times.

Yeah I was thinking more in terms of cache lines, as 4 counters means half a cache line is used up.

But there's no point in worrying about this prematurely, so please go ahead and send a PR. Thanks!

[f18m]

Ok perfect, just a question about github: should I create my own libzmq fork before submitting the pull request? Sorry but I never worked with git that much, I'm mostly a Subversion guy :) Thanks, Francesco 2017-09-05 14:16 GMT+02:00 Luca Boccassi <notifications@github.com>:

…

Generally speaking I agree but in this case I think the ON/OFF check itself would be overhead: in practice we would add an "if branch" before doing ++ and += operations. However I think that it's faster to simply do the add operations always rather than doing the "if" check and then skip the add operations. Nowadays ADD ops are almost free in any modern CPU while check-and-jump operations can be dramatic for performance if the CPU branch predictor fails. Moreover the counters should be small enough (64B) to stay in CPU cache at all times. Yeah I was thinking more in terms of cache lines, as 4 counters means half a cache line is used up. But there's no point in worrying about this prematurely, so please go ahead and send a PR. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2736 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTAc5GvNhsUM4QPqddbbPi6T13N_2shks5sfTu0gaJpZM4PMvD6> .

[bluca]

Yes, click on the "Fork" button at the top right, then clone your fork, create a new branch git checkout -b proxy_stat, change the files and do your commit, then push it when you are happy git push --set-upstream origin proxy_stat and then you'll be able to open a pull request

Please use the following format for commit message:

Problem: cannot do X

Solution: add Y and Z to do X

[sigiesec]

@f18m I see you already forked :) In general, I would recommend that you create a branch in your fork, and don't commit directly on the master branch, but if that is your only work item, it will work this way too.

You might also want to register yourself and your forked project with travis-ci.org and appveyor.com, so that you benefit from CI before creating a PR. Once you registered, builds should run automatically when you push a commit.

[f18m]

Thank you guys, I managed to fork the project, submit my changes there (I'm sorry but I used the master branch of the fork... I hope that's not a big issue) and then open a PR. I'm keeping an eye on the automatic builds on travis-ci.org now. I'm also integrating this new feature in the zmq-based project I'm working now. Let me know if I should be doing something more, THanks Francesco 2017-09-05 15:05 GMT+02:00 Simon Giesecke <notifications@github.com>:

…

@f18m <https://github.com/f18m> I see you already forked :) In general, I would recommend that you create a branch in your fork, and don't commit directly on the master branch, but if that is your only work item, it will work this way too. You might also want to register yourself and your forked project with travis-ci.org and appveyor.com, so that you benefit from CI before creating a PR. Once you registered, builds should run automatically when you push a commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2736 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTAc79ZAZIh53R7I4b5khmcxnEBkFWDks5sfUcVgaJpZM4PMvD6> .

[f18m]

Perhaps I can close this issue now, since the contribution is going through the Pull-Request #2737

[sigiesec]

If you add #2736 to the description of the pull request, this issue will be automatically closed when the PR is merged :)

[bluca]

@f18m I want to do a couple more changes, I can do them if you don't have time.

1. Wrap the code that responds to STATISTICS in #ifdef ZMQ_BUILD_DRAFT_API ... #endif as it's a new API, and we always keep them disabled by default. It's less of a problem since it's not an actual shared object public symbol, but it's still an external API
2. Split again the stats. Instead of returning a struct, which is very awkward and will eventually give problems on embedded devices (alignment, etc etc) have one frame per statistic. This way it's super-easy, especially for high level bindings, to iterate over the message and extract the 64 bit value. No need to redefine a struct and so on.

[f18m]

Hi Luca, 2017-09-05 18:10 GMT+02:00 Luca Boccassi <notifications@github.com>:

@f18m <https://github.com/f18m> I want to do a couple more changes, I can do them if you don't have time. 1. Wrap the code that responds to STATISTICS in #ifdef ZMQ_BUILD_DRAFT_API ... #endif as it's a new API, and we always keep them disabled by default. It's less of a problem since it's not an actual shared object public symbol, but it's still an external API OK 1. Split again the stats. Instead of returning a struct, which is very awkward and will eventually give problems on embedded devices (alignment, etc etc) have one frame per statistic. This way it's super-easy, especially for high level bindings, to iterate over the message and extract the 64 bit value. No need to redefine a struct and so on. So that every uint64_t is in a separate ZMQ message part, right?

I can do that even if from my libzmq user's point of view it seems to make things more verbose... you will need to do 8 zmq_recv() instead of just 1... but never mind. Another last point about this PR: should we update docs for zmq_proxy_steerable() to mention this new command and the format of the output? Thanks, FM

[bluca]

So that every uint64_t is in a separate ZMQ message part, right?
I can do that even if from my libzmq user's point of view it seems to make
things more verbose... you will need to do 8 zmq_recv() instead of just
1... but never mind.

It might seem like that, but this way there's no need to define custom binary structures just to deal with an API.
Think about developers and users of high level bindings - having a message as the basic abstraction makes their life very easy. Having an arbitrary length binary buffer that has to be manually chopped up to find out the information needed makes it a bit harder.

Another last point about this PR: should we update docs for
zmq_proxy_steerable() to mention this new command and the format of the
output?

Yes, once it's finalised we should.

[bluca]

I took care of the DRAFT change, and of a 32bit build failure due to printing u64 (it's a pain to do that portably...)

[bluca]

@f18m have a look at #2740

[f18m]

I looked at the changes and they seem good to me, thanks! I will pull the latest GIT and try it in my project! Thanks, Francesco 2017-09-06 2:49 GMT+02:00 Luca Boccassi <notifications@github.com>:

…

@f18m <https://github.com/f18m> have a look at #2740 <#2740> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2736 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTAc4SowZ56Q1RDbghqZeahB4F9imCFks5sfewugaJpZM4PMvD6> .

[bluca]

Great! As the API is in DRAFT state, if you have any improvements that change the behaviour feel free to send PRs.

[bluca]

@f18m any news on your tests?

[f18m]

Hi Luca, sorry for the very long delay. I managed to actually grab the latest version and test it in my software only today. It works fine regarding PROXY stats (I have some other issues but they seem to be totally unrelated - I may write an email later to the mailing list). Thanks! Francesco 2017-09-13 19:56 GMT+02:00 Luca Boccassi <notifications@github.com>:

…

@f18m <https://github.com/f18m> any news on your tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2736 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJTAc8JTJHs4UYM_XIz4P9dDSoqQlRQMks5siBdGgaJpZM4PMvD6> .

[bluca]

Great, thanks