Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactors kafkajs tests so that instrumentation errors are easier to see #385

Merged
merged 8 commits into from
Jul 2, 2019

Conversation

codefromthecrypt
Copy link
Member

@codefromthecrypt codefromthecrypt commented Jun 30, 2019

This refactors the tests for kafkajs to use v2 format and fixes some lower hanging problems

  • docker containers didn't cleanup which made debugging mysterious
  • setup was being used same way as client-server, which is incorrect
    • remote service should be the broker, not the guessed name of a server
    • producer and consumer should never share span ID (join) especially because this library does retries for failed message processors!
  • incorrectly read Buffer as a string which broke propagation
  • incorrectly set remote service name where service name goes
  • mislabled span names differently than the calling functions

Notable concerns left out of scope are:

  • consumer span is being labeled on a processor function, which is a modeling glitch. Consumer should represent the time spent getting the message from kafka, which is quite short.
  • docker setup is ok now, but still could be better possibly if managed by mocha somehow instead of shell script

See #384

@codefromthecrypt
Copy link
Member Author

this still needs work to do as there's a bunch of timeout problems in both kafka via docker, and also the sequencing of promises in general. I suspect we should be looking at testcontainers-node.. cc @bsideup in case you know an existing setup

parentId: fromNullable(parentId),
spanId,
sampled: fromNullable(sampled),
traceId: bufferToAscii(traceId),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might consider moving this logic into traceId, if it is common for headers to be of type Buffer, though I think Buffer might be node.js only and so far this is the only user of it.

@codefromthecrypt
Copy link
Member Author

ps the timeouts are not so much a problem of tests not running (they do) it is more about the obfuscation of issues. It isn't as high priority to manage test differently, just it is hard to maintain especially the test that looks for errors to occur.

@@ -5,18 +5,19 @@
"main": "lib/zipkin-instrumentation-kafkajs.js",
"scripts": {
"build": "babel src -d lib",
"pretest": "docker-compose up -d; timeout 60 bash -c 'until echo > /dev/tcp/localhost/9092; do sleep 1; done'",
"test": "mocha --require ../../test/helper.js",
"pretest": "(docker kill kafkajs-test; docker rm kafkajs-test) 2>&-; docker run -d -p 9092:9092 --name kafkajs-test --hostname localhost openzipkin/zipkin-kafka && timeout 60 bash -c 'until echo > /dev/tcp/localhost/9092; do sleep 1; done'",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in local dev, tests would fail because the pre-existing docker containers weren't deleted. the result would be strange as it would appear parsing was busted because the same headers are always read... this was due to replaying whatever the first test message was. now this is fixed, but really we have to switch this to something else!

@bsideup
Copy link

bsideup commented Jul 1, 2019

@adriancole one can run the tests with GraalVM and enjoy the Java version of Testcontainers :D
https://medium.com/graalvm/using-testcontainers-from-a-node-js-application-3aa2273bf3bb

@codefromthecrypt codefromthecrypt changed the title WIP: refactors kafkajs tests so that instrumentation errors are easier to see Refactors kafkajs tests so that instrumentation errors are easier to see Jul 1, 2019
@codefromthecrypt
Copy link
Member Author

OK I rewrote the description. I'm done with this work because there are a lot more modules to refactor and the most important things here are addressed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants