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

Improve AWS Cloudwatch Scaler metric exporting logic #2243

Merged
merged 7 commits into from
Nov 4, 2021
Merged

Improve AWS Cloudwatch Scaler metric exporting logic #2243

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c6ef7c7
improve aws cloudwatch scaler metrics pulling logic
fivesheep Nov 1, 2021
adc95b4
add additional checks and unit test
fivesheep Nov 2, 2021
2ccc901
fix formatting
fivesheep Nov 3, 2021
d5c2437
fix linting
fivesheep Nov 3, 2021
fad0143
fix linting
fivesheep Nov 3, 2021
540df89
refactor parseAwsCloudwatchMetadata to reduce cyclomatic complexity
fivesheep Nov 3, 2021
92f94b2
update CHANGELOG
fivesheep Nov 4, 2021
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
149 changes: 98 additions & 51 deletions pkg/scalers/aws_cloudwatch_scaler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ type awsCloudwatchMetadata struct {

metricCollectionTime int64
metricStat string
metricUnit string
metricStatPeriod int64
metricEndTimeOffset int64

awsRegion string

Expand All @@ -67,44 +69,8 @@ func NewAwsCloudwatchScaler(config *ScalerConfig) (Scaler, error) {
}, nil
}

func parseMetricValues(config *ScalerConfig) (*awsCloudwatchMetadata, error) {
metricsMeta := awsCloudwatchMetadata{}

if val, ok := config.TriggerMetadata["metricCollectionTime"]; ok && val != "" {
if n, ok := strconv.ParseInt(val, 10, 64); ok == nil {
metricsMeta.metricCollectionTime = n
} else {
return nil, fmt.Errorf("metricCollectionTime not a valid number")
}
} else {
metricsMeta.metricCollectionTime = defaultMetricCollectionTime
}

if val, ok := config.TriggerMetadata["metricStatPeriod"]; ok && val != "" {
if n, ok := strconv.ParseInt(val, 10, 64); ok == nil {
metricsMeta.metricStatPeriod = n
} else {
return nil, fmt.Errorf("metricStatPeriod not a valid number")
}
} else {
metricsMeta.metricStatPeriod = defaultMetricStatPeriod
}

if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" {
metricsMeta.metricStat = val
} else {
metricsMeta.metricStat = defaultMetricStat
}

return &metricsMeta, nil
}

