From 8ad6e512c93d8380c86bf5f4baa5b52d5952cb0e Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Tue, 30 Jun 2020 12:06:04 +0300 Subject: [PATCH 01/11] Fix the pricing for ELB --- collector/aws/elb.go | 26 +++++-------------- collector/aws/elbv2.go | 59 +++++++++++++++++++++++------------------- 2 files changed, 40 insertions(+), 45 deletions(-) diff --git a/collector/aws/elb.go b/collector/aws/elb.go index 94bd3fc9..e4494504 100644 --- a/collector/aws/elb.go +++ b/collector/aws/elb.go @@ -51,7 +51,7 @@ func NewELBManager(collector collector.CollectorDescriber, client ELBClientDescr pricingClient: pricing, region: region, namespace: "AWS/ELB", - servicePricingCode: "AmazonEC2", + servicePricingCode: "AWSELB", Name: fmt.Sprintf("%s_elb", ResourcePrefix), } } @@ -109,7 +109,7 @@ func (el *ELBManager) Detect() ([]DetectedELB, error) { StartTime: &metricEndTime, EndTime: &now, Dimensions: []*cloudwatch.Dimension{ - &cloudwatch.Dimension{ + { Name: awsClient.String("LoadBalancerName"), Value: instance.LoadBalancerName, }, @@ -201,27 +201,15 @@ func (el *ELBManager) GetPricingFilterInput() *pricing.GetProductsInput { return &pricing.GetProductsInput{ ServiceCode: &el.servicePricingCode, Filters: []*pricing.Filter{ - - &pricing.Filter{ - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("usagetype"), - Value: awsClient.String("LoadBalancerUsage"), - }, - &pricing.Filter{ + { Type: awsClient.String("TERM_MATCH"), Field: awsClient.String("productFamily"), - Value: awsClient.String("Load Balancer-Application"), + Value: awsClient.String("Load Balancer"), }, - &pricing.Filter{ - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("TermType"), - Value: awsClient.String("OnDemand"), - }, - - &pricing.Filter{ + { Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("group"), - Value: awsClient.String("ELB:Balancer"), + Field: awsClient.String("groupDescription"), + Value: awsClient.String("LoadBalancer hourly usage by Classic Load Balancer"), }, }, } diff --git a/collector/aws/elbv2.go b/collector/aws/elbv2.go index 7d290698..1dd0f4d5 100644 --- a/collector/aws/elbv2.go +++ b/collector/aws/elbv2.go @@ -29,7 +29,6 @@ type ELBV2Manager struct { pricingClient *PricingManager metrics []config.MetricConfig region string - namespace string servicePricingCode string Name string } @@ -38,9 +37,15 @@ type ELBV2Manager struct { type DetectedELBV2 struct { Metric string Region string + Type string collector.PriceDetectedFields } +var loadBalancerCWNameSpace = map[string]string{ + "application": "AWS/ApplicationELB", + "network": "AWS/NetworkELB", +} + // NewELBV2Manager implements AWS GO SDK func NewELBV2Manager(collector collector.CollectorDescriber, client ELBV2ClientDescreptor, cloudWatchCLient *CloudwatchManager, pricing *PricingManager, metrics []config.MetricConfig, region string) *ELBV2Manager { @@ -51,8 +56,7 @@ func NewELBV2Manager(collector collector.CollectorDescriber, client ELBV2ClientD metrics: metrics, pricingClient: pricing, region: region, - namespace: "AWS/ApplicationELB", - servicePricingCode: "AmazonEC2", + servicePricingCode: "AWSELB", Name: fmt.Sprintf("%s_elbv2", ResourcePrefix), } } @@ -89,10 +93,13 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { now := time.Now() for _, instance := range instances { + var cloudWatchNameSpace string + if cloudWatchNS, found := loadBalancerCWNameSpace[*instance.Type]; found { + cloudWatchNameSpace = cloudWatchNS + } - log.WithField("name", *instance.LoadBalancerName).Debug("cheking elbV2") - - price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput(), "", el.region) + log.WithField("name", *instance.LoadBalancerName).Debug("checking elbV2") + price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput(instance), "", el.region) for _, metric := range el.metrics { @@ -110,13 +117,13 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { elbv2Name := regx.ReplaceAllString(*instance.LoadBalancerArn, "") metricInput := cloudwatch.GetMetricStatisticsInput{ - Namespace: &el.namespace, + Namespace: &cloudWatchNameSpace, MetricName: &metric.Description, Period: &period, StartTime: &metricEndTime, EndTime: &now, Dimensions: []*cloudwatch.Dimension{ - &cloudwatch.Dimension{ + { Name: awsClient.String("LoadBalancer"), Value: &elbv2Name, }, @@ -169,6 +176,7 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { elbv2 := DetectedELBV2{ Region: el.region, Metric: metric.Description, + Type: *instance.Type, PriceDetectedFields: collector.PriceDetectedFields{ ResourceID: *instance.LoadBalancerName, LaunchTime: *instance.CreatedTime, @@ -203,32 +211,31 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { } // GetPricingFilterInput prepare document elb pricing filter -func (el *ELBV2Manager) GetPricingFilterInput() *pricing.GetProductsInput { +func (el *ELBV2Manager) GetPricingFilterInput(loadbalancer *elbv2.LoadBalancer) *pricing.GetProductsInput { + + var loadBalancerProductFamily string + var loadBalancerGroupDescription string + switch *loadbalancer.Type { + case "application": + loadBalancerProductFamily = "Load Balancer-Application" + loadBalancerGroupDescription = "LoadBalancer hourly usage by Application Load Balancer" + case "network": + loadBalancerProductFamily = "Load Balancer-Network" + loadBalancerGroupDescription = "LoadBalancer hourly usage by Network Load Balancer" + } return &pricing.GetProductsInput{ ServiceCode: &el.servicePricingCode, Filters: []*pricing.Filter{ - - &pricing.Filter{ - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("usagetype"), - Value: awsClient.String("LoadBalancerUsage"), - }, - &pricing.Filter{ + { Type: awsClient.String("TERM_MATCH"), Field: awsClient.String("productFamily"), - Value: awsClient.String("Load Balancer-Application"), + Value: &loadBalancerProductFamily, }, - &pricing.Filter{ - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("TermType"), - Value: awsClient.String("OnDemand"), - }, - - &pricing.Filter{ + { Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("group"), - Value: awsClient.String("ELB:Balancer"), + Field: awsClient.String("groupDescription"), + Value: &loadBalancerGroupDescription, }, }, } From 02b940bfaf124aed5ac9746f122aba45f64fb2d6 Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Tue, 30 Jun 2020 13:42:49 +0300 Subject: [PATCH 02/11] Fix tests. --- collector/aws/elbv2_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/collector/aws/elbv2_test.go b/collector/aws/elbv2_test.go index 1f3e37f0..918e6cc9 100644 --- a/collector/aws/elbv2_test.go +++ b/collector/aws/elbv2_test.go @@ -14,7 +14,8 @@ import ( var defaultELBV2Mock = elbv2.DescribeLoadBalancersOutput{ LoadBalancers: []*elbv2.LoadBalancer{ - &elbv2.LoadBalancer{ + { + Type: awsClient.String("application"), LoadBalancerName: awsClient.String("i-1"), LoadBalancerArn: awsClient.String("i-1"), CreatedTime: testutils.TimePointer(time.Now()), From ce153d5d9e8523318d4c9008b313273300661a1f Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Fri, 3 Jul 2020 10:56:49 +0300 Subject: [PATCH 03/11] ELBV2 use pricing term of usagetype instead of description. --- collector/aws/elbv2.go | 92 +++++++++++++++++++++++++-------------- collector/aws/pricing.go | 93 +++++++++++++++++++++++++--------------- 2 files changed, 118 insertions(+), 67 deletions(-) diff --git a/collector/aws/elbv2.go b/collector/aws/elbv2.go index 1dd0f4d5..2927cf04 100644 --- a/collector/aws/elbv2.go +++ b/collector/aws/elbv2.go @@ -33,7 +33,7 @@ type ELBV2Manager struct { Name string } -// DetectedELBV2 define the detected AWS ELB instances +// DetectedELBV2 defines the detected AWS ELB instances type DetectedELBV2 struct { Metric string Region string @@ -41,9 +41,33 @@ type DetectedELBV2 struct { collector.PriceDetectedFields } -var loadBalancerCWNameSpace = map[string]string{ - "application": "AWS/ApplicationELB", - "network": "AWS/NetworkELB", +// loadBalancerConfig defines loadbalancer's configuration of metrics and pricing +type loadBalancerConfig struct { + cloudWatchNamespace string + pricingfilters []*pricing.Filter +} + +var loadBalancersConfig = map[string]loadBalancerConfig{ + "application": { + cloudWatchNamespace: "AWS/ApplicationELB", + pricingfilters: []*pricing.Filter{ + { + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("productFamily"), + Value: awsClient.String("Load Balancer-Application"), + }, + }, + }, + "network": { + cloudWatchNamespace: "AWS/NetworkELB", + pricingfilters: []*pricing.Filter{ + { + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("productFamily"), + Value: awsClient.String("Load Balancer-Network"), + }, + }, + }, } // NewELBV2Manager implements AWS GO SDK @@ -94,13 +118,28 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { for _, instance := range instances { var cloudWatchNameSpace string - if cloudWatchNS, found := loadBalancerCWNameSpace[*instance.Type]; found { - cloudWatchNameSpace = cloudWatchNS - } + var price float64 + if loadBalancerConfig, found := loadBalancersConfig[*instance.Type]; found { + cloudWatchNameSpace = loadBalancerConfig.cloudWatchNamespace - log.WithField("name", *instance.LoadBalancerName).Debug("checking elbV2") - price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput(instance), "", el.region) + if cloudWatchNameSpace == "" { + log.WithError(err).WithFields(log.Fields{ + "name": *instance.LoadBalancerName, + "type": *instance.Type, + }).Error("Could not get CloudWatch namespace because of unknown loadbalancer type") + continue + } + log.WithField("name", *instance.LoadBalancerName).Debug("checking elbV2") + pricingRegionPrefix := el.pricingClient.GetRegionPrefix(el.region) + loadBalancerConfig.pricingfilters = append( + loadBalancerConfig.pricingfilters, &pricing.Filter{ + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("usagetype"), + Value: awsClient.String(fmt.Sprintf("%sLoadBalancerUsage", pricingRegionPrefix)), + }) + price, _ = el.pricingClient.GetPrice(el.GetPricingFilterInput(loadBalancerConfig.pricingfilters), "", el.region) + } for _, metric := range el.metrics { log.WithFields(log.Fields{ @@ -211,33 +250,22 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { } // GetPricingFilterInput prepare document elb pricing filter -func (el *ELBV2Manager) GetPricingFilterInput(loadbalancer *elbv2.LoadBalancer) *pricing.GetProductsInput { - - var loadBalancerProductFamily string - var loadBalancerGroupDescription string - switch *loadbalancer.Type { - case "application": - loadBalancerProductFamily = "Load Balancer-Application" - loadBalancerGroupDescription = "LoadBalancer hourly usage by Application Load Balancer" - case "network": - loadBalancerProductFamily = "Load Balancer-Network" - loadBalancerGroupDescription = "LoadBalancer hourly usage by Network Load Balancer" +func (el *ELBV2Manager) GetPricingFilterInput(extraFilters []*pricing.Filter) *pricing.GetProductsInput { + filters := []*pricing.Filter{ + { + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("termType"), + Value: awsClient.String("OnDemand"), + }, + } + + if extraFilters != nil { + filters = append(filters, extraFilters...) } return &pricing.GetProductsInput{ ServiceCode: &el.servicePricingCode, - Filters: []*pricing.Filter{ - { - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("productFamily"), - Value: &loadBalancerProductFamily, - }, - { - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("groupDescription"), - Value: &loadBalancerGroupDescription, - }, - }, + Filters: filters, } } diff --git a/collector/aws/pricing.go b/collector/aws/pricing.go index 1c1cd84c..92460698 100644 --- a/collector/aws/pricing.go +++ b/collector/aws/pricing.go @@ -19,32 +19,38 @@ const ( defaultRateCode = "6YS6EN2CT7" ) -var regionToLocation = map[string]string{ - "us-east-2": "US East (Ohio)", - "us-east-1": "US East (N. Virginia)", - "us-west-1": "US West (N. California)", - "us-west-2": "US West (Oregon)", - "ap-east-1": "Asia Pacific (Hong Kong)", - "ap-south-1": "Asia Pacific (Mumbai)", - "ap-northeast-3": "Asia Pacific (Osaka-Local)", - "ap-northeast-2": "Asia Pacific (Seoul)", - "ap-southeast-1": "Asia Pacific (Singapore)", - "ap-southeast-2": "Asia Pacific (Sydney)", - "ap-northeast-1": "Asia Pacific (Tokyo)", - "ca-central-1": "Canada (Central)", - "cn-north-1": "China (Beijing)", - "cn-northwest-1": "China (Ningxia)", - "eu-central-1": "EU (Frankfurt)", - "eu-west-1": "EU (Ireland)", - "eu-west-2": "EU (London)", - "eu-west-3": "EU (Paris)", - "eu-south-1": "EU (Milan)", - "eu-north-1": "EU (Stockholm)", - "sa-east-1": "South America (Sao Paulo)", - "us-gov-east-1": "AWS GovCloud (US-East)", - "us-gov-west-1": "AWS GovCloud (US)", - "af-south-1": "Africa (Cape Town)", - "me-south-1": "Middle East (Bahrain)", +// regionInfo will hold data about a region pricing options +type regionInfo struct { + fullName string + prefix string +} + +var regionsInfo = map[string]regionInfo{ + "us-east-2": {fullName: "US East (Ohio)", prefix: "USE2"}, + "us-east-1": {fullName: "US East (N. Virginia)", prefix: ""}, + "us-west-1": {fullName: "US West (N. California)", prefix: "USW1"}, + "us-west-2": {fullName: "US West (Oregon)", prefix: "USW2"}, + "ap-east-1": {fullName: "Asia Pacific (Hong Kong)", prefix: "APE1"}, + "ap-south-1": {fullName: "Asia Pacific (Mumbai)", prefix: ""}, + "ap-northeast-3": {fullName: "Asia Pacific (Osaka-Local)", prefix: "APN3"}, + "ap-northeast-2": {fullName: "Asia Pacific (Seoul)", prefix: "APN2"}, + "ap-southeast-1": {fullName: "Asia Pacific (Singapore)", prefix: "APS1"}, + "ap-southeast-2": {fullName: "Asia Pacific (Sydney)", prefix: "APS2"}, + "ap-northeast-1": {fullName: "Asia Pacific (Tokyo)", prefix: "APN1"}, + "ca-central-1": {fullName: "Canada (Central)", prefix: "CAN1"}, + "cn-north-1": {fullName: "China (Beijing)", prefix: ""}, + "cn-northwest-1": {fullName: "China (Ningxia)", prefix: ""}, + "eu-central-1": {fullName: "EU (Frankfurt)", prefix: "EUC1"}, + "eu-west-1": {fullName: "EU (Ireland)", prefix: "EUW1"}, + "eu-west-2": {fullName: "EU (London)", prefix: "EUW2"}, + "eu-west-3": {fullName: "EU (Paris)", prefix: "EUW3"}, + "eu-south-1": {fullName: "EU (Milan)", prefix: "EUS1"}, + "eu-north-1": {fullName: "EU (Stockholm)", prefix: "EUN1"}, + "sa-east-1": {fullName: "South America (Sao Paulo)", prefix: "SAE1"}, + "us-gov-east-1": {fullName: "AWS GovCloud (US-East)", prefix: "UGE1"}, + "us-gov-west-1": {fullName: "AWS GovCloud (US)", prefix: "UGW1"}, + "af-south-1": {fullName: "Africa (Cape Town)", prefix: "AFS1"}, + "me-south-1": {fullName: "Middle East (Bahrain)", prefix: "MES1"}, } // PricingClientDescreptor is an interface defining the aws pricing client @@ -95,7 +101,7 @@ type PriceCurrencyCode struct { // NewPricingManager implements AWS GO SDK func NewPricingManager(client PricingClientDescreptor, region string) *PricingManager { - log.Debug("Init aws pricing SDK client") + log.Debug("Initializing aws pricing SDK client") return &PricingManager{ client: client, region: region, @@ -103,22 +109,24 @@ func NewPricingManager(client PricingClientDescreptor, region string) *PricingMa } } -// GetPrice return the product price by given product filter -// The result (of the given product input) should be only one product. +// GetPrice returns the product price filtered by product filters +// The result (of the given product input) should be only one product as a specific product with specific usage +// Should have only 1 price to calculate total price func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode string, region string) (float64, error) { if rateCode == "" { rateCode = defaultRateCode } - location, found := regionToLocation[region] + + regionInfo, found := regionsInfo[region] if !found { - return 0, errors.New("Given region not found") + return 0, errors.New("Given region was not found") } input.Filters = append(input.Filters, &pricing.Filter{ Type: awsClient.String("TERM_MATCH"), Field: awsClient.String("location"), - Value: awsClient.String(location), + Value: awsClient.String(regionInfo.fullName), }) hash, err := hashstructure.Hash(input, nil) @@ -145,7 +153,7 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri "products": len(priceResponse.PriceList), }).Error("Price list response should be equal to 1 product") - return 0, errors.New(fmt.Sprint("Pricelice response should be equal to 1 product")) + return 0, errors.New(fmt.Sprint("Price list response should be equal only to 1 product")) } product := priceResponse.PriceList[0] @@ -155,7 +163,7 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri log.WithError(err).WithFields(log.Fields{ "search_query": input, "product": product, - }).Error("could not encoded JSON value") + }).Error("could not encode JSON value") return 0, err } @@ -177,7 +185,7 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri log.WithError(err).WithFields(log.Fields{ "search_query": input, "product": product, - }).Error("could not pare USD price from string to float64") + }).Error("could not parse USD price from string to float64") return 0, err } @@ -189,3 +197,18 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri }).Debug("AWS resource price was found") return price, nil } + +// GetRegionPrefix will return the prefix for a +// pricing filter value according to a given region. +// For example: +// Region: "us-east-2" prefix will be: "USE2-" +func (p *PricingManager) GetRegionPrefix(region string) string { + var prefix string + switch regionsInfo[region].prefix { + case "": + prefix = "" + default: + prefix = fmt.Sprintf("%s-", regionsInfo[region].prefix) + } + return prefix +} From 793612516cc8ca4a12d2fce8110f149351a3aee3 Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Sun, 5 Jul 2020 12:05:12 +0300 Subject: [PATCH 04/11] Add tests for a new pricing function. --- collector/aws/pricing_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/collector/aws/pricing_test.go b/collector/aws/pricing_test.go index 552063c9..6249169e 100644 --- a/collector/aws/pricing_test.go +++ b/collector/aws/pricing_test.go @@ -98,3 +98,24 @@ func TestGetPrice(t *testing.T) { }) } + +func TestGetRegionPrefix(t *testing.T) { + testCases := []struct { + region string + expectedPrefix string + }{ + {"us-east-1", ""}, + {"us-west-1", "USW1-"}, + {"ap-northeast-2", "APN2-"}, + } + + pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") + for _, tc := range testCases { + t.Run(tc.region, func(t *testing.T) { + pricingValuePrefix := pricingManager.GetRegionPrefix(tc.region) + if tc.expectedPrefix != pricingValuePrefix { + t.Fatalf("unexpected pricing value prefix, got: %s, expected: %s", pricingValuePrefix, tc.expectedPrefix) + } + }) + } +} From 52753fc102609da9c62556bc012e3b79969c0fe4 Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Sun, 5 Jul 2020 12:15:07 +0300 Subject: [PATCH 05/11] use more specific filters for elb and add usage type. --- collector/aws/elb.go | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/collector/aws/elb.go b/collector/aws/elb.go index e4494504..b8fb4cbc 100644 --- a/collector/aws/elb.go +++ b/collector/aws/elb.go @@ -90,8 +90,14 @@ func (el *ELBManager) Detect() ([]DetectedELB, error) { for _, instance := range instances { log.WithField("name", *instance.LoadBalancerName).Debug("checking elb") - - price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput(), "", el.region) + pricingRegionPrefix := el.pricingClient.GetRegionPrefix(el.region) + price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput([]*pricing.Filter{ + { + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("usagetype"), + Value: awsClient.String(fmt.Sprintf("%sLoadBalancerUsage", pricingRegionPrefix)), + }, + }), "", el.region) for _, metric := range el.metrics { @@ -196,22 +202,28 @@ func (el *ELBManager) Detect() ([]DetectedELB, error) { } // GetPricingFilterInput prepare document elb pricing filter -func (el *ELBManager) GetPricingFilterInput() *pricing.GetProductsInput { +func (el *ELBManager) GetPricingFilterInput(extraFilters []*pricing.Filter) *pricing.GetProductsInput { + + filters := []*pricing.Filter{ + { + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("termType"), + Value: awsClient.String("OnDemand"), + }, + { + Type: awsClient.String("TERM_MATCH"), + Field: awsClient.String("productFamily"), + Value: awsClient.String("Load Balancer"), + }, + } + + if extraFilters != nil { + filters = append(filters, extraFilters...) + } return &pricing.GetProductsInput{ ServiceCode: &el.servicePricingCode, - Filters: []*pricing.Filter{ - { - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("productFamily"), - Value: awsClient.String("Load Balancer"), - }, - { - Type: awsClient.String("TERM_MATCH"), - Field: awsClient.String("groupDescription"), - Value: awsClient.String("LoadBalancer hourly usage by Classic Load Balancer"), - }, - }, + Filters: filters, } } From f37ee696cad4b41d50451ca660853d11c5e82120 Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Sun, 5 Jul 2020 14:57:05 +0300 Subject: [PATCH 06/11] Fix review comments. --- collector/aws/elb.go | 9 ++++++++- collector/aws/elbv2.go | 17 ++++++++--------- collector/aws/pricing.go | 13 +++++++++---- collector/aws/pricing_test.go | 14 +++++++++++--- 4 files changed, 36 insertions(+), 17 deletions(-) diff --git a/collector/aws/elb.go b/collector/aws/elb.go index b8fb4cbc..0643520d 100644 --- a/collector/aws/elb.go +++ b/collector/aws/elb.go @@ -88,9 +88,16 @@ func (el *ELBManager) Detect() ([]DetectedELB, error) { now := time.Now() + pricingRegionPrefix, err := el.pricingClient.GetRegionPrefix(el.region) + if err != nil { + log.WithError(err).WithFields(log.Fields{ + "region": el.region, + }).Error("Could not get pricing region prefix") + return detectedELB, err + } + for _, instance := range instances { log.WithField("name", *instance.LoadBalancerName).Debug("checking elb") - pricingRegionPrefix := el.pricingClient.GetRegionPrefix(el.region) price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput([]*pricing.Filter{ { Type: awsClient.String("TERM_MATCH"), diff --git a/collector/aws/elbv2.go b/collector/aws/elbv2.go index 2927cf04..07f3ee0a 100644 --- a/collector/aws/elbv2.go +++ b/collector/aws/elbv2.go @@ -115,6 +115,13 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { } now := time.Now() + pricingRegionPrefix, err := el.pricingClient.GetRegionPrefix(el.region) + if err != nil { + log.WithError(err).WithFields(log.Fields{ + "region": el.region, + }).Error("Could not get pricing region prefix") + return detectedELBV2, err + } for _, instance := range instances { var cloudWatchNameSpace string @@ -122,16 +129,8 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { if loadBalancerConfig, found := loadBalancersConfig[*instance.Type]; found { cloudWatchNameSpace = loadBalancerConfig.cloudWatchNamespace - if cloudWatchNameSpace == "" { - log.WithError(err).WithFields(log.Fields{ - "name": *instance.LoadBalancerName, - "type": *instance.Type, - }).Error("Could not get CloudWatch namespace because of unknown loadbalancer type") - continue - } - log.WithField("name", *instance.LoadBalancerName).Debug("checking elbV2") - pricingRegionPrefix := el.pricingClient.GetRegionPrefix(el.region) + loadBalancerConfig.pricingfilters = append( loadBalancerConfig.pricingfilters, &pricing.Filter{ Type: awsClient.String("TERM_MATCH"), diff --git a/collector/aws/pricing.go b/collector/aws/pricing.go index 92460698..4e8d55c2 100644 --- a/collector/aws/pricing.go +++ b/collector/aws/pricing.go @@ -153,7 +153,7 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri "products": len(priceResponse.PriceList), }).Error("Price list response should be equal to 1 product") - return 0, errors.New(fmt.Sprint("Price list response should be equal only to 1 product")) + return 0, errors.New("Price list response should be equal only to 1 product") } product := priceResponse.PriceList[0] @@ -202,13 +202,18 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri // pricing filter value according to a given region. // For example: // Region: "us-east-2" prefix will be: "USE2-" -func (p *PricingManager) GetRegionPrefix(region string) string { +func (p *PricingManager) GetRegionPrefix(region string) (string, error) { var prefix string - switch regionsInfo[region].prefix { + regionInfo, found := regionsInfo[region] + if !found { + return "", errors.New("region was not found as part of the regionsInfo map") + } + + switch regionInfo.prefix { case "": prefix = "" default: prefix = fmt.Sprintf("%s-", regionsInfo[region].prefix) } - return prefix + return prefix, nil } diff --git a/collector/aws/pricing_test.go b/collector/aws/pricing_test.go index 6249169e..48da2d6d 100644 --- a/collector/aws/pricing_test.go +++ b/collector/aws/pricing_test.go @@ -107,14 +107,22 @@ func TestGetRegionPrefix(t *testing.T) { {"us-east-1", ""}, {"us-west-1", "USW1-"}, {"ap-northeast-2", "APN2-"}, + {"bla", ""}, } pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") for _, tc := range testCases { t.Run(tc.region, func(t *testing.T) { - pricingValuePrefix := pricingManager.GetRegionPrefix(tc.region) - if tc.expectedPrefix != pricingValuePrefix { - t.Fatalf("unexpected pricing value prefix, got: %s, expected: %s", pricingValuePrefix, tc.expectedPrefix) + if tc.region != "bla" { + pricingValuePrefix, _ := pricingManager.GetRegionPrefix(tc.region) + if tc.expectedPrefix != pricingValuePrefix { + t.Fatalf("unexpected pricing value prefix, got: %s, expected: %s", pricingValuePrefix, tc.expectedPrefix) + } + } else { + _, err := pricingManager.GetRegionPrefix(tc.region) + if err == nil { + t.Fatalf("expected an error for getRegionPrefix") + } } }) } From 242b5d9430e36feb76d8dfd458561d626b0fad85 Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Sun, 5 Jul 2020 15:01:10 +0300 Subject: [PATCH 07/11] fix go fmt. --- collector/aws/elb.go | 1 - collector/aws/elbv2.go | 1 - 2 files changed, 2 deletions(-) diff --git a/collector/aws/elb.go b/collector/aws/elb.go index be08c923..8a07a3bb 100644 --- a/collector/aws/elb.go +++ b/collector/aws/elb.go @@ -230,7 +230,6 @@ func (el *ELBManager) GetPricingFilterInput(extraFilters []*pricing.Filter) *pri return &pricing.GetProductsInput{ ServiceCode: &el.servicePricingCode, Filters: filters, - } } diff --git a/collector/aws/elbv2.go b/collector/aws/elbv2.go index 426cc5c8..9ec0d84f 100644 --- a/collector/aws/elbv2.go +++ b/collector/aws/elbv2.go @@ -265,7 +265,6 @@ func (el *ELBV2Manager) GetPricingFilterInput(extraFilters []*pricing.Filter) *p return &pricing.GetProductsInput{ ServiceCode: &el.servicePricingCode, Filters: filters, - } } From 65e26a5cf6244c646cc02c52796215d7da8c700d Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Sun, 5 Jul 2020 21:51:22 +0300 Subject: [PATCH 08/11] Fix pricing tests , move region prefix logic to be the first item of detect. --- collector/aws/elb.go | 35 ++++++++++++++++++++--------------- collector/aws/elbv2.go | 35 ++++++++++++++++++++++------------- collector/aws/pricing.go | 7 +++++-- collector/aws/pricing_test.go | 25 +++++++++++-------------- 4 files changed, 58 insertions(+), 44 deletions(-) diff --git a/collector/aws/elb.go b/collector/aws/elb.go index 8a07a3bb..dd64f18b 100644 --- a/collector/aws/elb.go +++ b/collector/aws/elb.go @@ -73,29 +73,23 @@ func (el *ELBManager) Detect() ([]DetectedELB, error) { detectedELB := []DetectedELB{} - instances, err := el.DescribeLoadbalancers(nil, nil) - if err != nil { - - el.collector.UpdateServiceStatus(collector.EventCollector{ - ResourceName: el.Name, - Data: collector.EventStatusData{ - Status: collector.EventError, - ErrorMessage: err.Error(), - }, - }) - return detectedELB, err - } - - now := time.Now() - pricingRegionPrefix, err := el.pricingClient.GetRegionPrefix(el.region) if err != nil { log.WithError(err).WithFields(log.Fields{ "region": el.region, }).Error("Could not get pricing region prefix") + el.updateErrorServiceStatus(err) return detectedELB, err } + instances, err := el.DescribeLoadbalancers(nil, nil) + if err != nil { + el.updateErrorServiceStatus(err) + return detectedELB, err + } + + now := time.Now() + for _, instance := range instances { log.WithField("name", *instance.LoadBalancerName).Debug("checking elb") price, _ := el.pricingClient.GetPrice(el.GetPricingFilterInput([]*pricing.Filter{ @@ -258,3 +252,14 @@ func (el *ELBManager) DescribeLoadbalancers(marker *string, loadbalancers []*elb return loadbalancers, nil } + +// updateErrorServiceStatus reports when elb can't collect data +func (el *ELBManager) updateErrorServiceStatus(err error) { + el.collector.UpdateServiceStatus(collector.EventCollector{ + ResourceName: el.Name, + Data: collector.EventStatusData{ + Status: collector.EventError, + ErrorMessage: err.Error(), + }, + }) +} diff --git a/collector/aws/elbv2.go b/collector/aws/elbv2.go index 9ec0d84f..bf83ff39 100644 --- a/collector/aws/elbv2.go +++ b/collector/aws/elbv2.go @@ -47,6 +47,8 @@ type loadBalancerConfig struct { pricingfilters []*pricing.Filter } +// loadBalancersConfig defines loadbalancers configuration of metrics and pricing for +// Multiple types of LoadBalancers. var loadBalancersConfig = map[string]loadBalancerConfig{ "application": { cloudWatchNamespace: "AWS/ApplicationELB", @@ -102,27 +104,23 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) { detectedELBV2 := []DetectedELBV2{} - instances, err := el.DescribeLoadbalancers(nil, nil) - if err != nil { - el.collector.UpdateServiceStatus(collector.EventCollector{ - ResourceName: el.Name, - Data: collector.EventStatusData{ - Status: collector.EventError, - ErrorMessage: err.Error(), - }, - }) - return detectedELBV2, err - } - - now := time.Now() pricingRegionPrefix, err := el.pricingClient.GetRegionPrefix(el.region) if err != nil { log.WithError(err).WithFields(log.Fields{ "region": el.region, }).Error("Could not get pricing region prefix") + el.updateErrorServiceStatus(err) return detectedELBV2, err } + instances, err := el.DescribeLoadbalancers(nil, nil) + if err != nil { + el.updateErrorServiceStatus(err) + return detectedELBV2, err + } + + now := time.Now() + for _, instance := range instances { var cloudWatchNameSpace string var price float64 @@ -293,3 +291,14 @@ func (el *ELBV2Manager) DescribeLoadbalancers(marker *string, loadbalancers []*e return loadbalancers, nil } + +// updateErrorServiceStatus reports when elbv2 can't collect data +func (el *ELBV2Manager) updateErrorServiceStatus(err error) { + el.collector.UpdateServiceStatus(collector.EventCollector{ + ResourceName: el.Name, + Data: collector.EventStatusData{ + Status: collector.EventError, + ErrorMessage: err.Error(), + }, + }) +} diff --git a/collector/aws/pricing.go b/collector/aws/pricing.go index 1555ce13..8b85a7e8 100644 --- a/collector/aws/pricing.go +++ b/collector/aws/pricing.go @@ -19,6 +19,9 @@ const ( defaultRateCode = "6YS6EN2CT7" ) +// ErrRegionNotFound when a region is not found +var ErrRegionNotFound = errors.New("region was not found as part of the regionsInfo map") + // regionInfo will hold data about a region pricing options type regionInfo struct { fullName string @@ -120,7 +123,7 @@ func (p *PricingManager) GetPrice(input *pricing.GetProductsInput, rateCode stri regionInfo, found := regionsInfo[region] if !found { - return 0, errors.New("Given region was not found") + return 0, ErrRegionNotFound } input.Filters = append(input.Filters, &pricing.Filter{ @@ -205,7 +208,7 @@ func (p *PricingManager) GetRegionPrefix(region string) (string, error) { var prefix string regionInfo, found := regionsInfo[region] if !found { - return "", errors.New("region was not found as part of the regionsInfo map") + return "", ErrRegionNotFound } switch regionInfo.prefix { diff --git a/collector/aws/pricing_test.go b/collector/aws/pricing_test.go index 2e074cd2..38c1701b 100644 --- a/collector/aws/pricing_test.go +++ b/collector/aws/pricing_test.go @@ -103,26 +103,23 @@ func TestGetRegionPrefix(t *testing.T) { testCases := []struct { region string expectedPrefix string + expectedError error }{ - {"us-east-1", ""}, - {"us-west-1", "USW1-"}, - {"ap-northeast-2", "APN2-"}, - {"bla", ""}, + {"us-east-1", "", nil}, + {"us-west-1", "USW1-", nil}, + {"ap-northeast-2", "APN2-", nil}, + {"bla", "", aws.ErrRegionNotFound}, } pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") for _, tc := range testCases { t.Run(tc.region, func(t *testing.T) { - if tc.region != "bla" { - pricingValuePrefix, _ := pricingManager.GetRegionPrefix(tc.region) - if tc.expectedPrefix != pricingValuePrefix { - t.Fatalf("unexpected pricing value prefix, got: %s, expected: %s", pricingValuePrefix, tc.expectedPrefix) - } - } else { - _, err := pricingManager.GetRegionPrefix(tc.region) - if err == nil { - t.Fatalf("expected an error for getRegionPrefix") - } + pricingValuePrefix, err := pricingManager.GetRegionPrefix(tc.region) + if tc.expectedPrefix != pricingValuePrefix { + t.Fatalf("unexpected pricing value prefix, got: %s, expected: %s", pricingValuePrefix, tc.expectedPrefix) + } + if err != tc.expectedError { + t.Fatalf("unexpected error response, got: %v, expected: %v", err, tc.expectedError) } }) } From ce86f826246bf8a5ec5955e1f1b3c7313de876d9 Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Mon, 6 Jul 2020 07:42:23 +0300 Subject: [PATCH 09/11] Increase test coverage for elb and elbv2 --- collector/aws/elb_test.go | 49 +++++++++++++++++++------------- collector/aws/elbv2_test.go | 56 ++++++++++++++++++++++--------------- 2 files changed, 62 insertions(+), 43 deletions(-) diff --git a/collector/aws/elb_test.go b/collector/aws/elb_test.go index c004229d..cc4ec631 100644 --- a/collector/aws/elb_test.go +++ b/collector/aws/elb_test.go @@ -107,32 +107,41 @@ func TestDetectELB(t *testing.T) { } func TestDetectELBError(t *testing.T) { - - collector := testutils.NewMockCollector() - mockCloudwatchClient := MockAWSCloudwatchClient{ - responseMetricStatistics: defaultResponseMetricStatistics, - } - cloutwatchManager := aws.NewCloudWatchManager(&mockCloudwatchClient) - pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") - mockClient := MockAWSELBClient{ err: errors.New(""), } - elbManager := aws.NewELBManager(collector, &mockClient, cloutwatchManager, pricingManager, defaultMetricConfig, "us-east-1") - - response, _ := elbManager.Detect() - - if len(response) != 0 { - t.Fatalf("unexpected elb detected, got %d expected %d", len(response), 0) + testCases := []struct { + region string + expectedPrefix string + expectedError error + }{ + {"us-east-1", "", mockClient.err}, + {"no-region-1", "", aws.ErrRegionNotFound}, } + for _, tc := range testCases { + collector := testutils.NewMockCollector() + mockCloudwatchClient := MockAWSCloudwatchClient{ + responseMetricStatistics: defaultResponseMetricStatistics, + } + cloutwatchManager := aws.NewCloudWatchManager(&mockCloudwatchClient) + pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") + elbManager := aws.NewELBManager(collector, &mockClient, cloutwatchManager, pricingManager, defaultMetricConfig, tc.region) + response, err := elbManager.Detect() - if len(collector.Events) != 0 { - t.Fatalf("unexpected collector elb resources, got %d expected %d", len(collector.Events), 0) - } + if len(response) != 0 { + t.Fatalf("unexpected elb detected, got %d expected %d", len(response), 0) + } - if len(collector.EventsCollectionStatus) != 2 { - t.Fatalf("unexpected resource status events count, got %d expected %d", len(collector.EventsCollectionStatus), 2) - } + if len(collector.Events) != 0 { + t.Fatalf("unexpected collector elb resources, got %d expected %d", len(collector.Events), 0) + } + if len(collector.EventsCollectionStatus) != 2 { + t.Fatalf("unexpected resource status events count, got %d expected %d", len(collector.EventsCollectionStatus), 2) + } + if !errors.Is(err, tc.expectedError) { + t.Fatalf("unexpected error response, got: %v, expected: %v", err, tc.expectedError) + } + } } diff --git a/collector/aws/elbv2_test.go b/collector/aws/elbv2_test.go index 918e6cc9..6d40ba9d 100644 --- a/collector/aws/elbv2_test.go +++ b/collector/aws/elbv2_test.go @@ -106,36 +106,46 @@ func TestDetectELBV2(t *testing.T) { if len(collector.EventsCollectionStatus) != 2 { t.Fatalf("unexpected resource status events count, got %d expected %d", len(collector.EventsCollectionStatus), 2) } - } func TestDetectELBV2Error(t *testing.T) { - - collector := testutils.NewMockCollector() - mockCloudwatchClient := MockAWSCloudwatchClient{ - responseMetricStatistics: defaultResponseMetricStatistics, - } - cloutwatchManager := aws.NewCloudWatchManager(&mockCloudwatchClient) - pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") - mockClient := MockAWSELBV2Client{ err: errors.New(""), } - - elbv2Manager := aws.NewELBV2Manager(collector, &mockClient, cloutwatchManager, pricingManager, defaultMetricConfig, "us-east-1") - - response, _ := elbv2Manager.Detect() - - if len(response) != 0 { - t.Fatalf("unexpected elbv2 detected, got %d expected %d", len(response), 0) - } - - if len(collector.Events) != 0 { - t.Fatalf("unexpected collector elbv2 resources, got %d expected %d", len(collector.Events), 0) + testCases := []struct { + region string + expectedPrefix string + expectedError error + }{ + {"us-east-1", "", mockClient.err}, + {"no-region-1", "", aws.ErrRegionNotFound}, } - if len(collector.EventsCollectionStatus) != 2 { - t.Fatalf("unexpected resource status events count, got %d expected %d", len(collector.EventsCollectionStatus), 2) + for _, tc := range testCases { + collector := testutils.NewMockCollector() + mockCloudwatchClient := MockAWSCloudwatchClient{ + responseMetricStatistics: defaultResponseMetricStatistics, + } + cloutwatchManager := aws.NewCloudWatchManager(&mockCloudwatchClient) + pricingManager := aws.NewPricingManager(&defaultPricingMock, "us-east-1") + + elbv2Manager := aws.NewELBV2Manager(collector, &mockClient, cloutwatchManager, pricingManager, defaultMetricConfig, tc.region) + response, err := elbv2Manager.Detect() + t.Run(tc.region, func(t *testing.T) { + if len(response) != 0 { + t.Fatalf("unexpected elbv2 detected, got %d expected %d", len(response), 0) + } + + if len(collector.Events) != 0 { + t.Fatalf("unexpected collector elbv2 resources, got %d expected %d", len(collector.Events), 0) + } + + if len(collector.EventsCollectionStatus) != 2 { + t.Fatalf("unexpected resource status events count, got %d expected %d", len(collector.EventsCollectionStatus), 2) + } + if !errors.Is(err, tc.expectedError) { + t.Fatalf("unexpected error response, got: %v, expected: %v", err, tc.expectedError) + } + }) } - } From bb765430e01ab6a07062a82d03cc7fc734f47b2d Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Mon, 6 Jul 2020 09:42:27 +0300 Subject: [PATCH 10/11] return prefix instead of empty string. --- collector/aws/pricing.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/collector/aws/pricing.go b/collector/aws/pricing.go index 8b85a7e8..087e7fd9 100644 --- a/collector/aws/pricing.go +++ b/collector/aws/pricing.go @@ -208,7 +208,7 @@ func (p *PricingManager) GetRegionPrefix(region string) (string, error) { var prefix string regionInfo, found := regionsInfo[region] if !found { - return "", ErrRegionNotFound + return prefix, ErrRegionNotFound } switch regionInfo.prefix { From c20cd61f12239abfe99efcf1f223a8dd918c51aa Mon Sep 17 00:00:00 2001 From: Costya Regev Date: Mon, 6 Jul 2020 12:34:59 +0300 Subject: [PATCH 11/11] Add changelog changes. --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc738aef..5983ff54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - Add the option to filter resources by tags in the API [[PR-73](https://github.com/similarweb/finala/pull/73)] -- Ignore Neptune based instances for RDS instances collection -- Implement golangci-lint and golang fmt +- Fix the pricing for ELB/ELBV2 [[PR-76](https://github.com/similarweb/finala/pull/76)] +- Ignore Neptune based instances for RDS instances collection [[PR-71](https://github.com/similarweb/finala/pull/71)] +- Implement golangci-lint and golang fmt [[PR-78](https://github.com/similarweb/finala/pull/78)] ## [0.3.3] ### Added