swift-server / swift-kafka-client

Apache License 2.0
83 stars 24 forks source link

Feature: expose librdkafka statistics as swift metrics #92

Closed blindspotbounty closed 1 year ago

blindspotbounty commented 1 year ago

This is approximate solution for exposing librdkafka statistics callback (https://github.com/swift-server/swift-kafka-gsoc/issues/79)

Main ideas:

  1. Specify interval for metrics updates
  2. Allow to specify metrics per librdkafka key
blindspotbounty commented 1 year ago

Generally a good start! However, I think we should not have a statistics closure and all these new initializers, but rather an AsyncSequence as we do with KafkaConsumer.messages or the producer acknowledgements. The implementation of statistics should therefore follow a similar pattern. Feel free to reach out to me if you need any more pointers for that!

Thank you for the feedback. I think I got you. Basically, we add type to KafkaEvent, e.g. statistics and use it across the code. Let me try it out, it sounds promising regarding how it should look like.

felixschlegel commented 1 year ago

Generally a good start! However, I think we should not have a statistics closure and all these new initializers, but rather an AsyncSequence as we do with KafkaConsumer.messages or the producer acknowledgements. The implementation of statistics should therefore follow a similar pattern. Feel free to reach out to me if you need any more pointers for that!

Thank you for the feedback. I think I got you. Basically, we add type to KafkaEvent, e.g. statistics and use it across the code. Let me try it out, it sounds promising regarding how it should look like.

Yes, so you would probably want to add that to KafkaEvent, receive the statistics event through KafkaClient.eventPoll and then yield the statistics to some new AsyncSequence that is exposed by KafkaConsumer + KafkaProducer.

blindspotbounty commented 1 year ago

Generally a good start! However, I think we should not have a statistics closure and all these new initializers, but rather an AsyncSequence as we do with KafkaConsumer.messages or the producer acknowledgements. The implementation of statistics should therefore follow a similar pattern. Feel free to reach out to me if you need any more pointers for that!

Thank you for the feedback. I think I got you. Basically, we add type to KafkaEvent, e.g. statistics and use it across the code. Let me try it out, it sounds promising regarding how it should look like.

Yes, so you would probably want to add that to KafkaEvent, receive the statistics event through KafkaClient.eventPoll and then yield the statistics to some new AsyncSequence that is exposed by KafkaConsumer + KafkaProducer.

I pushed a draft for Consumer (not touching producer so far), just to be sure that I got the idea correctly. If it is okay, I will perform other cleanup in code and put the same code for producer as well.

felixschlegel commented 1 year ago

Yes it is going into the right direction!

FranzBusch commented 1 year ago

@blindspotbounty If you are ready and can push up the changes addressing the review, I am happy to get this over the line

blindspotbounty commented 1 year ago

@FranzBusch I've pushed commit with addressing conversations. But remained two conversations open in case it should be different.

FranzBusch commented 1 year ago

@swift-server-bot add to allowlist

FranzBusch commented 1 year ago

@swift-ci please test

FranzBusch commented 1 year ago

@blindspotbounty can you take a look at the CI failures?

blindspotbounty commented 1 year ago

@FranzBusch sure. But I think it is better to restart ci after resolving conflicts.

FranzBusch commented 1 year ago

@swift-ci please test

blindspotbounty commented 1 year ago

Seems, it is the same as for PR for backpressure:

11:12:15 /code/Sources/Kafka/RDKafka/RDKafkaClient.swift:33:17: error: stored property 'kafkaHandle' of 'Sendable'-conforming class 'RDKafkaClient' has non-sendable type 'OpaquePointer'

https://github.com/swift-server/swift-kafka-client/pull/139#issuecomment-1772685064