aws-cloudformation / aws-cloudformation-resource-providers-logs

The CloudFormation Resource Provider Package For Amazon CloudWatch Logs
https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/WhatIsCloudWatchLogs.html
Apache License 2.0
33 stars 35 forks source link

Remove callchain approach from MetricFilter resource type to reduce latency in stack operations #40

Closed miparnisari closed 4 years ago

miparnisari commented 4 years ago

Issue #, if available: https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-logs/issues/39

Description of changes:

Before this change:

image

With this change:

image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

PatMyron commented 4 years ago

Is this applicable to all resource types?

miparnisari commented 4 years ago

the latency could be reduced by removing read handler call at the end, call chain does not add additional latency. we should keep call chain

I tried that, I didn't get a major improvement. Tests still took 20 minutes to run

Is this applicable to all resource types?

No idea. This is a simple type, with no stabilization.

ammokhov commented 4 years ago

the latency could be reduced by removing read handler call at the end, call chain does not add additional latency. we should keep call chain

I tried that, I didn't get a major improvement. Tests still took 20 minutes to run

Is this applicable to all resource types?

No idea. This is a simple type, with no stabilization.

we can rework this together. KMS has lower latency than than usual by using the callchain

wbingli commented 4 years ago

I don't think callchain framework is suitable for resource without stabilization. It abstracts the re-invoke but because of the abstraction, it lacks the flexibility to control it. If we need to workaround and really careful to avoid pitfalls to make what we need, it takes more effort than this implementation, with simple request-response handling.

Same as for issue in (https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-logs/pull/42#issuecomment-653652724), it brings difficulty for development and debugging. To solve such issue, the developer eventually needs to understand the complex framework, it's not easy and over kill for a resource simple as this.

dchakrav-github commented 4 years ago

I don't think callchain framework is suitable for resource without stabilization. It abstracts the re-invoke but because of the abstraction, it lacks the flexibility to control it. If we need to workaround and really careful to avoid pitfalls to make what we need, it takes more effort than this implementation, with simple request-response handling.

Same as for issue in (#42 (comment)), it brings difficulty for development and debugging. To solve such issue, the developer eventually needs to understand the complex framework, it's not easy and over kill for a resource simple as this.

Did we ever remove the 60s wait time that was introduced to support handshake model for the original CWE, which had a minimum of 60s delay that was needed?