taskcluster / ec2-manager

Mozilla Public License 2.0
2 stars 14 forks source link

Calculate and update total EBS volume counts and GB usage per region, type, and status #41

Closed rstiyer closed 6 years ago

rstiyer commented 6 years ago

Outline

This PR combines the work that @SamanthaYu and @rstiyer have done individually in preliminary pull requests (here and here). We separated the tasks as follows:

@SamanthaYu:

@rstiyer:

Rachel's notes

I modified the AWS API call to request paginated data. The max page size is 500, and I don't see any reason not to use the maximum size. The 'describeVolumes' endpoint appears to return all data if you don't specify a page size, but we don't want to rely on this since many other endpoints only return the max page size.

The statsum series names suggested in the UCOSP document were of the form ebs-volumes:${region}:type:${type}:gb-usage, but I modified this to be more consistent with other series. I suggest ebs-volumes.${region}.type:${type}.gb-usage since the other series use periods in place of colons except for tagged measures. It might be good to tag each series with a volume status label as well, e.g.ebs-volumes.${region}.type:${type}.all-status.gb-usage and ebs-volumes.${region}.status:unused.gb-usage.

I have currently included only the statsum series that were mentioned in the UCOSP document, but it probably makes sense to include others. For instance, we might want to distinguish between active and unused volumes per type, per region by adding ebs-volumes.${region}.type:${type}.active.gb-usage and ebs-volumes.${region}.type:${type}.unused.gb-usage. It seems like the main use of these series is for tracking unused volumes, so I'm not sure if there is any value in tracking more metrics.

Unfortunately, it isn't possible to test the values of the series programatically, so it is not really possible to test the _submitVolumeTotals function. For this reason, I have added assert statements to the method itself to guarantee that the input to the method is provided in the expected format.

SamanthaYu commented 6 years ago

Maybe we could add a few _submitVolumeTotals() tests to check that it gets the correct global usage and count. I found out that the this object in _submitVolumeTotals()'s innermost foreach() loop was undefined when I added _calculateVolumeTotals(). Our tests without _calculateVolumeTotals() passed because _submitVolumeTotals() received an empty totalsByCategory object so the innermost foreach() loop never ran. To write these tests, we could stub out _calculateVolumeTotals() (e.g. Pass in nothing in _totalsByCategory; pass in some totals in various categories inside _totalsByCategory).

SamanthaYu commented 6 years ago

Also, sorry @jhford, @imbstack, and @djmitche for the long commit history! We weren't sure how to squash the commits and still preserve the commit history of who did what.

SamanthaYu commented 6 years ago

I also added the util module to housekeeping.js. The util module can print the entire object using console.log() instead of printing something like [object Object]; e.g. console.log(util.inspect(myObject, false, null));. I added this util module primarily for debugging purposes.

See https://nodejs.org/api/util.html#util_util_inspect_object_options for more information.

SamanthaYu commented 6 years ago

In the UCOSP requirements document, _calculateVolumeTotals() is supposed to return an object of the form: { $region: { $vol_type: {count: INT, gb: INT}}}. In order to get a series like ${region}.unused-gb-usage, _calculateVolumeTotals() must also categorize totals by region, type and status so we decided upon the following signature:

{
  ${region}: {
    ${type}: {
      active: {
        gb: value,
        count: value,
      },
      unused: {
        gb: value,
        count: value,
      }
    }, ...
  }, ...
}
jhford commented 6 years ago

Hey, i tried to rebase this patch onto origin/master and had some merge conflicts which I wasn't sure how to handle. The code looks good to me. Do you think you could rebase the code and I can deploy it to staging?

SamanthaYu commented 6 years ago

@jhford I just rebased origin/master into rstiyer/calculate-and-update-totals. I didn't get any merge conflicts so hopefully, I did rebase it properly.