-
-
Notifications
You must be signed in to change notification settings - Fork 628
feat: Add support for predictive autoscaling policies #361
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
base: master
Are you sure you want to change the base?
feat: Add support for predictive autoscaling policies #361
Conversation
I want to address a naming inconsistency across
Target tracking using dimensionS while all predictive scaling metrics use dimension naming. In the module configuration, I decided to use metrics = optional(list(object({
expression = optional(string)
id = string
label = optional(string)
metric_stat = optional(object({
metric = object({
dimensions = optional(list(object({
name = string
value = string
})))
metric_name = string
namespace = string
})
stat = string
unit = optional(string)
}))
return_data = optional(bool)
}))) I believe it can help prevent mistakes or typos when using multiple scaling policies for a single service. |
thats due to the API - https://github.com/hashicorp/terraform-provider-aws/blob/782cca0e2343caa5ee37950708d563ef25a95aa7/internal/service/appautoscaling/policy.go#L249 We usually try to match the underlying API since thats the easiest and least confusing, only deviating in select scenarios |
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.
great start! few changes to the structure and definitions
} | ||
} | ||
|
||
dynamic "predictive_scaling_policy_configuration" { |
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.
general comment, lets follow the order of fields on the AWS provider. It makes it easier to check and compare https://github.com/hashicorp/terraform-provider-aws/blob/782cca0e2343caa5ee37950708d563ef25a95aa7/internal/service/appautoscaling/policy.go#L67
for_each = each.value.policy_type == "PredictiveScaling" && each.value.predictive_scaling_policy_configuration != null ? [each.value.predictive_scaling_policy_configuration] : [] | ||
|
||
content { | ||
mode = try(predictive_scaling_policy_configuration.value.mode, null) |
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.
with variable optional attributes, we can drop all the try()
wrappings. you can refer to the target_tracking_scaling_policy_configuration
for reference
scale_out_cooldown = optional(number, 60) | ||
target_value = optional(number, 75) | ||
})) | ||
predictive_scaling_policy_configuration = optional(object({ |
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.
and same here, lets follow the order of fields on the AWS provider which also follows the order of the implementation in this module
max_capacity_buffer = optional(number) | ||
max_capacity_breach_behavior = optional(string) | ||
scheduling_buffer_time = optional(number) | ||
metric_specification = object({ |
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 of type list(object{ ...
per https://github.com/hashicorp/terraform-provider-aws/blob/782cca0e2343caa5ee37950708d563ef25a95aa7/internal/service/appautoscaling/policy.go#L84-L85
})) | ||
})) | ||
customized_scaling_metric_specification = optional(object({ | ||
metric_data_query = list(object({ |
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.
outer type should change to optional(object{...
per https://github.com/hashicorp/terraform-provider-aws/blob/782cca0e2343caa5ee37950708d563ef25a95aa7/internal/service/appautoscaling/policy.go#L416-L417
value = string | ||
}))) | ||
metric_name = string | ||
namespace = string |
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.
namespace = string | |
namespace = optional(string) |
return_data = optional(bool) | ||
})) | ||
})) | ||
customized_capacity_metric_specification = optional(object({ |
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.
same type as the corrected customized_scaling_metric_specification
predefined_metric_type = string | ||
resource_label = optional(string) | ||
})) | ||
customized_load_metric_specification = optional(object({ |
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.
same type as the corrected customized_scaling_metric_specification
target_value = optional(number, 75) | ||
})) | ||
predictive_scaling_policy_configuration = optional(object({ | ||
mode = optional(string, "ForecastAndScale") |
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.
whats the reasoning behind this default value?
scale_out_cooldown = optional(number) | ||
target_value = optional(number) | ||
})) | ||
predictive_scaling_policy_configuration = optional(object({ |
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.
update this once the corrections to module/service/variables.tf
are made
Description
Added support for predefined metrics
predefined_load_metric_specification
predefined_metric_pair_specification
predefined_scaling_metric_specification
Added support for customized metrics
customized_capacity_metric_specification
customized_load_metric_specification
customized_scaling_metric_specification
Motivation and Context
predictive_scaling_policy_configuration
configuration block for service autoscaling #352Breaking Changes
No
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request