-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Backend Service Custom Metrics. #13205
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @efimki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces the ability to use custom metrics for load balancing in Google Compute Engine's Backend Services. It modifies the BackendService.yaml
file to include support for CUSTOM_METRICS
as a balancing mode and adds related properties for configuring custom metrics. It also introduces WEIGHTED_ROUND_ROBIN
as a locality load balancing policy, which uses custom metrics reported by backends to compute weights for per-endpoint load balancing.
Highlights
- Balancing Mode: Adds
CUSTOM_METRICS
as a valid balancing mode for Backend Services, allowing load balancing based on custom utilization signals. - Custom Metrics Configuration: Introduces a
customMetrics
property to configure the custom metrics used for load balancing, including the metric name, dry-run mode, and target utilization. - Weighted Round Robin Load Balancing: Adds
WEIGHTED_ROUND_ROBIN
as a locality load balancing policy, enabling per-endpoint weighted round-robin load balancing based on custom metrics reported by backends. - Backend Service Custom Metrics: Adds a
customMetrics
property to the Backend service, which is a list of custom metrics used for theWEIGHTED_ROUND_ROBIN
locality load balancing policy.
Changelog
- mmv1/products/compute/BackendService.yaml
- Added
CUSTOM_METRICS
to the list of valid values for thebalancingMode
property. - Added a
customMetrics
property to configure custom metrics for theCUSTOM_METRICS
balancing mode, including name, dryRun, and maxUtilization. - Added
WEIGHTED_ROUND_ROBIN
to the list of valid values for thelocalityLbPolicy
property. - Added a
customMetrics
property to configure custom metrics for theWEIGHTED_ROUND_ROBIN
locality load balancing policy, including name and dryRun.
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A backend's plight, with metrics untold,
Load balancing's dance, both new and old.
Custom signals rise, a weighted decree,
Routing traffic with metrics to see.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces custom metrics for backend services, enhancing the flexibility and control over load balancing configurations. The addition of 'CUSTOM_METRICS' as a balancing mode and the associated properties are well-structured. However, some descriptions could be improved for clarity.
Summary of Findings
- Description Clarity: The descriptions for 'customMetrics' and its nested properties could benefit from more clarity, especially regarding the purpose and usage of the metrics.
- Dry Run Functionality: The description for the 'dryRun' field could be more explicit about its behavior and implications for load balancing.
Assessment
The pull request introduces custom metrics for backend services, which is a valuable addition. The code changes seem well-structured, but there are a few points that need attention before merging. I recommend addressing the comments provided, particularly regarding the descriptions and potential clarification of the 'dryRun' functionality. Once these issues are resolved, the pull request should be in good shape for merging. As always, ensure that others review and approve this code before merging.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_backend_service" "primary" {
backend {
custom_metrics {
dry_run = # value needed
max_utilization = # value needed
name = # value needed
}
}
custom_metrics {
dry_run = # value needed
name = # value needed
}
}
|
Tests analyticsTotal tests: 1120 Click here to see the affected service packages
🟢 All tests passed! View the build log |
Signed-off-by: Misha Efimov <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_backend_service" "primary" {
backend {
custom_metrics {
max_utilization = # value needed
}
}
}
|
Signed-off-by: Misha Efimov <[email protected]>
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Signed-off-by: Misha Efimov <[email protected]>
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Signed-off-by: Misha Efimov <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Signed-off-by: Misha Efimov <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Signed-off-by: Misha Efimov <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Signed-off-by: Misha Efimov <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1121 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
/retest |
Signed-off-by: Misha Efimov <[email protected]>
Signed-off-by: Misha Efimov <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
1 similar comment
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1123 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Tests analyticsTotal tests: 1125 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
/gemini summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an update test for updating some of these fields. Things like:
- Adding a custom metric when several already exist
- Removing a custom metric from several
- Updating dry_run from true -> false. We typically see issues with this type of update, so it would be good to check
type: Array | ||
description: | | ||
The set of custom metrics that are used for <code>CUSTOM_METRICS</code> BalancingMode. | ||
is_set: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the default, so it's not needed
@@ -960,6 +1010,31 @@ properties: | |||
description: | | |||
An optional, arbitrary JSON object with configuration data, understood | |||
by a locally installed custom policy implementation. | |||
- name: 'customMetrics' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not need maxUtilization?
} | ||
custom_metrics { | ||
name = "orca.named_metrics.foo" | ||
max_utilization = 0.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you not set this value in one of these tests?
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.