Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(autoscaling): add instanceMonitoring option #8213

Merged
merged 2 commits into from
Jun 11, 2020
Merged

feat(autoscaling): add instanceMonitoring option #8213

merged 2 commits into from
Jun 11, 2020

Conversation

rbobrowicz
Copy link
Contributor

@rbobrowicz rbobrowicz commented May 26, 2020 •
edited
Loading

Gives users the option to choose between detailed and basic monitoring.
Defaults to detailed when not specified, maintaining current behavior.

Fixes #8212

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@rbobrowicz rbobrowicz changed the title feat(autoscaling): add instanceMonitoring option (#8212) feat(autoscaling): add instanceMonitoring option May 26, 2020
@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from da9baf7 to 8f6e2a6 Compare May 26, 2020 20:05
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8f6e2a6
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: da9baf7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from 8f6e2a6 to 7d7537e Compare May 26, 2020 20:46
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 7d7537e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr
rix0rrr reviewed May 27, 2020
View reviewed changes
packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts Outdated
*
* @default true
*/
readonly instanceMonitoring?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this an enum? Monitoring.DETAILED and Monitoring.BASIC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, I updated the PR.

@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from 7d7537e to bf9827a Compare June 1, 2020 23:31
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: bf9827a
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from bf9827a to 3142a98 Compare June 1, 2020 23:47
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 3142a98
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from 3142a98 to 5ae7759 Compare June 2, 2020 00:02
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 5ae7759
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

NetaNir
NetaNir previously requested changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
We need to add a test that verifies the behavior when no value is provided - the default behavior.
Thanks!

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts Outdated
*
* @see https://docs.aws.amazon.com/autoscaling/latest/userguide/as-instance-monitoring.html#enable-as-instance-metrics
*
* @default Monitoring.DETAILED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @default Monitoring.DETAILED
* @default - Monitoring.DETAILED

We use the @default - syntax to note that it's the default CloudFormation/AWS behavior, not CDK implementation.

@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from 5ae7759 to e4ec245 Compare June 10, 2020 21:44
@mergify mergify bot dismissed NetaNir’s stale review June 10, 2020 21:45

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e4ec245
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rbobrowicz
feat(autoscaling): add instanceMonitoring option
ac37031 
Gives users the option to choose between detailed and basic monitoring.
Defaults to detailed when not specified, maintaining current behavior.

fixes aws#8212
@rbobrowicz rbobrowicz force-pushed the instance-monitoring branch from e4ec245 to ac37031 Compare June 10, 2020 21:57
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ac37031
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rbobrowicz rbobrowicz requested a review from NetaNir June 10, 2020 23:05
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e5bc5c4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 11, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 6e23ae7 into aws:master Jun 11, 2020
@rbobrowicz rbobrowicz deleted the instance-monitoring branch June 12, 2020 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr left review comments

@NetaNir NetaNir NetaNir approved these changes

Assignees

@NetaNir NetaNir

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Instance Monitoring option to AutoScaling Groups
4 participants
@rbobrowicz @aws-cdk-automation @rix0rrr @NetaNir