Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConsumeCompleted -> ConsumeCompletedIn #9

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

rukai
Copy link
Member

@rukai rukai commented Sep 18, 2024

Previously we allowed for measuring the throughput of consumption but not the latency.
This was because it wasnt clear to me how to define latency for consumption.

I am comparing the results of windsock benchmarks against the results of the https://github.com/confluentinc/librdkafka/blob/master/examples/rdkafka_performance.c tool which another team uses for measuring shotover + kafka performance.
I investigated how rdkafka_performance measures latency of consumption:
It stores a timestamp in the record payload at produce time and then compares the payload against the current time at consume time.
This approach makes a lot of sense and we should enable use of this approach in windsock.

This PR adds infrastructure to windsock for measuring consumption latency.
The logic for measuring consumption latency mirrors that of producing latency with one exception:
The durations passed to windsock in Report::ConsumeCompletedIn are an Option<Duration> instead of just Duration. This allows a None to be passed in the case where the benchmark is testing a payload size so small that it cannot fit a timestamp.

Since consumer latency can be None, I took the opportunity to allow the intermediate layers of the producer latency logic to store latency as None. This is used when no messages were sent during the bench, in this case it will now display the latency as N/A. Its a rare edge case but still worth handling nicely.

windsock codebase

A quick walkthrough of the relevant bits of the windsock codebase:

  • The bench implementation sends instances of the Report enum to windsock to keep it up to date with how the bench is going. In this PR, in the report.rs file, we replace Report::ConsumeCompleted with Report::ConsumeCompletedIn changing the way the user interacts with windsock to allow them to report consumer latency.
  • There is also some logic for processing of Reports within windsock itself living within report.rs. This PR modifies this so that consumer latency is processed and stored in the final report.
  • tables.rs takes a windsocks final reports and displays them as ascii tables in the console. In this PR we modify the table to include rows for consumer latency.

New table format

This is what the table output looks like now with consume latency:

image

@rukai rukai force-pushed the consume_completed_in branch from 6e089c4 to 41b514a Compare September 18, 2024 23:29
@rukai rukai force-pushed the consume_completed_in branch from 41b514a to f6d929a Compare September 19, 2024 00:17
@rukai rukai force-pushed the consume_completed_in branch from f6d929a to f803736 Compare September 19, 2024 00:45
@rukai rukai merged commit 1181bea into shotover:main Sep 19, 2024
34 checks passed
@rukai rukai mentioned this pull request Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants