-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Amazon CloudWatch query runners #4372
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just have a couple of questions:
-
How do you decide if a helper function is inside or outside a class scope? For example, I'm not sure why
CloudWatch._get_client
is inside theCloudWatch
scope, butparse_response
is outside. I know it's not a big deal, but if it's just arbitrary, then a unified approach would make the code slightly easier to step into. -
How do you pick between list comprehensions and building with
for
loops? For example, thefor
block inparse_response
undercloudwatch.py
could easily be represented with a list comprehension. Again, not a big deal, but having a rule to follow (for example - 'prefer list comprehensions unless they look like a pile of manure') might make things easier.
Mostly habit thing. When the for loop is simple I use list comprehensions, but when it's multi line I usually do a for loop. |
Yeah, but encapsulating is about knowledge, not access to memory. |
I'm looking at it not at the class level, but the module level. There is also a benefit in testing: it makes it trivial to test |
@arikfr Hi mate, good job & great effort |
Hey @kangzj, the CloudWatch query runner documentation is underway. The associated issue is getredash/website#453. This data source is part of V9 which is still in beta. The doc should be ready before the full release. The short answer is that you don't use SQL to query CloudWatch. It uses a YAML query (similar to the JSON query source or MongoDB). |
What type of PR is this? (check all applicable)
Description
Query runner for Amazon CloudWatch and CloudWatch Insights.