Upgrade to be terraform .12 compatible & other changes #21

caleb15 · 2020-07-02T18:33:28Z

Upgrades code to be terraform .12 compatible fixes update to be hcl2 compliant #16
Makes alarm names unique fixes alarm names should be unique #5
Breaking change: requires sns topic param instead of creating one for the user. The user may already have a prexisting SNS topic that they might want to use. The repo should just concern itself with alarms and not with SNS.
Breaking change: db_instance_id changed to db_instance_ids to support multiple db's
add alarm_period and alarm_evaluation_periods params

We are running this change in production for 15Five.

jamengual · 2020-07-02T18:44:22Z

It shouldn't be assumed that it's okay to create a new topic - the user might want to reuse an existing sns topic for the RDS alarms. DEV-812

That is going to break other modules, could it be possible for you to use

count = length(var.aws_sns_topic_arn) != 0 ? 1 : 0

to the topics so that if there is no var.aws_sns_topic_arn supplied it created one for you?

thanks

caleb15 · 2020-07-07T16:26:27Z

@jamengual Yeah, that should be possible. But either way the PR will still have a breaking change with the db instance id param.

I'm going to wait for a maintainer to respond to see what changes would be necessary to merge this in.

jamengual · 2020-07-07T17:14:20Z

for the breaking changes I think is possible to maybe leave them empty or use a dynamic.

nitrocode · 2020-07-07T17:17:00Z

To make this easier to review, could you put these changes in separate PRs?

We should at the very least have a separate PR for terraform11 to terraform12 changes to make this easier to review.

3h4x

Great work @caleb15 ! Thank you very much for PR.
These are great changes. I'm in favour of removing SNS topic creation without any backward compatibility. It should be fairly easy to migrate to new version thanks to variable aws_sns_topic_arn. In @cloudposse/contributors we are working on SNS module (initial PR already pushed) that should be used in conjunction with any CloudWatch alarm module.
Additionally it's a good change to monitor multiple RDS instances with this single module. We can avoid backward incompatibilty by reducing readablity of code and by having tech debt. Let's avoid that.
I left couple of questions and few improvements.

PS: I'm going to push pre-commit to this repo for tf fmt and other lints. Please merge it to your PR when it's done.

3h4x · 2020-07-07T17:38:38Z

main.tf


  source_type = "db-instance"
