Skip to content

Conversation

iambriccardo
Copy link
Contributor

@iambriccardo iambriccardo commented Apr 8, 2025

This PR updates the metrics emitted by the async pool:

  • utilization -> it now calculates how busy the pool is by checking the CPU-bound work it is doing.
  • activity -> it checks how active the pool is (this was the previous utilization metric). The activity determines how many futures are currently being driven by the pool w.r.t. the total capacity of futures it can hold.

To make this possible I have reused the ServiceMonitor which has been renamed to TimedFuture and it has been made agnostic of any service related internals. Additionally, the future now relies on a TimedFutureReceiver to emit lifecycle and state changes.

Closes: https://github.com/getsentry/team-ingest/issues/689

@iambriccardo iambriccardo changed the title Fix feat(pool): Expose utilization and activity metrics from the pool Apr 9, 2025
@iambriccardo iambriccardo marked this pull request as ready for review April 9, 2025 12:24
@iambriccardo iambriccardo requested a review from a team as a code owner April 9, 2025 12:24
@iambriccardo iambriccardo requested a review from Dav1dde April 10, 2025 07:42
///
/// Note that this metric is collected and updated for each thread when the main future is polled,
/// thus if no work is being done, it will not be updated.
pub fn utilization(&self) -> u8 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call that cpu_utilization and expose a total_utilization which does max(activity, cpu_utilization) which can be used in the auto scaler thingy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking of the edge case where we have 100% activity since we are doing lots of I/O and 0% max utilization (assuming no cpu bound work is being done). Do we really want to scale in this case? Scaling might not help.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you, without thinking too much about I would have said yes, because scaling is probably the safer option.

@iambriccardo iambriccardo enabled auto-merge (squash) April 10, 2025 13:14
@iambriccardo iambriccardo merged commit 501d3e6 into master Apr 10, 2025
27 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/expose-busy-metric branch April 10, 2025 13:20
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.

2 participants