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

fix shotover_chain_messages_per_batch_count metric #1633

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

rukai
Copy link
Member

@rukai rukai commented May 21, 2024

Change 1

Every time shotover processes a transform chain, multiple requests and responses are batched and processed together.
This can be seen by the ChainState, which is passed through each transform in the chain, containing multiple requests.
This batching is purely opportunistic, we don't intentionally wait for requests or responses but if there are multiple requests or responses pending at the time we go to process them then they will be all processed in a single batch.
Lets call this batch a "chain batch".

Shotover has an existing metric shotover_chain_messages_per_batch_count which measures the number of requests included in a chain batch.
At the time this metric was written the number of responses returned by a transform chain call was guaranteed to be the number of requests in the request batch.
However, for performance reasons, this guarantee was removed and now any number of responses can be returned in a chain call.
This internal change has altered the meaning of this metric, leaving it kind of broken:

  • The metric measures the size of request batches, but from the messages in the name its ambiguous if its measuring the batch size of requests or responses.
  • There is no way to measure the batch size of responses.

So the existing shotover_chain_messages_per_batch_count metric should be split into 2 separate metrics one for requests and one for responses.

TL;DR
shotover_chain_messages_per_batch_count is no longer meaningful after some internal changes to shotover that occurred a year ago, to fix it, it needs to be split into two separate metrics.

Change 2

Additionally, this PR changes the metric logic of the two metrics to only write to the metric when the batch is not empty.
Previously empty batches never occured, but now that requests and responses are decoupled it is common for a response batch to have responses while the request batch is empty, and vice versa.
In order to ensure the histogram continues to show meaningful data (not full of zeroes) we skip those cases.

While there could be some value in seeing how often we hit empty batches, that is not the point of these metrics, instead we are trying to get a good picture of the spread of batch sizes.
If we find the need to measure empty batches in the future we could add simple shotover_empty_request_batch + shotover_empty_response_batch counter metrics.

Copy link

codspeed-hq bot commented May 21, 2024

CodSpeed Performance Report

Merging #1633 will not alter performance

Comparing rukai:fix_chain_metrics (101a666) with main (84189fc)

Summary

✅ 39 untouched benchmarks

@rukai rukai changed the title fix chain metrics fix shotover_chain_messages_per_batch_count metric May 21, 2024
@rukai rukai force-pushed the fix_chain_metrics branch 2 times, most recently from 07c0d79 to c08e6ec Compare September 22, 2024 23:45
@rukai rukai marked this pull request as ready for review September 23, 2024 03:33
@rukai rukai force-pushed the fix_chain_metrics branch 3 times, most recently from b1098f2 to 39a02d2 Compare September 23, 2024 05:25
@rukai rukai force-pushed the fix_chain_metrics branch 2 times, most recently from 5c21a58 to 0ab6be1 Compare September 23, 2024 07:20
@rukai rukai enabled auto-merge (squash) September 23, 2024 07:23
@rukai rukai merged commit fca032a into shotover:main Sep 23, 2024
41 checks passed
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