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

Implement new concurrency limit mechanism for the CloudWatch client #1107

Conversation

thepalbi
Copy link
Contributor

This PR implements a new concurrency mechanism for the CloudWatch client. For this, the client is decoupled of how the concurrency is limited, and two implementations are provided:

  1. a single limit limiter, which is the current implementation
  2. one that keeps one limit per api call

The second one will allow us to take full advantage of the rate limits imposed by AWS into each api.

cmd/yace/main.go Outdated Show resolved Hide resolved
// perAPICallLimiter is a ConcurrencyLimiter that keeps a different concurrency limiter per different API call. This allows
// a more granular control of concurrency, allowing us to take advantage of different api limits. For example, ListMetrics
// has a limit of 25 TPS, while GetMetricData has none.
type perAPICallLimiter map[string]chan struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we potentially use three different singleLimiter instead of the perAPICallLimiter? I think that would be a bit more readable perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, completely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so extracted the common funcionality into a simple semaphore type, and re-used that everywhere.

@cristiangreco cristiangreco added this to the v0.56.0 milestone Sep 12, 2023
@thepalbi thepalbi force-pushed the pablo/new-concurrency-limiter branch from 1578282 to d551711 Compare September 13, 2023 17:28
@thepalbi
Copy link
Contributor Author

@cristiangreco what do you think of the latest changes here?

cmd/yace/main.go Outdated Show resolved Hide resolved
cmd/yace/main.go Outdated Show resolved Hide resolved
cmd/yace/main.go Outdated Show resolved Hide resolved
cmd/yace/main.go Outdated Show resolved Hide resolved
cmd/yace/main.go Outdated Show resolved Hide resolved
pkg/exporter.go Outdated Show resolved Hide resolved
pkg/exporter.go Outdated Show resolved Hide resolved
cmd/yace/scraper.go Outdated Show resolved Hide resolved
pkg/exporter.go Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
@cristiangreco cristiangreco merged commit fad9e02 into prometheus-community:master Sep 27, 2023
3 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.

2 participants