-
Notifications
You must be signed in to change notification settings - Fork 1
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
Correct monitoring dimensions #55
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.
Looks good to my non-sre eyes!
memory_utilization = resource.name.apply( | ||
lambda res_name: aws.cloudwatch.MetricAlarm( | ||
memory_utilization = pulumi.Output.all(res_name=resource.name, cluster_arn=resource.cluster).apply( | ||
lambda outputs: aws.cloudwatch.MetricAlarm( |
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.
Should this be singular since it's acting on single output out of the all.apply, or is this a list of outputs?
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.
pulumi.Output.all
takes either a list or a dict of resources. When those resources' promised values become available, the lambda runs, and outputs
is either a list or a dict of values, depending on the input. So either way, outputs
should represent a plurality of values.
There is a different way of calling this if you only have one value you need to wait on, and that is just that output's apply
function, like resource.arn.apply
. So you wouldn't use this form of waiting on outputs to receive a singular value.
@@ -132,7 +132,7 @@ def __init__( | |||
f'{self.name}-5xx', | |||
name=f'{self.project.name_prefix}-5xx', | |||
comparison_operator='GreaterThanOrEqualToThreshold', | |||
dimensions={'LoadBalancer': f'app/{outputs['res_name']}/{outputs['res_suffix']}'}, | |||
dimensions={'LoadBalancer': outputs['res_suffix']}, |
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.
Since we use this more than once would be a good idea to pop this to a variable?
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.
I will evaluate this when I get back to this code today. I may have overlooked something while working through this bug.
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.
I looked at this a little bit, and here's the reason this isn't a separate variable (which would be cleaner were it so simple, I agree):
The variable outputs
doesn't exist outside of this lambda. The lambda itself can't be more than a single instruction. So if we want to rename this variable for clearer meaning, we also have to place this call to aws.cloudwatch.MetricAlarm
inside a separate function which the lambda calls. We do this in some places (like when the Fargate service declares a task definition, which is a complex structure that benefits from having this function with a bunch of inputs). If this is just a cosmetic issue, I'm not sure that makes things any clearer. I think I'm going to skip this one unless you feel very strongly about it.
This resolves Issue #49, where alarms always report "Insufficient data" because the dimensions given do not correctly identify the metrics.