-  source_ids  = ["${var.db_instance_id}"]
+  source_ids  = var.db_instance_ids

  event_categories = [


What is the reason behind removing other categories?
I think it would be best to preserve previous list as a default. To customize you can create event_categories variable of list(string) type.

3h4x · 2020-07-07T17:44:07Z

variables.tf

  default     = 64
 }

 variable "freeable_memory_threshold" {
  description = "The minimum amount of available random access memory in Byte."
-  type        = "string"
+  type        = string
  default     = 64000000



3h4x · 2020-07-07T17:50:39Z

main.tf

-resource "aws_sns_topic" "default" {
-  name_prefix = "rds-threshold-alerts"
-}
-
 resource "aws_db_event_subscription" "default" {
  name_prefix = "rds-event-sub"


Definitely we need a name prefix so all resources with names created by this module are aligned. Additionally I think rds-sns-alarms is more telling.

Suggested change

name_prefix = "rds-event-sub"

name_prefix = "${var.name_prefix}rds-sns-alarms"

3h4x · 2020-07-07T17:57:45Z

variables.tf

+}
+
+variable "aws_sns_topic_arn" {
+  description = "The bla of the SNS topic you want to use for alerting"


"The bla"? I guess you mean ARN

3h4x · 2020-07-07T17:59:37Z

alarms.tf

  metric_name         = "BurstBalance"
  namespace           = "AWS/RDS"
-  period              = "600"
+  period              = var.period


unit = "Percent"

3h4x · 2020-07-07T18:01:15Z

alarms.tf

  metric_name         = "DiskQueueDepth"
  namespace           = "AWS/RDS"
-  period              = "600"
+  period              = var.period


unit = "Count"

3h4x · 2020-07-07T18:03:29Z

alarms.tf

  metric_name         = "FreeableMemory"
  namespace           = "AWS/RDS"
-  period              = "600"
+  period              = var.period


unit = "Bytes"

3h4x · 2020-07-07T18:03:44Z

alarms.tf

  metric_name         = "FreeStorageSpace"
  namespace           = "AWS/RDS"
-  period              = "600"
+  period              = var.period


unit = "Bytes"

3h4x · 2020-07-07T18:04:00Z

alarms.tf

  metric_name         = "SwapUsage"
  namespace           = "AWS/RDS"
-  period              = "600"
+  period              = var.period


unit = "Bytes"

3h4x · 2020-07-07T18:12:09Z

alarms.tf

-  alarm_actions       = ["${aws_sns_topic.default.arn}"]
-  ok_actions          = ["${aws_sns_topic.default.arn}"]
+  threshold           = local.thresholds["BurstBalanceThreshold"]
+  alarm_description   = "Average database storage burst balance over last ${var.period/60} minutes too low, expect a significant performance drop soon"


Please add db_instance_id to every alarm_description.

caleb15 · 2020-09-15T23:07:05Z

Sorry for the delay on responding to the PR review, I've created a ticket so I remember to get to it eventually. It might be a while. Thanks for the review!

macnibblet · 2020-10-27T05:54:40Z

@caleb15 Any chance we might see an update :)?

caleb15 · 2020-10-27T17:40:22Z

@macnibblet yes, but it may take a while 🙈 I have the email starred in my inbox along with 30 others.

We only need failure event because we only know for sure that the user wants to be informed of a failure. Other events a user might not care about. Also there is already alarm for low storage.

this module is just for alarms we know it applies to alarms already so no need to state it

* Added rapid increase space alert * add rds free storage space alert * fix alarm variable value * add anomoly detection metric query Co-authored-by: jamesjsanders <[email protected]>

remove validate - it doesn't work

jamengual · 2021-01-09T04:33:24Z

main.tf

-    }
-  }
-}
+}


why all this have to be deleted?

Added datapoints_to_alarm requirement to free-storage alarm

needs set for for_each to work

tovbinm · 2021-04-15T04:41:38Z

@caleb15 how far are you from finishing this one up?

adamantike · 2021-04-15T13:13:16Z

Also waiting for this PR to be merged, to stop using a fork of this repo!

Gowiem · 2021-04-15T20:18:42Z

@maximmi are you still the owning migration to 0.12 PRs? Any chance you could check this one out?

Tf.13 update

maximmi

@caleb15 thank you for contribution
we have our standards for Terraform 0.12+ code, so that tests and github workflows should be in place. It might be hard to implement such things by you, so I will try to update your PR to respect all requirements.
For the reference you could pick any our repo with terraform 0.12+ support, for example: https://github.com/cloudposse/terraform-aws-s3-bucket

mergify · 2021-06-17T23:58:09Z

This pull request is now in conflict. Could you fix it @caleb15? 🙏

osterman requested a review from 3h4x July 7, 2020 17:24

3h4x suggested changes Jul 7, 2020

View reviewed changes

3h4x self-assigned this Jul 9, 2020

adamantike added a commit to adamantike/terraform-aws-rds-cloudwatch-sns-alarms that referenced this pull request Nov 20, 2020

Apply current state of PR cloudposse#21, commit 30bade8

412e2cd

Daniel Patriche and others added 19 commits January 3, 2021 12:11

