Add basic addition support to cloudwatch.get_metric_data expressions #8416
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8416 +/- ##
==========================================
- Coverage 94.59% 94.59% -0.01%
==========================================
Files 1163 1163
Lines 101636 101661 +25
==========================================
+ Hits 96147 96170 +23
- Misses 5489 5491 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @sleepdeprecation, thank you for the PR!
Looking at the docs, these expressions can get quite advanced - so I don't think that this is a long-term solution.
If you mostly want the simple addition for now, then I'm willing to merge it (after the comments are addressed), but future extensions should use a proper parser, as that will make it much easier to extend/maintain the solution.
There are quite a few parsers out there, but my preference would be to use pyparsing, just because we're already using it in the Glue-module (see here) - so if we use that we don't have to add yet another dependency.
| expression = expression.replace(" ", "") | ||
| plus_splits = expression.split("+") | ||
| if len(plus_splits) != 2: | ||
| raise ValidationError( |
There was a problem hiding this comment.
This should fail silently. The result may be wrong, but that's preferable to throwing an exception and completely stopping people from using this feature.
I am writing some code that uses the Cloudwatch
GetMetricDataAPI's math expression feature, specifically in trying to add two metrics together.This PR adds support for adding two metrics together using the expression feature.
There are some additional features and validation I would like to add to this (specifically, checking that there aren't any other special characters, so that
m1 + m2 - m3would fail, and the ability to add more than two metrics together, exm1 + m2 + m3), but I wanted to open the PR at this point to see if this feature would be considered useful and check if this seems like a reasonable approach to the problem.