chore: handle aggregation queries sequentially follow up #8450

gastonfournier · 2024-10-15T09:41:27Z

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

github-actions · 2024-10-15T09:41:42Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

vercel · 2024-10-15T09:42:17Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 17, 2024 9:40pm

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 17, 2024 9:40pm

gastonfournier · 2024-10-15T11:24:12Z

src/lib/metrics-gauge.ts

    count: number;
    labels: RestrictedRecord<GaugeDefinition<R>['labelNames']>;
 };
+type MapResult<R> = (result: R) => MetricValue<R> | MetricValue<R>[];


A single task can output different counts for different labels. This is useful for cases like client_apps_total at line 40

gastonfournier · 2024-10-15T11:29:17Z

src/lib/metrics.ts

+        // schedule and execute immediately
+        await dbMetrics.registerGaugeDbMetric({


These tasks need to execute immediately because the tests expect that. I'd argue that some of these can be delayed, but maybe they are important because of other components depending on these. So I'm delaying this decision until we migrated all gauges to dbMetrics

…cs for stats

gastonfournier · 2024-10-17T21:08:45Z

src/lib/metrics.ts

            try {
-                dbMetrics.refreshDbMetrics();
-
-                const stats = await instanceStatsService.getStats();


getStats was triggering many queries in parallel, some of which were not used in prometheus.

gastonfournier · 2024-10-17T21:10:47Z

src/lib/metrics.ts

@@ -512,16 +858,6 @@ export default class MetricsMonitor {
                legacyTokensActive.reset();
                legacyTokensActive.set(deprecatedTokens.activeLegacyTokens);

-                if (maxEnvironmentStrategies) {


This got migrated to: https://github.com/Unleash/unleash/pull/8450/files#diff-0610ce1595d1dcf150ca2efb2f0c5806d082f5bc6b8429a437970c7d60171cbdR181-R194 where both the gauge definition and refresh query are together

gastonfournier · 2024-10-17T21:13:46Z

src/lib/metrics.ts

-                eventBus.on(
-                    events.STAGE_ENTERED,
-                    (entered: { stage: string; feature: string }) => {
-                        if (flagResolver.isEnabled('trackLifecycleMetrics')) {
-                            logger.info(
-                                `STAGE_ENTERED listened ${JSON.stringify(entered)}`,
-                            );
-                        }
-                        featureLifecycleStageEnteredCounter.increment({
-                            stage: entered.stage,
-                        });
-                    },
-                );
-
-                eventBus.on(
-                    events.EXCEEDS_LIMIT,
-                    ({
-                        resource,
-                        limit,
-                    }: { resource: string; limit: number }) => {
-                        exceedsLimitErrorCounter.increment({ resource, limit });
-                    },
-                );


Moved up to line 485 on the right side. Now lives inside registerPrometheusMetrics instead of startMonitoring

gastonfournier · 2024-10-17T21:14:50Z

src/lib/metrics.ts

+        const { collectStaticCounters, collectDbMetrics } =
+            registerPrometheusMetrics(
+                config,
+                stores,
+                version,
+                eventBus,
+                instanceStatsService,
+            );


Start monitoring now calls helper functions that register prometheus metrics and then adds schedules to frequently update these metrics

gastonfournier · 2024-10-18T06:56:58Z

src/lib/features/instance-stats/instance-stats-service.ts

@@ -338,11 +369,12 @@ export class InstanceStatsService {
            this.clientInstanceStore.getDistinctApplicationsCount(30),
            this.clientInstanceStore.getDistinctApplicationsCount(),
        ]);
-        return {
+        this.appCount = {


Store the result, so it can be used internally (line 261)

nunogois

Discussed 1on1, LGTM

gastonfournier added 3 commits October 15, 2024 11:29

fix: move event listeners outside metric collection

9bf5ed5

Migrate max_feature_environment_strategies

7b585f1

Migrate feature_toggles_total

8cda5cd

github-actions bot assigned gastonfournier Oct 15, 2024

gastonfournier changed the title ~~chore: Proposal/handle aggregation queries sequentially follow up~~ chore: handle aggregation queries sequentially follow up Oct 15, 2024

vercel bot deployed to Preview – unleash-monorepo-frontend October 15, 2024 10:05 View deployment

Ability to execute a task immediately

e2cd347

vercel bot deployed to Preview – unleash-monorepo-frontend October 15, 2024 10:47 View deployment

Remove jitter and migrate tasks to execute immediately

73f3559

vercel bot deployed to Preview – unleash-monorepo-frontend October 15, 2024 11:17 View deployment

Add type for MetricValue

4806106

gastonfournier commented Oct 15, 2024

View reviewed changes

gastonfournier marked this pull request as ready for review October 15, 2024 11:29

vercel bot deployed to Preview – unleash-monorepo-frontend October 15, 2024 11:41 View deployment

gastonfournier added 4 commits October 16, 2024 23:17

Get closer to prom-client types

05a338c

Renames to have gauge and task together

f5cf84c

Split metric registration from refresh, use existing prometheus metri…

5edd0f8

…cs for stats

Instance stats should not rely on prometheus cause it can be disabled

e7de20f

vercel bot deployed to Preview – unleash-monorepo-frontend October 17, 2024 21:06 View deployment

Replace removed method

e97f748

vercel bot deployed to Preview – unleash-monorepo-frontend October 17, 2024 21:19 View deployment

Add comment

add3bed

vercel bot deployed to Preview – unleash-monorepo-frontend October 17, 2024 21:24 View deployment

Move rate limits outside the refresh function as they're static

1a977fe

gastonfournier commented Oct 17, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend October 17, 2024 21:40 View deployment

gastonfournier commented Oct 18, 2024

View reviewed changes

nunogois approved these changes Oct 18, 2024

View reviewed changes

gastonfournier merged commit 7a4c855 into proposal/handle-aggregation-queries-sequentially Oct 18, 2024
9 checks passed

gastonfournier deleted the proposal/handle-aggregation-queries-sequentially-follow-up branch October 18, 2024 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: handle aggregation queries sequentially follow up #8450

chore: handle aggregation queries sequentially follow up #8450

gastonfournier commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

vercel bot commented Oct 15, 2024 •

edited

Loading

gastonfournier Oct 15, 2024

gastonfournier Oct 15, 2024

gastonfournier Oct 17, 2024

gastonfournier Oct 17, 2024

gastonfournier Oct 17, 2024

gastonfournier Oct 17, 2024

gastonfournier Oct 18, 2024

nunogois left a comment

		// schedule and execute immediately
		await dbMetrics.registerGaugeDbMetric({

chore: handle aggregation queries sequentially follow up #8450

chore: handle aggregation queries sequentially follow up #8450

Conversation

gastonfournier commented Oct 15, 2024 • edited Loading

github-actions bot commented Oct 15, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

vercel bot commented Oct 15, 2024 • edited Loading

gastonfournier Oct 15, 2024

Choose a reason for hiding this comment

gastonfournier Oct 15, 2024

Choose a reason for hiding this comment

gastonfournier Oct 17, 2024

Choose a reason for hiding this comment

gastonfournier Oct 17, 2024

Choose a reason for hiding this comment

gastonfournier Oct 17, 2024

Choose a reason for hiding this comment

gastonfournier Oct 17, 2024

Choose a reason for hiding this comment

gastonfournier Oct 18, 2024

Choose a reason for hiding this comment

nunogois left a comment

Choose a reason for hiding this comment

gastonfournier commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

vercel bot commented Oct 15, 2024 •

edited

Loading