Feature: expose librdkafka statistics as swift metrics #92

blindspotbounty · 2023-07-18T14:51:50Z

This is approximate solution for exposing librdkafka statistics callback (#79)

Main ideas:

Specify interval for metrics updates
Allow to specify metrics per librdkafka key

felixschlegel

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!

Sources/SwiftKafka/KafkaClient.swift

Tests/SwiftKafkaTests/KafkaProducerTests.swift

blindspotbounty · 2023-07-20T13:34:40Z

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 · 2023-07-20T13:44:16Z

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 · 2023-07-20T15:03:02Z

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.

Tests/SwiftKafkaTests/KafkaConsumerTests.swift

felixschlegel · 2023-07-20T15:33:36Z

Yes it is going into the right direction!

Sources/SwiftKafka/Utilities/KafkaStatistics.swift

…expose-librdkafka-statistics

Sources/SwiftKafka/Configuration/KafkaConfiguration.swift

Sources/SwiftKafka/Configuration/KafkaConsumerConfiguration.swift

Sources/SwiftKafka/Configuration/KafkaProducerConfiguration.swift

Sources/SwiftKafka/KafkaConsumer.swift

Sources/SwiftKafka/KafkaProducer.swift

Tests/SwiftKafkaTests/KafkaConsumerTests.swift

Tests/SwiftKafkaTests/KafkaProducerTests.swift

Sources/SwiftKafka/KafkaConsumerEvent.swift

Sources/SwiftKafka/Utilities/KafkaStatistics.swift

Sources/SwiftKafka/Utilities/KafkaStatisticsJsonModel.swift

…expose-librdkafka-statistics

felixschlegel

Great work! Please see my comments 😄

Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

Sources/Kafka/Configuration/KafkaConfiguration.swift

Sources/Kafka/Configuration/KafkaConsumerConfiguration.swift

Sources/Kafka/KafkaProducer.swift

Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

Sources/Kafka/KafkaConsumer.swift

Sources/Kafka/KafkaProducer.swift

Tests/KafkaTests/Utilities.swift

Co-authored-by: Felix Schlegel <[email protected]>

felixschlegel

Hey @blindspotbounty,

Again, excellent work!
I have left some minor comments inline and also a comment regarding how we are "filling in" the statistics.

-Felix

Tests/KafkaTests/KafkaConsumerTests.swift

Tests/KafkaTests/KafkaProducerTests.swift

Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

Sources/Kafka/KafkaConsumer.swift

Sources/Kafka/Configuration/KafkaConsumerConfiguration.swift

Sources/Kafka/Utilities/KafkaStatistics.swift

Sources/Kafka/Utilities/KafkaStatisticsJsonModel.swift

Sources/Kafka/Utilities/KafkaStatistics.swift

…stics

FranzBusch

Overall looks good and thanks for the work. I left some comments inline

Package.swift

Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

Sources/Kafka/RDKafka/RDKafkaStatistics.swift

Tests/KafkaTests/KafkaConsumerTests.swift

FranzBusch · 2023-10-04T13:47:02Z

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

blindspotbounty · 2023-10-10T11:46:11Z

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

Tests/KafkaTests/Utilities.swift

FranzBusch

I think we are really close! Thanks for all the work on this one last comment

Sources/Kafka/RDKafka/RDKafkaStatistics.swift

Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

…stics

FranzBusch

Looks really good! Thanks for doing the work.

FranzBusch · 2023-10-31T17:36:25Z

@swift-server-bot add to allowlist

FranzBusch · 2023-10-31T17:36:31Z

@swift-ci please test

FranzBusch · 2023-11-01T12:52:10Z

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

…stics

blindspotbounty · 2023-11-03T08:53:02Z

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

FranzBusch · 2023-11-03T09:06:35Z

@swift-ci please test

blindspotbounty · 2023-11-03T09:16:47Z

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'

#139 (comment)

* Feature: expose librdkafka statistics as swift metrics (swift-server#92) * introduce statistics for producer * add statistics to new consumer with events * fix some artefacts * adjust to KeyRefreshAttempts * draft: statistics with metrics * make structures internal * Update Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift Co-authored-by: Felix Schlegel <[email protected]> * Update Sources/Kafka/Configuration/KafkaConsumerConfiguration.swift Co-authored-by: Felix Schlegel <[email protected]> * Update Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift Co-authored-by: Felix Schlegel <[email protected]> * Update Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift Co-authored-by: Felix Schlegel <[email protected]> * address review comments * formatting * map gauges in one place * move json mode as rd kafka statistics, misc renaming + docc * address review comments * remove import Metrics * divide producer/consumer configuration * apply swiftformat * fix code after conflicts * fix formatting --------- Co-authored-by: Felix Schlegel <[email protected]> * Add benchmark infratructure without actual tests (swift-server#146) * add benchmark infratructure without actual test * apply swiftformat * fix header in sh file * use new async seq methods * Update to latest librdkafka & add a define for RAND_priv_bytes (swift-server#148) Co-authored-by: Franz Busch <[email protected]> * exit from consumer batch loop when no more messages left (swift-server#153) * Lower requirements for consumer state machine (swift-server#154) * lower requirements for kafka consumer * add twin test for kafka producer * defer source.finish (swift-server#157) * Add two consumer benchmark (swift-server#149) * benchmark for consumer * attempty to speedup benchmarks * check CI works for one test * enable one more test * try to lower poll interval * adjust max duration of test * remain only manual commit test * check if commit is the reason for test delays * try all with schedule commit * revert max test time to 5 seconds * dockerfiles * test set threasholds * create dummy thresholds from ci results * disable benchmark in CI * add header * add stable metrics * update thresholds to stable metrics only * try use '1' instead of 'true' * adjust thresholds to CI results (as temporary measure) * set 20% threshold.. * move arc to unstable metrics * try use 'true' in quotes for CI * try reduce number of messages for more reliable results * try upgrade bench * disable benchmark in CI * Update librdkafka for BoringSSL (swift-server#162) * chore(patch): [sc-8379] use returned error (swift-server#163) * [producer message] Allow optional key for initializer (swift-server#164) Co-authored-by: Harish Yerra <[email protected]> * Allow groupID to be specified when assigning partition (swift-server#161) * Allow groupID to be specified when assigning partition Motivation: A Consumer Group can provide a lot of benefits even if the dynamic loadbalancing features are not used. Modifications: Allow for an optional GroupID when creating a partition consumer. Result: Consumer Groups can now be used when manual assignment is used. * fix format --------- Co-authored-by: Ómar Kjartan Yasin <[email protected]> Co-authored-by: blindspotbounty <[email protected]> Co-authored-by: Franz Busch <[email protected]> * Wrap rd_kafka_consumer_poll into iterator (use librdkafka embedded backpressure) (swift-server#158) * remove message sequence * test consumer with implicit rebalance * misc + format * remove artefact * don't check a lot of messages * fix typo * slow down first consumer to lower message to fit CI timeout * remove helpers * use exact benchmark version to avoid missing thresholds error (as no thresholds so far) * add deprecated marks for backpressure, change comment for future dev * address comments --------- Co-authored-by: Felix Schlegel <[email protected]> Co-authored-by: Axel Andersson <[email protected]> Co-authored-by: Franz Busch <[email protected]> Co-authored-by: Samuel M <[email protected]> Co-authored-by: Harish Yerra <[email protected]> Co-authored-by: Harish Yerra <[email protected]> Co-authored-by: Omar Yasin <[email protected]> Co-authored-by: Ómar Kjartan Yasin <[email protected]>

blindspotbounty marked this pull request as ready for review July 19, 2023 07:08

felixschlegel suggested changes Jul 19, 2023

View reviewed changes

Sources/SwiftKafka/KafkaClient.swift Outdated Show resolved Hide resolved

Tests/SwiftKafkaTests/KafkaProducerTests.swift Outdated Show resolved Hide resolved

blindspotbounty marked this pull request as draft July 20, 2023 14:58

felixschlegel reviewed Jul 20, 2023

View reviewed changes

Tests/SwiftKafkaTests/KafkaConsumerTests.swift Outdated Show resolved Hide resolved

blindspotbounty commented Jul 20, 2023

View reviewed changes

Sources/SwiftKafka/Utilities/KafkaStatistics.swift Outdated Show resolved Hide resolved

blindspotbounty mentioned this pull request Jul 24, 2023

Feature/sc 1976/gsoc expose librdkafka statistics ordo-one/swift-kafka-client#1

Merged

introduce statistics for producer

c870864

blindspotbounty force-pushed the feature/sc-1976/gsoc-expose-librdkafka-statistics branch from 2f2d828 to c870864 Compare July 26, 2023 15:37

blindspotbounty added 2 commits July 31, 2023 17:48

Merge remote-tracking branch 'origin/main' into feature/sc-1976/gsoc-…

633773f

…expose-librdkafka-statistics

add statistics to new consumer with events

d9887b9

blindspotbounty requested a review from felixschlegel August 2, 2023 08:54

felixschlegel suggested changes Aug 8, 2023

View reviewed changes

blindspotbounty added 5 commits August 10, 2023 16:08

Merge remote-tracking branch 'origin/main' into feature/sc-1976/gsoc-…

e5f0483

…expose-librdkafka-statistics

fix some artefacts

d55a7fd

adjust to KeyRefreshAttempts

8b4525b

draft: statistics with metrics

23e08fc

make structures internal

612a3c4

felixschlegel suggested changes Aug 18, 2023

View reviewed changes

blindspotbounty and others added 7 commits August 21, 2023 11:00

Update Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

5c10435

Co-authored-by: Felix Schlegel <[email protected]>

Update Sources/Kafka/Configuration/KafkaConsumerConfiguration.swift

2be2bd9

Co-authored-by: Felix Schlegel <[email protected]>

Update Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

2cd0f8b

Co-authored-by: Felix Schlegel <[email protected]>

Update Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift

abd97de

Co-authored-by: Felix Schlegel <[email protected]>

address review comments

15284e1

formatting

a4ee678

map gauges in one place

0a0f1b8

blindspotbounty closed this Aug 29, 2023

blindspotbounty changed the title ~~Feature: expose librdkafka statistics~~ Feature: expose librdkafka statistics as swift metrics Aug 29, 2023

felixschlegel suggested changes Aug 29, 2023

View reviewed changes

blindspotbounty added 2 commits August 30, 2023 11:03

move json mode as rd kafka statistics, misc renaming + docc

5448eb4

Merge branch 'main' into feature/sc-1976/gsoc-expose-librdkafka-stati…

900cb38

…stics

FranzBusch requested changes Sep 21, 2023

View reviewed changes

address review comments

a83c970

felixschlegel reviewed Oct 10, 2023

View reviewed changes

Tests/KafkaTests/Utilities.swift Outdated Show resolved Hide resolved

remove import Metrics

4ebdf9d

FranzBusch reviewed Oct 12, 2023

View reviewed changes

Sources/Kafka/RDKafka/RDKafkaStatistics.swift Outdated Show resolved Hide resolved

Sources/Kafka/Configuration/KafkaConfiguration+Metrics.swift Outdated Show resolved Hide resolved

blindspotbounty added 3 commits October 20, 2023 17:06

divide producer/consumer configuration

05cf1b9

apply swiftformat

3febfcd

Merge branch 'main' into feature/sc-1976/gsoc-expose-librdkafka-stati…

455be80

…stics

blindspotbounty requested review from FranzBusch and felixschlegel October 25, 2023 13:57

FranzBusch approved these changes Oct 31, 2023

View reviewed changes

FranzBusch enabled auto-merge (squash) October 31, 2023 17:36

Merge branch 'main' into feature/sc-1976/gsoc-expose-librdkafka-stati…

a96edf7

…stics

auto-merge was automatically disabled November 3, 2023 08:51
Head branch was pushed to by a user without write access

fix code after conflicts

af05f5b

fix formatting

8a3caf3

FranzBusch merged commit c3bbb75 into swift-server:main Nov 6, 2023
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: expose librdkafka statistics as swift metrics #92

Feature: expose librdkafka statistics as swift metrics #92

blindspotbounty commented Jul 18, 2023 •

edited

Loading

felixschlegel left a comment

blindspotbounty commented Jul 20, 2023

felixschlegel commented Jul 20, 2023

blindspotbounty commented Jul 20, 2023

felixschlegel commented Jul 20, 2023

felixschlegel left a comment

felixschlegel left a comment •

edited

Loading

FranzBusch left a comment

FranzBusch commented Oct 4, 2023

blindspotbounty commented Oct 10, 2023

FranzBusch left a comment

FranzBusch left a comment

FranzBusch commented Oct 31, 2023

FranzBusch commented Oct 31, 2023

FranzBusch commented Nov 1, 2023

blindspotbounty commented Nov 3, 2023

FranzBusch commented Nov 3, 2023

blindspotbounty commented Nov 3, 2023

Feature: expose librdkafka statistics as swift metrics #92

Feature: expose librdkafka statistics as swift metrics #92

Conversation

blindspotbounty commented Jul 18, 2023 • edited Loading

felixschlegel left a comment

Choose a reason for hiding this comment

blindspotbounty commented Jul 20, 2023

felixschlegel commented Jul 20, 2023

blindspotbounty commented Jul 20, 2023

felixschlegel commented Jul 20, 2023

felixschlegel left a comment

Choose a reason for hiding this comment

felixschlegel left a comment • edited Loading

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment

FranzBusch commented Oct 4, 2023

blindspotbounty commented Oct 10, 2023

FranzBusch left a comment

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment

FranzBusch commented Oct 31, 2023

FranzBusch commented Oct 31, 2023

FranzBusch commented Nov 1, 2023

blindspotbounty commented Nov 3, 2023

FranzBusch commented Nov 3, 2023

blindspotbounty commented Nov 3, 2023

blindspotbounty commented Jul 18, 2023 •

edited

Loading

felixschlegel left a comment •

edited

Loading