Possible bug with adding up message counts?

[aliostad]

This code looks like a bug to me but not sure the intended purpose hence asking:

countSinceCheckpoint += buffer.size();

https://github.com/openzipkin/zipkin-azure/blob/master/collector/eventhub/src/main/java/zipkin/collector/eventhub/ZipkinEventProcessor.java#L70

So let's say we have 5 spans in first message and 6 in the second, my countSinceCheckpoint will be 16 (5+11) rather than 11. I think just comparing with buffer.size() should be enough.

[codefromthecrypt]

looks sensible. want to add a test and fix? https://github.com/openzipkin/zipkin-azure/blob/master/collector/eventhub/src/test/java/zipkin/collector/eventhub/ZipkinEventProcessorTest.java#L72

[codefromthecrypt]

ps I have work in progress on this. The SDK is a bit of a bear to work with as it is signed, etc, which makes it hard to mock certain things.

[codefromthecrypt]

#17

[codefromthecrypt]

cutting as 0.1.2 since I presume not checkpointing properly is a fairly major bug!

@aliostad thanks for discovering this

[aliostad]

I am sorry I have to open this issue again, since the problem is closely related to the same code.

It is not really our fault (I believe MS samples are wrong too) but basically the host creates a single IEventProcessor (in our case ZipkinEventProcessor) but the onEvents gets called per each partition, on different threads. Hence keeping a single count for ZipkinEventProcessor does not cut it.

Let me raise this with Microsoft before making any changes but my understanding is to keep a thread-safe dictionary of counts to solve.

[codefromthecrypt]

k sounds sensible, albeit possibly tricky. if it ends up that way, we'd need to know if partitions come and go a lot. I thought I saw something about only 4 partitions in some thread someplace. Anything that helps fix the keys can help us avoid a memory leak when mapping on partition. If you can, please ping MS to make a test version of their service as this sort of issue is a good example of where flying blind can hurt. anyway, thanks for raising this and keep us posted!