Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bug/rds calculation is missing additional costs #143

Merged
merged 8 commits into from
Sep 6, 2020
Merged

Bug/rds calculation is missing additional costs #143

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9b63f83
RDS pricing fix.
cregev Aug 20, 2020
f18a930
Fix pricing
cregev Aug 20, 2020
51c4541
Bug fix.
cregev Aug 21, 2020
93178c9
Merge branch 'master' into bug/rds-calculation-is-missing-additional-…
cregev Aug 21, 2020
22c3f3b
Add more tests , more fixes for the bug.
cregev Aug 22, 2020
191af3a
Review notes.
cregev Sep 1, 2020
814cec5
Make test utils more generic.
cregev Sep 1, 2020
13ce6be
Merge branch 'master' into bug/rds-calculation-is-missing-additional-…
kaplanelad Sep 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 133 additions & 14 deletions collector/aws/resources/rds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"finala/collector/aws/register"
"finala/collector/config"
"finala/expression"
"fmt"
"time"

awsClient "github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -41,6 +42,14 @@ type DetectedAWSRDS struct {
collector.PriceDetectedFields
}

// RDSVolumeType will hold the available volume types for RDS types
var rdsStorageType = map[string]string{
"gp2": "General Purpose",
"standard": "Magnetic",
"io1": "Provisioned IOPS",
"aurora": "General Purpose-Aurora",
}

func init() {
register.Registry("rds", NewRDSManager)
}
Expand Down Expand Up @@ -77,6 +86,16 @@ func (r *RDSManager) Detect(metrics []config.MetricConfig) (interface{}, error)
r.awsManager.GetCollector().CollectStart(r.Name)

detected := []DetectedAWSRDS{}

pricingRegionPrefix, err := r.awsManager.GetPricingClient().GetRegionPrefix(r.awsManager.GetRegion())
if err != nil {
log.WithError(err).WithFields(log.Fields{
"region": r.awsManager.GetRegion(),
}).Error("Could not get pricing region prefix")
r.awsManager.GetCollector().CollectError(r.Name, err)
return detected, err
}

instances, err := r.describeInstances(nil, nil)
if err != nil {
log.WithField("error", err).Error("could not describe rds instances")
Expand All @@ -89,8 +108,6 @@ func (r *RDSManager) Detect(metrics []config.MetricConfig) (interface{}, error)

log.WithField("name", *instance.DBInstanceIdentifier).Debug("checking RDS")

price, _ := r.awsManager.GetPricingClient().GetPrice(r.getPricingFilterInput(instance), "", r.awsManager.GetRegion())

for _, metric := range metrics {
log.WithFields(log.Fields{
"name": *instance.DBInstanceIdentifier,
Expand Down Expand Up @@ -136,9 +153,50 @@ func (r *RDSManager) Detect(metrics []config.MetricConfig) (interface{}, error)
"formula_value": formulaValue,
"name": *instance.DBInstanceIdentifier,
"instance_type": *instance.DBInstanceClass,
"engine": *instance.Engine,
"region": r.awsManager.GetRegion(),
}).Info("RDS instance detected as unutilized resource")

instancePricingFilters := r.getPricingInstanceFilterInput(instance)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The price calculation really needs to be in the metrics loop?
the price can be changed when we have a different metric?

instancePrice, err := r.awsManager.GetPricingClient().GetPrice(instancePricingFilters, "", r.awsManager.GetRegion())
if err != nil {
log.WithError(err).Error("Could not get rds instance price")
continue
}

var hourlyStoragePrice float64
if rdsStorageType, found := rdsStorageType[*instance.StorageType]; found {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you say to move this logic into a function?
it will help us for better tests coverage + make Detect function to be more readable

var storagePricingFilters pricing.GetProductsInput
switch *instance.Engine {
case "aurora", "aurora-mysql", "aurora-postgresql":
storagePricingFilters = r.getPricingAuroraStorageFilterInput(rdsStorageType, pricingRegionPrefix)
default:
deploymentOption := r.getPricingDeploymentOption(instance)
storagePricingFilters = r.getPricingRDSStorageFilterInput(rdsStorageType, deploymentOption)
}

log.WithField("storage_filters", storagePricingFilters).Debug("pricing storage filters")
storagePrice, err := r.awsManager.GetPricingClient().GetPrice(storagePricingFilters, "", r.awsManager.GetRegion())
if err != nil {
log.WithError(err).Error("Could not get rds storage price")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you say to add the filter object into the log to improve the troubleshoot

continue
}

hourlyStoragePrice = (storagePrice * float64(*instance.AllocatedStorage)) / collector.TotalMonthHours
} else {
log.WithField("rds_storage_type", *instance.StorageType).Warn("Could not find RDS storage type")
continue
}

totalHourlyPrice := hourlyStoragePrice + instancePrice

log.WithFields(log.Fields{
"instance_hour_price": instancePrice,
"storage_hour_price": hourlyStoragePrice,
"total_hour_price": totalHourlyPrice,
"rds_AZ_multi": *instance.MultiAZ,
"region": r.awsManager.GetRegion()}).Debug("Found the following price list")

tags, err := r.client.ListTagsForResource(&rds.ListTagsForResourceInput{
ResourceName: instance.DBInstanceArn,
})
Expand All @@ -159,8 +217,8 @@ func (r *RDSManager) Detect(metrics []config.MetricConfig) (interface{}, error)
PriceDetectedFields: collector.PriceDetectedFields{
ResourceID: *instance.DBInstanceArn,
LaunchTime: *instance.InstanceCreateTime,
PricePerHour: price,
PricePerMonth: price * collector.TotalMonthHours,
PricePerHour: totalHourlyPrice,
PricePerMonth: totalHourlyPrice * collector.TotalMonthHours,
Tag: tagsData,
},
}
Expand All @@ -183,14 +241,34 @@ func (r *RDSManager) Detect(metrics []config.MetricConfig) (interface{}, error)
}

