Run ListMetrics calls with goroutines. #53
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…abelConsistencyForMetrics
Eliminate global labelMap and build a labelSet for metrics in ensureLabelConsistencyForMetrics
…tch to structured logging (#7)
* Pass logger to the different structs * Use a builder for the AWS services * Use job specific loggers to propagate properties
* Merge latest release 0.34.0-alpha * Add missing metric
* Move AWS metrics back to prometheus.go and expose them as array in update.go * The go workflow should run on PR's for live * Remove unused field
Execute ListMetrics calls in separate goroutines (one for each metric), in a similar way to how GetMetricData requests are handled. This change removes semaphore usage (it's not used in GetMetricData as well) for the sake of making it easier to reason about the code. It can easily be added back in case we hit any issue around e.g. rate limits. In local tests, the speedup seems to be particularly effective when requesting 4 or more metrics in parallel (e.g. especially with EC2/EBS).
matthewnolf
approved these changes
May 31, 2022
kgeckhart
approved these changes
May 31, 2022
There was a problem hiding this comment.
This change removes semaphore usage (it's not used in GetMetricData as well) for the sake of making it easier to reason about the code. It can easily be added back in case we hit any issue around e.g. rate limits.
Slight concern, errors aren't propagated out of the parallel sections of the code ATM so we might want to make sure we are monitoring errors a little closer after this.
IfSentient
approved these changes
May 31, 2022
thepalbi
approved these changes
May 31, 2022
thepalbi
reviewed
May 31, 2022
| metricsList, err := getFullMetricsList(ctx, svc.Namespace, m, clientCloudwatch) | ||
|
|
||
| if err != nil { | ||
| level.Error(logger).Log("msg", "Failed to get full metric list", "err", err, "metric_name", m.Name, "namespace", svc.Namespace) |
There was a problem hiding this comment.
Maybe we could add a prom metrics to track both this errors and the scenario below with zero resources?
ferruvich
approved these changes
May 31, 2022
|
|
f35cc9e
to
b15fdd9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Execute ListMetrics calls in separate goroutines (one for each metric),
in a similar way to how GetMetricData requests are handled.
This change removes semaphore usage (it's not used in GetMetricData as
well) for the sake of making it easier to reason about the code. It can
easily be added back in case we hit any issue around e.g. rate limits.
In local tests, the speedup seems to be particularly effective when
requesting 4 or more metrics in parallel (e.g. especially with EC2/EBS).