fix: support metric collections for autoscaling group #6712
Conversation
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
fogfish
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
HI @fogfish!
Thanks for submitting the PR!
I started reviewing it and I think there might be an opportunity to make this API neat:)
Given metricsCollection is a list of metrics Collection object:
{
granularity: '1Minute'
metrics: [] || undefined
}
Since granularity is a required property with a single allowed value - 1Minute, it make sense for the CDK to make it optional and have a default value of 1Minute. But, given that metrics is also optional as with a defaults to allMetrics this will result is a somewhat weird API, to collect all metrics in the default granularity the user code will be:
metricsCollections: [{}]
To make the API cleaner we can leverage the "union like class" pattern we use in the CDK. The union like pattern allows the initialization of a type using different parameters set. Here is a sketch of the code:
export interface MetricsCollectionOptions {
readonly granularity?: Granularity;
readonly metrics: MetricGroup[];
}
export interface MetricsCollectionProps {
readonly granularity: Granularity;
readonly metrics: MetricGroup[];
}
export class MetricsCollections {
/**
* Enable the monitoring of all metrics of the Auto Scaling group
* @param granularity The frequency at which Amazon EC2 Auto Scaling sends aggregated data to CloudWatch (default 1Minute)
*/
public static allMetrics(granularity?: Granularity): MetricsCollections {
return new MetricsCollections({
granularity: granularity ?? Granularity.ONE_MINUTE,
metrics: []
});
}
/**
*
* @param metricsCollections
*/
public static groups(...metricsCollections: MetricsCollectionOptions[]): MetricsCollections {
return new MetricsCollections(...metricsCollections.map(mc => ({
granularity: mc.granularity ?? Granularity.ONE_MINUTE,
metrics: mc.metrics
})));
}
public readonly collections: MetricsCollectionProps[];
private constructor(...metricsCollections: MetricsCollectionProps[]) {
this.collections = metricsCollections;
}
}And the API will look like this:
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
metricCollections: MetricsCollections.allMetrics(),
// other properties
}To collect all metrics.
or:
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
metricCollections: MetricsCollections.groups([
{
granularity: Granularity.ONE_MINUTE,
metrics: [MetricsGroup. GROUP_IN_SERVICE_INSTANCES]
}
]),
// other properties
}To collect different metricsGroups in different granularity.
I know it's a lot but I would be happy to do another round of review if you want to pick it up.
I'm not attached to any of the names so feel free to use other names.
Good examples for the union like class in the CDK are Schedule in aws-application-autoscaling and Code in aws-lambda.
Thanks again!
| export enum GroupMetric { | ||
| /** GroupMinSize */ | ||
| MIN_SIZE = "GroupMinSize", | ||
|
|
||
| /** GroupMaxSize */ | ||
| MAX_SIZE = "GroupMaxSize", | ||
|
|
||
| /** GroupDesiredCapacity */ | ||
| DESIRED_CAPACITY = "GroupDesiredCapacity", | ||
|
|
||
| /** GroupInServiceInstances */ | ||
| IN_SERVICE_INSTANCES = "GroupInServiceInstances", | ||
|
|
||
| /** GroupPendingInstances */ | ||
| PENDING_INSTANCES = "GroupPendingInstances", | ||
|
|
||
| /** GroupStandbyInstances */ | ||
| STANDBY_INSTANCES = "GroupStandbyInstances", | ||
|
|
||
| /** GroupTerminatingInstances */ | ||
| TERMINATING_INSTANCES = "GroupTerminatingInstances", | ||
|
|
||
| /** GroupTotalInstances */ | ||
| TOTAL_INSTANCES = "GroupTotalInstances", | ||
| } | ||
|
|
There was a problem hiding this comment.
If the docs doesn't add any information there is no need to add them
There was a problem hiding this comment.
I would love to remove these comments but the build system gonna give me a error that documentation is required for exported types.
| * Auto Scaling group metrics | ||
| */ | ||
| export enum GroupMetric { | ||
| /** GroupMinSize */ |
There was a problem hiding this comment.
- Change to
MetricGroup
| * https://docs.aws.amazon.com/autoscaling/ec2/userguide/as-instance-monitoring.html | ||
| */ | ||
| export interface AutoScalingGroupMetric { | ||
| /** |
There was a problem hiding this comment.
We can name this MetricsCollection so it will match the actual property.
There was a problem hiding this comment.
- Change to
MetricsCollection
| /** | ||
| * Describes the group metrics that an Amazon EC2 Auto Scaling group sends to Amazon CloudWatch. | ||
| * These metrics describe the group rather than any of its instances. | ||
| * For more information, see Monitoring Your Auto Scaling Groups and Instances Using Amazon CloudWatch in the Amazon EC2 Auto Scaling User Guide. | ||
| * https://docs.aws.amazon.com/autoscaling/ec2/userguide/as-instance-monitoring.html | ||
| */ |
There was a problem hiding this comment.
Since this describe MetricCollection this should refer to the MetricCollection doc
There was a problem hiding this comment.
- Update the link in the doc
| * The frequency at which Amazon EC2 Auto Scaling sends aggregated data to CloudWatch. | ||
| */ | ||
| readonly granularity: MetricGranularity, | ||
|
|
There was a problem hiding this comment.
This should be optional and have a default value of 1Minute which is the only allowed value.
| * The frequency at which Amazon EC2 Auto Scaling sends aggregated data to CloudWatch. | ||
| */ | ||
| export type MetricGranularity = "1Minute"; | ||
|
|
There was a problem hiding this comment.
You build system gives an error if I create enum with one entry. This is exactly how I've implemented at first time.
| /** | ||
| * The list of Auto Scaling group metrics to collect. If you specify Granularity and don't specify any metrics, all metrics are enabled. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * The list of Auto Scaling group metrics to collect. If you specify Granularity and don't specify any metrics, all metrics are enabled. | |
| */ | |
| /** | |
| * The list of Auto Scaling group metrics to collect. | |
| * @deafult - all metrics are enabled | |
| */ |
Use the @default - notation here
| * | ||
| * @default - metrics collection is disabled. | ||
| */ | ||
| readonly metricsCollection?: AutoScalingGroupMetric[] | ||
| } | ||
|
|
There was a problem hiding this comment.
rename to metricsCollections
There was a problem hiding this comment.
It is called metricCollection at CF.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-group.html#cfn-as-group-metricscollection
| /** | ||
| * The list of Auto Scaling group metrics to collect. If you specify Granularity and don't specify any metrics, all metrics are enabled. | ||
| */ | ||
| readonly metrics: GroupMetric[], |
| @@ -514,6 +573,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements | |||
| vpcZoneIdentifier: subnetIds, | |||
| healthCheckType: props.healthCheck && props.healthCheck.type, | |||
| healthCheckGracePeriod: props.healthCheck && props.healthCheck.gracePeriod && props.healthCheck.gracePeriod.toSeconds(), | |||
| metricsCollection: props.metricsCollection || undefined, | |||
| }; | |||
There was a problem hiding this comment.
there is no need to add the || undefined
NetaNir
added
the
needs-discussion
|
Thank you very much for your suggestion about the union pattern. I think it is helpful in may use-cases. Unfortunately, I've confused about its usability from developers perspective in this particular use-case. Let's step into developer shoe: As a developer I want to enable ASG monitoring so that ... As a developer I want to enable monitoring of specific metrics so that ... It became more friendly for developer to use boolean flag just to enable monitoring. Therefore I would tune ASG properties to accept Sum types class ASGProps {
readonly enableMonitoring: boolean | MetricsCollection[]
}
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
enableMonitoring: true
}
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
enableMonitoring: [
{
granularity: Granularity.ONE_MINUTE,
metrics: [MetricsGroup. GROUP_IN_SERVICE_INSTANCES]
},
]
} |
|
@NetaNir Any thoughts? I would not make any change to the PR until we agree about API. |
TypeScript represents enums similar to union types, and single-valued enums are "simplified" to the sole member (the TypeChecker fiercely refuses to give a handle to the actual `enum` type). This resulted in `jsii` incorrectly tagging the type at usage sites. This commit adds the necessary infrastructure to detect single-valued enums and tweak the FQN generation to obtain the correct result. A new test was added to validate this whole endeavor works correctly even when the single-valued enum is within a submodule (which adds even more complexity to the mix). References: aws/aws-cdk#6712 aws/aws-cdk#6948
TypeScript represents enums similar to union types, and single-valued enums are "simplified" to the sole member (the TypeChecker fiercely refuses to give a handle to the actual `enum` type). This resulted in `jsii` incorrectly tagging the type at usage sites. This commit adds the necessary infrastructure to detect single-valued enums and tweak the FQN generation to obtain the correct result. A new test was added to validate this whole endeavor works correctly even when the single-valued enum is within a submodule (which adds even more complexity to the mix). References: aws/aws-cdk#6712 aws/aws-cdk#6948
|
Hi @fogfish! The single enum bug was fixed! As for the API, the experience in Java, C# and typically other statically typed languages without type unions will be inadequate, which is why we try to avoid it in the CDK (we currently can't prevent it in jsii since it will be a breaking change) and use the union like pattern. |
|
Is there any update for this issue? My project is still waiting for this feature to be closed. Thanks! |
|
Closing this PR in favor of #7432 |
fixes #6453