Multithreading issue in ZipkinSpanCollector

[raunak-a]

Hi,

I could see that if in ZipkinSpanCollector, if you have more that one SpanProcessingThread, all the threads are sharing the same instance of ZipkinCollectorClientProvider. Hence there is a serious multithreading issue.

[raunak-a]

Any thoughts on the same?

[kristofa]

Hi,
Yes, that's a bug. Thanks for reporting. I have never used the ZipkinSpanCollector with more than 1 processing thread so the bug never surfaced for me.
Not sure if others are using more then 1 thread here. It feels like if 1 thread for 1 service instance is not able to flush the spans we probably should lower sample rate.

Feel free to submit a pull request.

I might do a request to ask if it would be feasible to remove the support for multiple threads here #59.

[raunak-a]

Hi @kristofa Thanks for your reply. I guess you are right, we might not need to have more than a thread if the sampling rate is correct but as a part of performance testing we might need to use more than a thread to throttle the system.

Let me make a fix for this and will send the pull request.

[raunak-a]

Hi @kristofa ,

I have submitted the pull request for the issue : #64

Please have a look and let me know in case there is any issue with the change.

Would it also be possible for you to release it, so that instead of using the patch in our production, we can use your library directly.

Thanks,
Raunak Agrawal

[kristofa]

I had 1 small remark on the pr. Once that's in place I can merge it.
How urgent is the release? Are you using multiple threads in ZipkinSpanCollector? Did you try using a single thread and did it not work for you?

[raunak-a]

Hi @kristofa,

I have fixed the code according to your comment. Please have a look.

We have been using with single thread and its working perfectly. But as I said earlier, at the time of QA regression and load testing, when we are passing nrOfThreads > 1, it is failing. It would be nice, if you can release, otherwise we would patch the code. No issues 👍

[raunak-a]

Hi @kristofa,

Could you please check the pull request and merge the code?

Thanks

[raunak-a]

Hi @kristofa,

Could you please check the pull request and merge the code?

Thanks

[kristofa]

I merged the pr. It will end up in a release which will be available hopefully by Monday.

[raunak-a]

@kristofa Thanks 👍