Fix copy-paste bug in fast_server module

[kellymclaughlin]

The changes for #21 introduced a small copy-paste bug in the fast-server module. The use of fc_collector here should actually be fs_collector.

[kellymclaughlin]

CR is here

[davepacheco]

Thanks for fixing this. Can you characterize the impact? Does this cause servers based on v2.6.0 to crash on startup if they expose metrics? Was there a test that should have caught this?

[kellymclaughlin]

That's correct, the server will crash at startup because of this bug if a metrics collector is defined. I discovered this while working with electric-moray for some buckets demo work. I was testing out some changes to node-fast and the latest master changes got pulled in.

The existing server test very nearly could have caught this. Right here a collector is defined for a test case, but the collector option is not passed in when creating the fast server a few lines later. This change is probably worthwhile, though it does break this test case because the rpcmethod is also converted to a label and an rpcmethod value of the empty string is treated as a null value and is an invalid label value. This can also be easily remedied by supplying a valid, but non-registered rpc method name.

I think I will make these test changes a part of my CR to address this problem.

[kellymclaughlin]

Here is the test output with the changes to tst.server.js I described in my previous comment:

[root@be5401a5-37f3-cd3c-c54f-c5d219a7f453 ~/node-fast]# make test
deps/catest/catest test/tst.client_request.js test/tst.socket_summarize.js test/tst.client_generic.js test/tst.protocol_encoder.js test/tst.protocol_decoder.js test/tst.server.js test/tst.allocator.js
Configuration:
    SRC:                         /root/node-fast
    Output directory:            /var/tmp/catest.806123
    Temp directory:              /var/tmp/catest.806123_tmpfiles
    Keep successful test output: false
    Found 7 test(s) to run

===================================================

Executing test test/tst.client_request.js ... success.
Executing test test/tst.socket_summarize.js ... success.
Executing test test/tst.client_generic.js ... success.
Executing test test/tst.protocol_encoder.js ... success.
Executing test test/tst.protocol_decoder.js ... success.
Executing test test/tst.server.js ... FAILED.
>>> failure details in /var/tmp/catest.806123/failure.0

Executing test test/tst.allocator.js ... success.

===================================================

Results:
        Tests passed:    6/ 7
        Tests failed:    1/ 7

===================================================
Cleaning up output from successful tests ... done.
make: *** [Makefile:43: test] Error 1

Here is the error output from the failed test:

[root@be5401a5-37f3-cd3c-c54f-c5d219a7f453 ~/node-fast]# cat /var/tmp/catest.806123/failure.0/806123.err
test case: basic RPC: no data

/root/node-fast/lib/fast_server.js:230
                if (this.fc_collector.FIXED_BUCKETS === true) {
                                     ^
TypeError: Cannot read property 'FIXED_BUCKETS' of undefined
    at new FastServer (/root/node-fast/lib/fast_server.js:230:24)
    at runTestCase (/root/node-fast/test/tst.server.js:170:19)
    at Array.0 (/root/node-fast/node_modules/vasync/lib/vasync.js:246:12)
    at pipeline (/root/node-fast/node_modules/vasync/lib/vasync.js:219:10)
    at Object.forEachPipeline (/root/node-fast/node_modules/vasync/lib/vasync.js:250:13)
    at main (/root/node-fast/test/tst.server.js:42:13)
    at Object.<anonymous> (/root/node-fast/test/tst.server.js:1192:1)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)

Now here is the test output with the fix from the CR in place and the test changes:

[root@be5401a5-37f3-cd3c-c54f-c5d219a7f453 ~/node-fast]# make test
deps/catest/catest test/tst.client_request.js test/tst.socket_summarize.js test/tst.client_generic.js test/tst.protocol_encoder.js test/tst.protocol_decoder.js test/tst.server.js test/tst.allocator.js
Configuration:
    SRC:                         /root/node-fast
    Output directory:            /var/tmp/catest.721225
    Temp directory:              /var/tmp/catest.721225_tmpfiles
    Keep successful test output: false
    Found 7 test(s) to run

===================================================

Executing test test/tst.client_request.js ... success.
Executing test test/tst.socket_summarize.js ... success.
Executing test test/tst.client_generic.js ... success.
Executing test test/tst.protocol_encoder.js ... success.
Executing test test/tst.protocol_decoder.js ... success.
Executing test test/tst.server.js ... success.
Executing test test/tst.allocator.js ... success.

===================================================

Results:
        Tests passed:    7/ 7
        Tests failed:    0/ 7

===================================================
Cleaning up output from successful tests ... done.
Note: Compatibility tests need to be run manually with "make test-compat".

[davepacheco]

Thanks for adding that!