func parseAwsCloudwatchMetadata(config *ScalerConfig) (*awsCloudwatchMetadata, error) {
meta, err := parseMetricValues(config)

if err != nil {
return nil, fmt.Errorf("an error occurred when the scaler tried to get the metrics values")
}
meta := awsCloudwatchMetadata{}

if val, ok := config.TriggerMetadata["namespace"]; ok && val != "" {
meta.namespace = val
Expand Down Expand Up @@ -137,7 +103,7 @@ func parseAwsCloudwatchMetadata(config *ScalerConfig) (*awsCloudwatchMetadata, e
if val, ok := config.TriggerMetadata["targetMetricValue"]; ok && val != "" {
targetMetricValue, err := strconv.ParseFloat(val, 64)
if err != nil {
cloudwatchLog.Error(err, "Error parsing targetMetricValue metadata")
return nil, fmt.Errorf("error parsing targetMetricValue metadata: %v", err)
} else {
meta.targetMetricValue = targetMetricValue
}
Expand All @@ -148,34 +114,65 @@ func parseAwsCloudwatchMetadata(config *ScalerConfig) (*awsCloudwatchMetadata, e
if val, ok := config.TriggerMetadata["minMetricValue"]; ok && val != "" {
minMetricValue, err := strconv.ParseFloat(val, 64)
if err != nil {
cloudwatchLog.Error(err, "Error parsing minMetricValue metadata")
return nil, fmt.Errorf("error parsing minMetricValue metadata: %v", err)
} else {
meta.minMetricValue = minMetricValue
}
} else {
return nil, fmt.Errorf("min metric value not given")
}

if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" {
if err := checkMetricStat(val); err != nil {
return nil, err
}
meta.metricStat = val
} else {
meta.metricStat = defaultMetricStat
}

if val, ok := config.TriggerMetadata["metricStatPeriod"]; ok && val != "" {
metricStatPeriod, err := strconv.Atoi(val)
if err != nil {
return nil, fmt.Errorf("error parsing metricStatPeriod metadata: %v", err)
}
if err := checkMetricStatPeriod(metricStatPeriod); err != nil {
return nil, err
} else {
meta.metricStatPeriod = int64(metricStatPeriod)
}
} else {
meta.metricStatPeriod = defaultMetricStatPeriod
}

if val, ok := config.TriggerMetadata["metricCollectionTime"]; ok && val != "" {
metricCollectionTime, err := strconv.Atoi(val)
if err != nil {
cloudwatchLog.Error(err, "Error parsing metricCollectionTime metadata")
} else {
meta.metricCollectionTime = int64(metricCollectionTime)
return nil, fmt.Errorf("error parsing metricCollectionTime metadata: %v", err)
}
meta.metricCollectionTime = int64(metricCollectionTime)
} else {
meta.metricCollectionTime = defaultMetricCollectionTime
}

if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" {
meta.metricStat = val
if meta.metricCollectionTime < 0 || meta.metricCollectionTime%meta.metricStatPeriod != 0 {
return nil, fmt.Errorf("metricCollectionTime must be greater than 0 and a multiple of metricStatPeriod(%d), %d is given", meta.metricStatPeriod, meta.metricCollectionTime)
}

if val, ok := config.TriggerMetadata["metricStatPeriod"]; ok && val != "" {
metricStatPeriod, err := strconv.Atoi(val)
if val, ok := config.TriggerMetadata["metricEndTimeOffset"]; ok && val != "" {
metricEndTimeOffset, err := strconv.Atoi(val)
if err != nil {
cloudwatchLog.Error(err, "Error parsing metricStatPeriod metadata")
return nil, fmt.Errorf("error parsing metricEndTimeOffset metadata: %v", err)
} else {
meta.metricStatPeriod = int64(metricStatPeriod)
meta.metricEndTimeOffset = int64(metricEndTimeOffset)
}
}

if val, ok := config.TriggerMetadata["metricUnit"]; ok && val != "" {
if err := checkMetricUnit(val); err != nil {
return nil, err
}
meta.metricUnit = val
}

if val, ok := config.TriggerMetadata["awsRegion"]; ok && val != "" {
Expand All @@ -193,7 +190,47 @@ func parseAwsCloudwatchMetadata(config *ScalerConfig) (*awsCloudwatchMetadata, e

meta.scalerIndex = config.ScalerIndex

return meta, nil
return &meta, nil
}

func checkMetricStat(stat string) error {
for _, s := range cloudwatch.Statistic_Values() {
if stat == s {
return nil
}
}
return fmt.Errorf("metricStat '%s' is not one of %v", stat, cloudwatch.Statistic_Values())
}

func checkMetricUnit(unit string) error {
if unit == "" {
return nil
}
for _, u := range cloudwatch.StandardUnit_Values() {
if unit == u {
return nil
}
}
return fmt.Errorf("metricUnit '%s' is not one of %v", unit, cloudwatch.StandardUnit_Values())
}

func checkMetricStatPeriod(period int) error {
if period < 1 {
return fmt.Errorf("metricStatPeriod can not be smaller than 1, however, %d is provided", period)
} else if period <= 60 {
switch period {
case 1, 5, 10, 30, 60:
return nil
default:
return fmt.Errorf("metricStatPeriod < 60 has to be one of [1, 5, 10, 30], however, %d is provided", period)
}
}

if period % 60 != 0 {
return fmt.Errorf("metricStatPeriod >= 60 has to be a multiple of 60, however, %d is provided", period)
}

return nil
}

func (c *awsCloudwatchScaler) GetMetrics(ctx context.Context, metricName string, metricSelector labels.Selector) ([]external_metrics.ExternalMetricValue, error) {
Expand Down Expand Up @@ -273,9 +310,18 @@ func (c *awsCloudwatchScaler) GetCloudwatchMetrics() (float64, error) {
})
}

endTime := time.Now().Add(time.Second * -1 * time.Duration(c.metadata.metricEndTimeOffset)).Truncate(time.Duration(c.metadata.metricStatPeriod) * time.Second)
startTime := endTime.Add(time.Second * -1 * time.Duration(c.metadata.metricCollectionTime))

var metricUnit *string
if c.metadata.metricUnit != "" {
metricUnit = aws.String(c.metadata.metricUnit)
}

input := cloudwatch.GetMetricDataInput{
StartTime: aws.Time(time.Now().Add(time.Second * -1 * time.Duration(c.metadata.metricCollectionTime))),
EndTime: aws.Time(time.Now()),
StartTime: aws.Time(startTime),
EndTime: aws.Time(endTime),
ScanBy: aws.String(cloudwatch.ScanByTimestampDescending),
MetricDataQueries: []*cloudwatch.MetricDataQuery{
{
Id: aws.String("c1"),
Expand All @@ -287,6 +333,7 @@ func (c *awsCloudwatchScaler) GetCloudwatchMetrics() (float64, error) {
},
Period: aws.Int64(c.metadata.metricStatPeriod),
Stat: aws.String(c.metadata.metricStat),
Unit: metricUnit,
},
ReturnData: aws.Bool(true),
},
Expand Down
75 changes: 75 additions & 0 deletions pkg/scalers/aws_cloudwatch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,81 @@ var testAWSCloudwatchMetadata = []parseAWSCloudwatchMetadataTestData{
"awsRegion": "eu-west-1"},
testAWSAuthentication, false,
"Missing metricStatPeriod not generate error because will get the default value"},
{map[string]string{
"namespace": "AWS/SQS",
"dimensionName": "QueueName",
"dimensionValue": "keda",
"metricName": "ApproximateNumberOfMessagesVisible",
"targetMetricValue": "2",
"minMetricValue": "0",
"metricStat": "Average",
"metricUnit": "Count",
"metricEndTimeOffset": "60",
"awsRegion": "eu-west-1"},
testAWSAuthentication, false,
"set a supported metricUnit"},
{map[string]string{
"namespace": "AWS/SQS",
"dimensionName": "QueueName",
"dimensionValue": "keda",
"metricName": "ApproximateNumberOfMessagesVisible",
"targetMetricValue": "2",
"minMetricValue": "0",
"metricCollectionTime": "300",
"metricStat": "SomeStat",
"awsRegion": "eu-west-1"},
testAWSAuthentication, true,
"metricStat is not supported"},
{map[string]string{
"namespace": "AWS/SQS",
"dimensionName": "QueueName",
"dimensionValue": "keda",
"metricName": "ApproximateNumberOfMessagesVisible",
"targetMetricValue": "2",
"minMetricValue": "0",
"metricStatPeriod": "300",
"metricCollectionTime": "100",
"metricStat": "Average",
"awsRegion": "eu-west-1"},
testAWSAuthentication, true,
"metricCollectionTime smaller than metricStatPeriod"},
{map[string]string{
"namespace": "AWS/SQS",
"dimensionName": "QueueName",
"dimensionValue": "keda",
"metricName": "ApproximateNumberOfMessagesVisible",
"targetMetricValue": "2",
"minMetricValue": "0",
"metricStatPeriod": "250",
"metricStat": "Average",
"awsRegion": "eu-west-1"},
testAWSAuthentication, true,
"unsupported metricStatPeriod"},
{map[string]string{
"namespace": "AWS/SQS",
"dimensionName": "QueueName",
"dimensionValue": "keda",
"metricName": "ApproximateNumberOfMessagesVisible",
"targetMetricValue": "2",
"minMetricValue": "0",
"metricStatPeriod": "25",
"metricStat": "Average",
"awsRegion": "eu-west-1"},
testAWSAuthentication, true,
"unsupported metricStatPeriod"},
{map[string]string{
"namespace": "AWS/SQS",
"dimensionName": "QueueName",
"dimensionValue": "keda",
"metricName": "ApproximateNumberOfMessagesVisible",
"targetMetricValue": "2",
"minMetricValue": "0",
"metricStatPeriod": "25",
"metricStat": "Average",
"metricUnit": "Hour",
"awsRegion": "eu-west-1"},
testAWSAuthentication, true,
"unsupported metricUnit"},
}

var awsCloudwatchMetricIdentifiers = []awsCloudwatchMetricIdentifier{
Expand Down