search

cdklabs / cdk-monitoring-constructs

Easy-to-use CDK constructs for monitoring your AWS infrastructure
https://constructs.dev/packages/cdk-monitoring-constructs
Apache License 2.0
480 stars 70 forks source link

feat(ec2): Also plot EBS disk metrics when NITRO instance type is specified #321

Closed edisongustavo closed 1 year ago

edisongustavo commented 1 year ago

EC2 provides different metrics for Nitro-based instances. These metrics have the prefix EBS.

These instances either have the Disk* or EBSDisk* metrics, never both. Because of this I thought in using metric math where if any of the time series is empty, it would be removed, and then it would display a single metric. This gets trickier when monitoring an ASG because it can have both metrics there, so what should be done with the 2 time series? An average of them? If you think this would be a better idea, please advise so I change the implementation.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

edisongustavo commented 1 year ago

I'm not familiar with EC2/Nitro usage, so I'm not a good judge of what makes the most sense in this case. Do you have teammates that have any thoughts?

From what I understood, these metrics just have a different prefix. I don't know why EC2 chose to do it this way, but that's the way it is. The metrics without the prefix don't appear for Nitro based instances, and vice-versa.

That's why I mentioned doing some metric math do "merge" both metrics so that the dashboard has a single time-series to look at.

I thought in something like this:

m1 = EBSDiskRead
m2 = DiskRead
AVG(REMOVE_EMPTY([m1, m2]))

I don't know if it would work, but maybe it's worth a shot.

echeung-amzn commented 1 year ago

I'm not entirely sure either and don't have sample metrics to play around with manually in the console. Have you tried graphing metrics like that manually?

edisongustavo commented 1 year ago

Have you tried graphing metrics like that manually?

Yes, I've done that. It is as I said.

I am updating this PR with another commit which uses the MetricMath and in the end we have a single time series. This looks much nicer and doesn't require users of the library to change their code.

edisongustavo commented 1 year ago

This is the final graph:

image

As you can see, there are no metrics for "classic" because the AutoScalingGroup is only for "nitro-based" instances:

image