feat(control-plane): [issue 4746] pool sufficiency metrics - #4747
feat(control-plane): [issue 4746] pool sufficiency metrics#4747mabbott-aurorasolar wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds optional pool sufficiency metrics to track whether runner pools are adequately sized to handle incoming jobs. The feature allows users to monitor if their pool configurations are right-sized by recording metrics when jobs arrive.
- Adds a new
enable_pool_sufficiencyconfiguration option across all relevant modules - Implements metric collection logic in the scale-up function to track pool adequacy
- Includes comprehensive tests to verify the metric behavior under different scenarios
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| variables.tf | Adds enable_pool_sufficiency option to root module metrics configuration |
| modules/runners/variables.tf | Adds enable_pool_sufficiency option to runners module metrics configuration |
| modules/runners/scale-up.tf | Passes pool sufficiency metric environment variable to scale-up Lambda |
| modules/runners/job-retry/variables.tf | Adds enable_pool_sufficiency option to job-retry module configuration |
| modules/runners/job-retry/main.tf | Passes pool sufficiency metric environment variable to job-retry Lambda |
| modules/multi-runner/variables.tf | Adds enable_pool_sufficiency option to multi-runner module metrics configuration |
| lambdas/functions/control-plane/src/scale-runners/scale-up.ts | Implements pool sufficiency metric collection logic |
| lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts | Adds comprehensive tests for pool sufficiency metric functionality |
| examples/multi-runner/main.tf | Documents the new enable_pool_sufficiency option in example |
| examples/default/main.tf | Documents the new enable_pool_sufficiency option in example |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| function createPoolSufficiencyMetric(environment: string, payload: ActionRequestMessage, wasSufficient: boolean) { | ||
| if (yn(process.env.ENABLE_METRIC_POOL_SUFFICIENCY, { default: false })) { | ||
| const metric = createSingleMetric('SufficientPoolHosts', MetricUnit.Count, wasSufficient ? 1.0 : 0.0, { | ||
| Environment: environment, | ||
| }); | ||
| metric.addMetadata('Environment', environment); | ||
| metric.addMetadata('RepositoryName', payload.repositoryName); | ||
| metric.addMetadata('RepositoryOwner', payload.repositoryOwner); | ||
| } | ||
| } |
There was a problem hiding this comment.
The Environment dimension is added twice - once in the metric creation and again as metadata. The dimension in the metric creation (line 487) should be sufficient for grouping metrics, making the duplicate metadata on line 489 redundant.
There was a problem hiding this comment.
I think this is not correct review comment. First time environment is added as dimension. addMetadata is only added in the log not as dimension, correct?
|
@mabbott-aurorasolar thx for our PR. I have the next day not much time to dig in. Are you considering the PR ready? If not you can mark it as draft. I see no documentation, at least this part of the docs needs to be adjusted: https://github-aws-runners.github.io/terraform-aws-github-runner/configuration/#multiple-runner-module-in-your-aws-account |
|
@npalm great callout on the docs. I'll update those as well, and follow-up on copilot's suggestions. I honestly threw this together in an hour this morning and wasn't sure what the usual process on this repo is, but yeah I do think this is mostly feature complete. |
|
Sorry for keep you wainting. Will do my best to put the PR on the list for the next week. |
|
@mabbott-aurorasolar PR looks good in general, not tested yet. Did you had time to have a look at the docs? |
|
@mabbott-aurorasolar did you had time to check for an update on the docs? |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions. |
See #4746 for full context.
In short, this PR will add metrics (optionally) to record whether or not a given runner pool was sufficient to handle a job when it arrives. This will allow users (specifically, my company) to better understand whether we're right-sizing our pools or not.