search

chaostoolkit-incubator / chaostoolkit-aws

Chaos Toolkit Extension for AWS
https://chaostoolkit.org/drivers/aws/
Apache License 2.0
109 stars 52 forks source link

Change probes.py cloudwatch #109

Closed javierfcamacho closed 3 years ago

javierfcamacho commented 3 years ago

chaostoolkit-aws/chaosaws/cloudwatch/probes.py get_metric_statistics / get_metric_data variables dimension_name: str and dimension_value: str are modified by a single variable dimensions: List [Dict [str, Any]]. Because a metric can contain multiple dimensions and if a specific combination of dimensions was not published, you can't retrieve statistics for it. https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_concepts.html#dimension-combinations

def get_metric_statistics(namespace: str, metric_name: str,
                          dimensions: List[Dict[str, Any]],
                          duration: int = 60, offset: int = 0,
                          statistic: str = None,
                          extended_statistic: str = None,
                          unit: str = None,
                          configuration: Configuration = None,
                          secrets: Secrets = None):

def get_metric_data(namespace: str, metric_name: str,
                    dimensions: List[Dict[str, Any]], statistic: str = None,
                    duration: int = 300, period: int = 60, offset: int = 0,
                    unit: str = None, configuration: Configuration = None,
                    secrets: Secrets = None) -> float:

Let me know your feedback.

ciaransweet commented 3 years ago

Hey @javierfcamacho thanks for the PR!

Rest assured, I'll be looking at this as soon as I can, right now we're still setup to use TravisCI and this isn't actually running any checks, I'm currently in the process of moving us over to GitHub Actions in #110 - Once that's in and I've made the failing tests in there pass, your PR will be my next thing to work on!

javierfcamacho commented 3 years ago

Hello @ciaranevans: Sure, no problem! Let me know your comments once you review it!

ciaransweet commented 3 years ago

Also @javierfcamacho could you please add a entry into CHANGELOG.md under the unreleased section, describing your change!

javierfcamacho commented 3 years ago

Also @javierfcamacho could you please add a entry into CHANGELOG.md under the unreleased section, describing your change! Re: Do it! Please let me know your feedback!

ciaransweet commented 3 years ago

Hi @javierfcamacho I'm not sure how you rebased your changes, but you've brought in changes not related to this Pull Request, for example all the GitHub actions changes are now in the master branch, they shouldn't be from your PR

javierfcamacho commented 3 years ago

Hey @ciaranevans, I did an update branch in my fork and re-clone the repo, after I applied my changes on this new version. It´s not ok? what I must to do for correct it?

ciaransweet commented 3 years ago

@javierfcamacho looks like you managed to sort the commits out, you're now getting test failures:

https://github.com/chaostoolkit-incubator/chaostoolkit-aws/pull/109/checks?check_run_id=3253675562

javierfcamacho commented 3 years ago

Hi @ciaranevans! I corrected some attributes into test_cloudwatch_probes.py, with this I hope the test failures will be corrected. Please, can you run the test again?

ciaransweet commented 3 years ago

@javierfcamacho adding to @Lawouach's comments https://github.com/chaostoolkit-incubator/chaostoolkit-aws/pull/109#pullrequestreview-724099979

I'd recommend either splitting out the dimensions version of the function into a new function - perhaps called get_metric_{type}_with_dimensions

Or:

We keep the current functions, instead allowing for dimension_name and dimension_value OR dimensions, with the former taking precedence to keep in with the current functionality.

Either way, like @Lawouach says, a WARNING statement alerting users to this new dimensions variable would be great.

Lawouach commented 3 years ago

I'm happy either way so long as we don't break compatibility yet.

javierfcamacho commented 3 years ago

Hey @ciaranevans @Lawouach, thanks for your recommendations, you're right! I'm going to work on your suggestions and once I have a solution I will commit my changes.

ciaransweet commented 3 years ago

@javierfcamacho Great! Thanks for being responsive to the comments! We would love to get your change in, breaking changes are just a no-no for us 😄

ciaransweet commented 3 years ago

Hey @javierfcamacho was there anything you needed a hand with?