// getPricingFilterInput prepare document rds pricing filter
func (r *RDSManager) getPricingFilterInput(instance *rds.DBInstance) pricing.GetProductsInput {
func (r *RDSManager) getPricingInstanceFilterInput(instance *rds.DBInstance) pricing.GetProductsInput {

deploymentOption := "Single-AZ"
databaseEngine := r.getPricingDatabaseEngine(instance)
deploymentOption := r.getPricingDeploymentOption(instance)

if *instance.MultiAZ {
deploymentOption = "Multi-AZ"
return pricing.GetProductsInput{
ServiceCode: &r.servicePricingCode,
Filters: []*pricing.Filter{
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("databaseEngine"),
Value: &databaseEngine,
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("instanceType"),
Value: instance.DBInstanceClass,
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("deploymentOption"),
Value: &deploymentOption,
},
},
}
}

func (r *RDSManager) getPricingDatabaseEngine(instance *rds.DBInstance) string {
var databaseEngine string
switch *instance.Engine {
case "postgres":
Expand All @@ -202,28 +280,69 @@ func (r *RDSManager) getPricingFilterInput(instance *rds.DBInstance) pricing.Get
default:
databaseEngine = *instance.Engine
}
return databaseEngine
}

func (r *RDSManager) getPricingDeploymentOption(instance *rds.DBInstance) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a description on this function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not seeing testing on this function.
can you please add one?

deploymentOption := "Single-AZ"

if *instance.MultiAZ {
deploymentOption = "Multi-AZ"
}

return deploymentOption
}

func (r *RDSManager) getPricingRDSStorageFilterInput(rdsStorageType string, deploymentOption string) pricing.GetProductsInput {
cregev marked this conversation as resolved.
Show resolved Hide resolved

return pricing.GetProductsInput{
ServiceCode: &r.servicePricingCode,
Filters: []*pricing.Filter{
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("databaseEngine"),
Value: &databaseEngine,
Field: awsClient.String("volumeType"),
Value: awsClient.String(rdsStorageType),
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("instanceType"),
Value: instance.DBInstanceClass,
Field: awsClient.String("productFamily"),
Value: awsClient.String("Database Storage"),
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("termType"),
Value: awsClient.String("OnDemand"),
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("deploymentOption"),
Value: &deploymentOption,
Value: awsClient.String(deploymentOption),
},
},
}
}
func (r *RDSManager) getPricingAuroraStorageFilterInput(rdsStorageType string, pricingRegionPrefix string) pricing.GetProductsInput {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a description on this function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not seeing testing on this function.
can you please add one?


return pricing.GetProductsInput{
ServiceCode: &r.servicePricingCode,
Filters: []*pricing.Filter{
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("volumeType"),
Value: awsClient.String(rdsStorageType),
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("databaseEngine"),
Value: awsClient.String("Any"),
},
{
Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("usagetype"),
Value: awsClient.String(fmt.Sprintf("%sAurora:StorageUsage", pricingRegionPrefix)),
},
},
}
}

// describeInstances return list of rds instances
Expand All @@ -244,7 +363,7 @@ func (r *RDSManager) describeInstances(Marker *string, instances []*rds.DBInstan
}

