Add Scheduler and TokenBucket metrics#125
Add Scheduler and TokenBucket metrics#125landonxjames wants to merge 2 commits intofeature/metricsfrom
Conversation
| pub(crate) struct TokenBucketMetrics { | ||
| max_tokens: u64, | ||
| available_tokens: UpDownCounter, | ||
| token_wait_time: Histogram, |
There was a problem hiding this comment.
I'm not sure if this will be meaningful or not given permit requests come with different token amounts requested.
There was a problem hiding this comment.
I wonder if tracking percentage of available tokens over time or something would be more useful?
aws-sdk-s3-transfer-manager/src/middleware/limit/concurrency/future.rs
Outdated
Show resolved
Hide resolved
| // NOTE: tokio semaphore is fair, permits are given out in the order requested | ||
| semaphore: Arc<Semaphore>, | ||
| mode: ConcurrencyMode, | ||
| tb_metrics: Arc<TokenBucketMetrics>, |
There was a problem hiding this comment.
I'm kind of wondering whether token bucket metrics provide value at this point in time. 🤔
There was a problem hiding this comment.
I am also not sure, but will probably leave them in place until I can actually run some benchmarks and test all of this on some real use cases. Can remove if we find it not necessary after that testing.
There was a problem hiding this comment.
Ok maybe add a comment or something to that effect
Remove permit acquisition/release tracking and simplify inflight request monitoring with consistent counter types.
| pub(crate) struct Scheduler { | ||
| token_bucket: TokenBucket, | ||
| pub(crate) metrics: SchedulerMetrics, | ||
| pub(crate) metrics: Arc<SchedulerMetrics>, |
There was a problem hiding this comment.
nit: will this end up double-Arc ing? Individual fields of SchedulerMetrics seem to have their types backed by Arc already, e.g. Histogram, Gauge?
| // High water mark for inflight tasks | ||
| max_inflight: IncreasingCounter, | ||
| // Count of failed permit acquisitions | ||
| permit_acquisition_failures: IncreasingCounter, |
There was a problem hiding this comment.
not sure we need this at this stage
| // NOTE: tokio semaphore is fair, permits are given out in the order requested | ||
| semaphore: Arc<Semaphore>, | ||
| mode: ConcurrencyMode, | ||
| tb_metrics: Arc<TokenBucketMetrics>, |
There was a problem hiding this comment.
Ok maybe add a comment or something to that effect
| fn new( | ||
| sem: PollSemaphore, | ||
| tokens: u32, | ||
| tb_metrics: Arc<TokenBucketMetrics>, |
There was a problem hiding this comment.
this feels off/wrong to need all these parameters for this future but I get why it's done. Wondering if there isn't a cleaner approach though.
3 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.