Add support for Kinesis enhanced monitoring

[cbroglie]

Kinesis enhanced monitoring enables shard-level CloudWatch metrics for Kinesis streams. More information is available here.

I wasn't able to find any existing issue open for this feature, and am happy to provide a PR for it. My proposal is to add a new optional field to the aws_kinesis_stream resource called shard_level_metrics, which would be an array of shard level metrics to enable (see http://docs.aws.amazon.com/kinesis/latest/APIReference/API_EnableEnhancedMonitoring.html).

Let me know if this sounds reasonable, and if so I can implement it. Thanks.

[stack72]

Hi @cbroglie

Thanks for opening the issue here. You proposal sounds correct - it's similar to what i did for enabled_metrics in autoscaling groups. Have a look there at how that is constructed

thanks

Paul

[cbroglie]

Thanks @stack72, I got started on this with #7684. The basic functionality is working, but I'm not sure how to best handle the special ALL metric value. AWS automatically expands it the list of all available metrics, so it seems like the best way for Terraform to handle it would be to expand the list before talking to AWS. Is there any precedent for transforming input this way? I found https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go#L94, but it looks like can only be used to transform individual elements, not expand 1 element into multiple.

[stack72]

Hi @cbroglie

So from looking at the docs, ALL is actually a valid value. I think we should make that a viable entry for Terraform as well!

P

[cbroglie]

Sorry @stack72, I didn't explain the issue well enough. The problem is that if you call enable-enhanced-monitoring with the value ALL, and then call describe-stream to see which metrics are enabled, it doesn't return ALL. It instead returns the list of individual metrics that ALL expands to.

And this leads to tests failures like:

--- FAIL: TestAccAWSKinesisStream_shardLevelMetrics (151.77s)
    testing.go:264: Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: aws_kinesis_stream.test_stream
          shard_level_metrics.#:          "7" => "1"
          shard_level_metrics.123700221:  "IncomingBytes" => ""
          shard_level_metrics.1625074920: "OutgoingBytes" => ""
          shard_level_metrics.1878455758: "OutgoingRecords" => ""
          shard_level_metrics.2914988887: "" => "ALL"
          shard_level_metrics.2954412273: "ReadProvisionedThroughputExceeded" => ""
          shard_level_metrics.3672855597: "WriteProvisionedThroughputExceeded" => ""
          shard_level_metrics.4086450432: "IncomingRecords" => ""
          shard_level_metrics.589667364:  "IteratorAgeMilliseconds" => ""

        STATE:

        aws_kinesis_stream.test_stream:
          ID = arn:aws:kinesis:us-west-2:980871662884:stream/terraform-kinesis-test-2469637430723468560
          arn = arn:aws:kinesis:us-west-2:980871662884:stream/terraform-kinesis-test-2469637430723468560
          name = terraform-kinesis-test-2469637430723468560
          retention_period = 24
          shard_count = 2
          shard_level_metrics.# = 7
          shard_level_metrics.123700221 = IncomingBytes
          shard_level_metrics.1625074920 = OutgoingBytes
          shard_level_metrics.1878455758 = OutgoingRecords
          shard_level_metrics.2954412273 = ReadProvisionedThroughputExceeded
          shard_level_metrics.3672855597 = WriteProvisionedThroughputExceeded
          shard_level_metrics.4086450432 = IncomingRecords
          shard_level_metrics.589667364 = IteratorAgeMilliseconds
          tags.% = 1
          tags.Name = tf-test

Is there any other precedent for how to best handle this?

[stack72]

Hey

I would be tempted to have a small if statement - and if the value was "all" then create a []string of all of the shard_level_metrics to pass in

That way we are dealing with it locally rather than letting AWS transform the list for us

P.

[apparentlymart]

@stack72 I don't think that would entirely solve it, since Terraform core would still see that the config has "ALL" and detect a diff compared to what was read back from the API.

For this sort of problem we've usually taken one of two approaches:

• Use StateFunc to normalize how a value is stored in state and compared from config, to help Terraform understand when two things are functionally equivalent. We can't do that here because StateFunc only works on scalar types.
• Have the Read function "reverse-normalize" what comes back from the API so that the state matches what the user would've written in the config. That doesn't seem appropriate here because the meaning of "ALL" might change in future to include additional metrics that Terraform doesn't know about yet, and also we would then cause the reverse problem: if the user writes out all of the metrics individually then Terraform would want to change it back to "ALL".

The best idea I have right now is to make Terraform reject the "ALL" value altogether and encourage the user to explicitly list them all out. That way Terraform will be able to understand the user's intent better (e.g. it won't drift if AWS adds new values to the set) and produce reasonable diffs, at the expense of a slightly-more-annoying user interface.

[cbroglie]

Thanks @apparentlymart, those were the same paths I had considered and rejected for the same reasons. Disallowing ALL sounds like a reasonable solution to me, and I'll implement that unless there are any other objections.

[cbroglie]

@stack72 @apparentlymart Took another look at this and I can't see how to have Terraform actually reject the value ALL since ValidateFunc is not supported for lists.

That said, it does seem fairly trivial to add support for lists by making this change to schema.go:

diff --git a/helper/schema/schema.go b/helper/schema/schema.go
index b2934a7..f1051b0 100644
--- a/helper/schema/schema.go
+++ b/helper/schema/schema.go
@@ -1106,6 +1106,10 @@ func (m schemaMap) validateList(

        var ws []string
        var es []error
+       if schema.ValidateFunc != nil {
+               ws, es = schema.ValidateFunc(raws, k)
+       }
        for i, raw := range raws {
                key := fmt.Sprintf("%s.%d", k, i)

But I don't know enough to say whether it's really that simple or if there are other considerations to account for. Thoughts?

[stack72]

Hi @cbroglie

You are indeed correct, we do not have list validation. Personally, I would just add some good documentation around the allowed values for now. When List validation drops (at some point in the future) then we can revisit!

Paul

[cbroglie]

@stack72 Followed your advice and just documented that ALL should not be used. #7684 is now ready for review.

[stack72]

@cbroglie will get this reviewed and merged in the morning (I am UK timezone)

P.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.