Add unicity to alert names using instance ID (#5)

f1264eb

add vscode workspace to gitignore

6cb23fe

use sns topic user wants instead of creating new

2c04d01

get rid of unnecessary event categories

4fe363f

We only need failure event because we only know for sure that the user wants to be informed of a failure. Other events a user might not care about. Also there is already alarm for low storage.

allow multiple db instances

3d2e87d

create sns topic in examples

2b1c73c

add alarm name prefix

93bbaf2

add aws_sns_topic_arn to inputs

f3206dd

default alarm_name_prefix to empty

53921c8

add alarm_name_prefix variable

bf8c438

make alarm name prefix optional

817ac9a

upgrade to terraform v12

4cccb7a

fix dimensions

6bf6bd7

count not needed for source_ids

3d987c5

sns topic policy not needed

1d1246a

add myself to contributors

c9e66ee

update example in yaml

e57d2a6

update examples and readme to t12 syntax

690ba8f

add myself to contributors in readme.md

68fc54e

caleb15 and others added 4 commits January 3, 2021 12:13

move more important inputs up top

54d294d

add evaluation_periods and alarm_period variables

42cad8e

get rid of alarm_

00ecdf8

this module is just for alarms we know it applies to alarms already so no need to state it

Add Rapid Free Space Decrease Alert (#1)

718eb64

* Added rapid increase space alert * add rds free storage space alert * fix alarm variable value * add anomoly detection metric query Co-authored-by: jamesjsanders <[email protected]>

caleb15 force-pushed the master branch from 30bade8 to 718eb64 Compare January 4, 2021 17:43

caleb15 and others added 3 commits January 4, 2021 09:56

fix formatting

e732cc4

remove terraform validate - fails

0558181

Merge pull request #3 from 15five/remove_terraform_validate

14cf47a

remove validate - it doesn't work

jamengual reviewed Jan 9, 2021

View reviewed changes

main.tf

}

}

}

}

Copy link

jamengual Jan 9, 2021

Choose a reason for hiding this comment

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

why all this have to be deleted?

jamesjsanders added 2 commits February 25, 2021 16:03

Added datapoints_to_alarm requirement to free-storage alarm

b3f5e67

Merge pull request #4 from 15five/free-storage-alarm-fix

1cb4fa3

Added datapoints_to_alarm requirement to free-storage alarm

Gowiem requested a review from maximmi February 27, 2021 18:09

jamesjsanders and others added 5 commits March 16, 2021 20:58

Updated db_instance_ids count with local var for tf version 0.13.6

0c7ebe8

Updated db_instance_ids count with local var for tf version 0.13.6

4f9adc5

Fixed for_each change for tf version 0.13.6

3aeec29

Fixed for_each change for tf version 0.13.6

39938a3

switch to set

64d0cb6

needs set for for_each to work

Merge pull request #5 from 15five/tf.13-update

0053516

Tf.13 update

maximmi self-assigned this Apr 20, 2021

maximmi suggested changes Apr 20, 2021

View reviewed changes

hans-d closed this Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to be terraform .12 compatible & other changes #21

Upgrade to be terraform .12 compatible & other changes #21

caleb15 commented Jul 2, 2020 •

edited

Loading

jamengual commented Jul 2, 2020 •

edited

Loading

caleb15 commented Jul 7, 2020

jamengual commented Jul 7, 2020

nitrocode commented Jul 7, 2020

3h4x left a comment

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

3h4x Jul 7, 2020

caleb15 commented Sep 15, 2020 •

edited

Loading

macnibblet commented Oct 27, 2020

caleb15 commented Oct 27, 2020

jamengual Jan 9, 2021

tovbinm commented Apr 15, 2021

adamantike commented Apr 15, 2021

Gowiem commented Apr 15, 2021

maximmi left a comment

mergify bot commented Jun 17, 2021

	name_prefix = "rds-event-sub"
	name_prefix = "${var.name_prefix}rds-sns-alarms"

Upgrade to be terraform .12 compatible & other changes #21

Upgrade to be terraform .12 compatible & other changes #21

Conversation

caleb15 commented Jul 2, 2020 • edited Loading

jamengual commented Jul 2, 2020 • edited Loading

caleb15 commented Jul 7, 2020

jamengual commented Jul 7, 2020

nitrocode commented Jul 7, 2020

3h4x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleb15 commented Sep 15, 2020 • edited Loading

macnibblet commented Oct 27, 2020

caleb15 commented Oct 27, 2020

Choose a reason for hiding this comment

tovbinm commented Apr 15, 2021

adamantike commented Apr 15, 2021

Gowiem commented Apr 15, 2021

maximmi left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 17, 2021

caleb15 commented Jul 2, 2020 •

edited

Loading

jamengual commented Jul 2, 2020 •

edited

Loading

caleb15 commented Sep 15, 2020 •

edited

Loading