Fix readme, add double-checked locking on SpanCollector

[klettier]

As SpanCollector.GetInstance can be called by multiple threads at the same time I think it's more safe to add double-checked locking in singleton creation expression.

[klettier]

I want to discuss about some modifications I would like to add, could you please enable Issues on your repository to allow us to share ?

Thx

[cabbott]

@klettier issues enabled!

[jcarres-mdsol]

@bvillanueva-mdsol @lschreck-mdsol @kenyamat お願いします 🙏

[bvillanueva-mdsol]

@klettier I think this looks good. I thought also the same with what @jcarres-mdsol commented, but I realized it is a good a double check thread-safe style for singleton. Talked with @lschreck-mdsol before about avoiding the use of lock() and use lazy instantiation instead. That would be preferable but the parameters of SpanCollector is not from config or any static property but supplied using the ZipkinClient constructor parameters. @lschreck-mdsol Do you have any idea on another way?

@klettier Please update branch from main develop.

Thanks!

[klettier]

Hi @bvillanueva-mdsol,

For me, it's not the responsability of SpanCollector to maintain a singleton instance of itself. The caller (ZipkinClient) has to have that responsiility because he makes the choice of passing the constructor instance or to give the default one. Don't be afraid by the lock because it will occur only one time so it's not going to have performance impact the important thing here is to protect your instance from being call by multiple threads at the same time.

[bvillanueva-mdsol]

@klettier I agree that ZipkinClient should have the responsibility to take are of the Singleton instance. I like the the helper method you made. Please check out my comments on the code. Thanks!

[klettier]

@bvillanueva-mdsol Thx for your comments I have more ideas on what we can do next so I will work on them and submit some issues soon ;)

[bvillanueva-mdsol]

@klettier This looks good. With this changes, we are planning to run a performance test soon just for verification. Performance test are executed to make sure we don't interfere with main operation. I'll ask for allocation of time for our next sprint on the performance tests. Will keep you posted on schedule if we can secure it as well as the results. Thank you so much!

[bvillanueva-mdsol]

@klettier Unfortunately we have things line up for this sprint and might not be able to touch on the performance test for the next two weeks. I'll get back to you after then. For the mean time, can please merge latest changes from develop in this branch. Thanks!

[thoshikawa-mdsol]

@klettier @mdsol/team-51

I've run performance test on Ampridatvir with this change. All the counters look good, so there is no performance regressions by this change.

Michine resource comparison

Full run to run comparison result is available here

[bvillanueva-mdsol]

Looks good @thoshikawa-mdsol . Thanks for sharing the performance test data.

[klettier]

Looks good, thanks @thoshikawa-mdsol

[bvillanueva-mdsol]

Merging. Thanks @klettier for this PR. :+1: