chore: add a class to handle aggreggation queries #8446
Merged
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
gastonfournier
commented
Oct 14, 2024
gastonfournier
force-pushed
the
proposal/handle-aggregation-queries-sequentially
branch
from
f02fe87 to
3cbbeff
Compare
gastonfournier
commented
Oct 15, 2024
src/lib/metrics.ts
Outdated
Comment on lines
131
to
140
| dbMetrics.registerGaugeDbMetric({ | ||
| name: 'max_feature_strategies', | ||
| help: 'Maximum number of strategies in one feature', | ||
| labelNames: ['feature'], | ||
| query: () => | ||
| stores.featureStrategiesReadModel.getMaxFeatureStrategies(), | ||
| map: (result) => ({ | ||
| count: result.count, | ||
| labels: { feature: result.feature }, | ||
| }), |
There was a problem hiding this comment.
Gauge and query are now declared together
sjaanus
reviewed
Oct 15, 2024
| @@ -408,7 +419,6 @@ export default class MetricsMonitor { | |||
| instanceOnboardingMetrics, | |||
| projectsOnboardingMetrics, | |||
| ] = await Promise.all([ | |||
There was a problem hiding this comment.
We do not need to run these in parallel. I would be to get rid of promise all.
There was a problem hiding this comment.
Exactly, we are going to do that gradually. When we register a new metric into this new implementation, the new implementation will run them sequentially
nunogois
reviewed
Oct 15, 2024
nunogois
reviewed
Oct 15, 2024
nunogois
reviewed
Oct 15, 2024
Co-authored-by: Nuno Góis <[email protected]>
…ies-sequentially' into proposal/handle-aggregation-queries-sequentially
This is a follow up on #8446 - migrating some metrics (check the commits) - moving event listener registration outside metric calculation (every 2 hs we are adding new listeners) - adding jitter to metric refresh - manually executing some of the tasks immediately to avoid changing the test logic (with jitter now we'd have to force or wait until the first execution of the scheduled task that collect the metrics). We might decide to change this later after the migration - try catch the execution of the task - register metrics now "just registers" and returns functions for refreshing metrics. In the future, we'll return an object with the ability of independently updating specific metrics so we can tweak the frequency of updates
…gation-queries-sequentially
gastonfournier
deleted the
proposal/handle-aggregation-queries-sequentially
branch
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.
About the changes
We have many aggregation queries that run on a schedule:
unleash/src/lib/metrics.ts
Lines 714 to 719 in f63496d
These staticCounters are usually doing db query aggregations that traverse tables and we run all of them in parallel:
unleash/src/lib/metrics.ts
Lines 410 to 412 in f63496d
This can add strain to the db. This PR suggests a way of handling these queries in a more structured way, allowing us to run them sequentially (therefore spreading the load):
unleash/src/lib/metrics-gauge.ts
Lines 38 to 40 in f02fe87
As an additional benefit, we get both the gauge definition and the queries in a single place:
unleash/src/lib/metrics.ts
Lines 131 to 141 in f02fe87
This PR only tackles 1 metric, and it only focuses on gauges to gather initial feedback. The plan is to migrate these metrics and eventually incorporate more types (e.g. counters)