for _, instance := range resp.DBInstances {
// Ignore DocumentDB and Neptune engine types as we have a seperate
// Ignore DocumentDB and Neptune engine types as we have a separate
// module for them and the default API call returns them
if *instance.Engine != "docdb" && *instance.Engine != "neptune" {
instances = append(instances, instance)
Expand Down
67 changes: 67 additions & 0 deletions collector/aws/resources/rds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ var defaultRDSMock = rds.DescribeDBInstancesOutput{
DBInstanceIdentifier: awsClient.String("i-1"),
MultiAZ: testutils.BoolPointer(true),
DBInstanceClass: awsClient.String("t2.micro"),
StorageType: awsClient.String("gp2"),
AllocatedStorage: awsClient.Int64(2),
Engine: awsClient.String("postgres"),
InstanceCreateTime: testutils.TimePointer(time.Now()),
},
Expand All @@ -29,6 +31,8 @@ var defaultRDSMock = rds.DescribeDBInstancesOutput{
DBInstanceIdentifier: awsClient.String("i-2"),
MultiAZ: testutils.BoolPointer(false),
DBInstanceClass: awsClient.String("t2.micro"),
StorageType: awsClient.String("aurora"),
AllocatedStorage: awsClient.Int64(4),
Engine: awsClient.String("aurora"),
InstanceCreateTime: testutils.TimePointer(time.Now()),
},
Expand All @@ -37,6 +41,8 @@ var defaultRDSMock = rds.DescribeDBInstancesOutput{
DBInstanceIdentifier: awsClient.String("i-3"),
MultiAZ: testutils.BoolPointer(false),
DBInstanceClass: awsClient.String("t2.micro"),
StorageType: awsClient.String("gp2"),
AllocatedStorage: awsClient.Int64(5),
Engine: awsClient.String("mysql"),
InstanceCreateTime: testutils.TimePointer(time.Now()),
},
Expand All @@ -45,6 +51,8 @@ var defaultRDSMock = rds.DescribeDBInstancesOutput{
DBInstanceIdentifier: awsClient.String("i-4"),
MultiAZ: testutils.BoolPointer(false),
DBInstanceClass: awsClient.String("t2.micro"),
StorageType: awsClient.String("gp2"),
AllocatedStorage: awsClient.Int64(8),
Engine: awsClient.String("docdb"),
InstanceCreateTime: testutils.TimePointer(time.Now()),
},
Expand All @@ -53,6 +61,8 @@ var defaultRDSMock = rds.DescribeDBInstancesOutput{
DBInstanceIdentifier: awsClient.String("i-5"),
MultiAZ: testutils.BoolPointer(false),
DBInstanceClass: awsClient.String("t2.micro"),
StorageType: awsClient.String("aurora"),
AllocatedStorage: awsClient.Int64(1),
Engine: awsClient.String("aurora-mysql"),
InstanceCreateTime: testutils.TimePointer(time.Now()),
},
Expand Down Expand Up @@ -163,6 +173,8 @@ func TestDetectRDS(t *testing.T) {
DBInstanceIdentifier: awsClient.String("i-1"),
MultiAZ: testutils.BoolPointer(true),
DBInstanceClass: awsClient.String("t2.micro"),
StorageType: awsClient.String("gp2"),
AllocatedStorage: awsClient.Int64(3),
Engine: awsClient.String("postgres"),
InstanceCreateTime: testutils.TimePointer(time.Now()),
},
Expand Down Expand Up @@ -201,3 +213,58 @@ func TestDetectRDS(t *testing.T) {
})

}

func TestGetPricingDatabaseEngine(t *testing.T) {
collector := collectorTestutils.NewMockCollector()
detector := awsTestutils.AWSManager(collector, nil, nil, "us-east-1")

mockClient := MockAWSRDSClient{
responseDescribeDBInstances: defaultRDSMock,
}

rdsInterface, err := NewRDSManager(detector, &mockClient)
if err != nil {
t.Fatalf("unexpected rds error happened, got %v expected %v", err, nil)
}

rdsManager, ok := rdsInterface.(*RDSManager)
if !ok {
t.Fatalf("unexpected rds struct, got %s expected %s", reflect.TypeOf(rdsInterface), "*RDSManager")
}

testResults := []string{"PostgreSQL", "Aurora MySQL", "mysql", "docdb", "Aurora MySQL"}

for index, instance := range defaultRDSMock.DBInstances {
databaseEngine := rdsManager.getPricingDatabaseEngine(instance)
if testResults[index] != databaseEngine {
t.Fatalf("unexpected database engine, got: %s expected: %s", databaseEngine, testResults[index])
}
}
}
func TestGetPricingDeploymentOption(t *testing.T) {
collector := collectorTestutils.NewMockCollector()
detector := awsTestutils.AWSManager(collector, nil, nil, "us-east-1")

mockClient := MockAWSRDSClient{
responseDescribeDBInstances: defaultRDSMock,
}

rdsInterface, err := NewRDSManager(detector, &mockClient)
if err != nil {
t.Fatalf("unexpected rds error happened, got %v expected %v", err, nil)
}

rdsManager, ok := rdsInterface.(*RDSManager)
if !ok {
t.Fatalf("unexpected rds struct, got %s expected %s", reflect.TypeOf(rdsInterface), "*RDSManager")
}

testResults := []string{"Multi-AZ", "Single-AZ", "Single-AZ", "Single-AZ", "Single-AZ"}

for index, instance := range defaultRDSMock.DBInstances {
deploymentOption := rdsManager.getPricingDeploymentOption(instance)
if testResults[index] != deploymentOption {
t.Fatalf("unexpected deployment option, got: %s expected: %s", deploymentOption, testResults[index])
}
}
}