Skip to content

Commit c7693f4

Browse files
rickbrouwerrobpickerill
rickbrouwer
authored and
robpickerill
committed
Refactor New Relic scaler (kedacore#6326)
Signed-off-by: rickbrouwer <[email protected]>
1 parent c988e5e commit c7693f4

File tree

3 files changed

+74
-124
lines changed

3 files changed

+74
-124
lines changed

‎pkg/scalers/newrelic_scaler.go

+38-109
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@ package scalers
33
import (
44
"context"
55
"fmt"
6-
"log"
7-
"strconv"
86

97
"github.com/go-logr/logr"
108
"github.com/newrelic/newrelic-client-go/newrelic"
@@ -17,31 +15,25 @@ import (
1715
)
1816

1917
const (
20-
account = "account"
21-
queryKeyParamater = "queryKey"
22-
regionParameter = "region"
23-
nrql = "nrql"
24-
threshold = "threshold"
25-
noDataError = "noDataError"
26-
scalerName = "new-relic"
18+
scalerName = "new-relic"
2719
)
2820

2921
type newrelicScaler struct {
3022
metricType v2.MetricTargetType
31-
metadata *newrelicMetadata
23+
metadata newrelicMetadata
3224
nrClient *newrelic.NewRelic
3325
logger logr.Logger
3426
}
3527

3628
type newrelicMetadata struct {
37-
account int
38-
region string
39-
queryKey string
40-
noDataError bool
41-
nrql string
42-
threshold float64
43-
activationThreshold float64
44-
triggerIndex int
29+
Account int `keda:"name=account, order=authParams;triggerMetadata"`
30+
Region string `keda:"name=region, order=authParams;triggerMetadata, default=US"`
31+
QueryKey string `keda:"name=queryKey, order=authParams;triggerMetadata"`
32+
NoDataError bool `keda:"name=noDataError, order=triggerMetadata, default=false"`
33+
NRQL string `keda:"name=nrql, order=triggerMetadata"`
34+
Threshold float64 `keda:"name=threshold, order=triggerMetadata"`
35+
ActivationThreshold float64 `keda:"name=activationThreshold, order=triggerMetadata, default=0"`
36+
TriggerIndex int
4537
}
4638

4739
func NewNewRelicScaler(config *scalersconfig.ScalerConfig) (Scaler, error) {
@@ -52,128 +44,69 @@ func NewNewRelicScaler(config *scalersconfig.ScalerConfig) (Scaler, error) {
5244

5345
logger := InitializeLogger(config, fmt.Sprintf("%s_scaler", scalerName))
5446

55-
meta, err := parseNewRelicMetadata(config, logger)
47+
meta, err := parseNewRelicMetadata(config)
5648
if err != nil {
5749
return nil, fmt.Errorf("error parsing %s metadata: %w", scalerName, err)
5850
}
5951

6052
nrClient, err := newrelic.New(
61-
newrelic.ConfigPersonalAPIKey(meta.queryKey),
62-
newrelic.ConfigRegion(meta.region))
63-
53+
newrelic.ConfigPersonalAPIKey(meta.QueryKey),
54+
newrelic.ConfigRegion(meta.Region))
6455
if err != nil {
65-
log.Fatal("error initializing client:", err)
56+
return nil, fmt.Errorf("error initializing client: %w", err)
6657
}
6758

68-
logMsg := fmt.Sprintf("Initializing New Relic Scaler (account %d in region %s)", meta.account, meta.region)
69-
70-
logger.Info(logMsg)
59+
logger.Info(fmt.Sprintf("Initializing New Relic Scaler (account %d in region %s)", meta.Account, meta.Region))
7160

7261
return &newrelicScaler{
7362
metricType: metricType,
7463
metadata: meta,
7564
nrClient: nrClient,
76-
logger: logger}, nil
65+
logger: logger,
66+
}, nil
7767
}
7868

79-
func parseNewRelicMetadata(config *scalersconfig.ScalerConfig, logger logr.Logger) (*newrelicMetadata, error) {
69+
func parseNewRelicMetadata(config *scalersconfig.ScalerConfig) (newrelicMetadata, error) {
8070
meta := newrelicMetadata{}
81-
var err error
82-
83-
val, err := GetFromAuthOrMeta(config, account)
84-
if err != nil {
85-
return nil, err
86-
}
87-
88-
t, err := strconv.Atoi(val)
89-
if err != nil {
90-
return nil, fmt.Errorf("error parsing %s: %w", account, err)
91-
}
92-
meta.account = t
93-
94-
if val, ok := config.TriggerMetadata[nrql]; ok && val != "" {
95-
meta.nrql = val
96-
} else {
97-
return nil, fmt.Errorf("no %s given", nrql)
98-
}
99-
100-
queryKey, err := GetFromAuthOrMeta(config, queryKeyParamater)
101-
if err != nil {
102-
return nil, err
103-
}
104-
meta.queryKey = queryKey
105-
106-
region, err := GetFromAuthOrMeta(config, regionParameter)
71+
err := config.TypedConfig(&meta)
10772
if err != nil {
108-
region = "US"
109-
logger.Info("Using default 'US' region")
73+
return meta, fmt.Errorf("error parsing newrelic metadata: %w", err)
11074
}
111-
meta.region = region
11275

113-
if val, ok := config.TriggerMetadata[threshold]; ok && val != "" {
114-
t, err := strconv.ParseFloat(val, 64)
115-
if err != nil {
116-
return nil, fmt.Errorf("error parsing %s", threshold)
117-
}
118-
meta.threshold = t
119-
} else {
120-
if config.AsMetricSource {
121-
meta.threshold = 0
122-
} else {
123-
return nil, fmt.Errorf("missing %s value", threshold)
124-
}
76+
if config.AsMetricSource {
77+
meta.Threshold = 0
12578
}
12679

127-
meta.activationThreshold = 0
128-
if val, ok := config.TriggerMetadata["activationThreshold"]; ok {
129-
activationThreshold, err := strconv.ParseFloat(val, 64)
130-
if err != nil {
131-
return nil, fmt.Errorf("queryValue parsing error %w", err)
132-
}
133-
meta.activationThreshold = activationThreshold
134-
}
135-
136-
// If Query Return an Empty Data , shall we treat it as an error or not
137-
// default is NO error is returned when query result is empty/no data
138-
if val, ok := config.TriggerMetadata[noDataError]; ok {
139-
noDataError, err := strconv.ParseBool(val)
140-
if err != nil {
141-
return nil, fmt.Errorf("noDataError has invalid value")
142-
}
143-
meta.noDataError = noDataError
144-
} else {
145-
meta.noDataError = false
146-
}
147-
meta.triggerIndex = config.TriggerIndex
148-
return &meta, nil
80+
meta.TriggerIndex = config.TriggerIndex
81+
return meta, nil
14982
}
15083

15184
func (s *newrelicScaler) Close(context.Context) error {
15285
return nil
15386
}
15487

15588
func (s *newrelicScaler) executeNewRelicQuery(ctx context.Context) (float64, error) {
156-
nrdbQuery := nrdb.NRQL(s.metadata.nrql)
157-
resp, err := s.nrClient.Nrdb.QueryWithContext(ctx, s.metadata.account, nrdbQuery)
89+
nrdbQuery := nrdb.NRQL(s.metadata.NRQL)
90+
resp, err := s.nrClient.Nrdb.QueryWithContext(ctx, s.metadata.Account, nrdbQuery)
15891
if err != nil {
159-
return 0, fmt.Errorf("error running NRQL %s (%s)", s.metadata.nrql, err.Error())
92+
return 0, fmt.Errorf("error running NRQL %s: %w", s.metadata.NRQL, err)
16093
}
161-
// Check for empty results set, as New Relic lib does not report these as errors
94+
16295
if len(resp.Results) == 0 {
163-
if s.metadata.noDataError {
164-
return 0, fmt.Errorf("query return no results %s", s.metadata.nrql)
96+
if s.metadata.NoDataError {
97+
return 0, fmt.Errorf("query returned no results: %s", s.metadata.NRQL)
16598
}
16699
return 0, nil
167100
}
168101
// Only use the first result from the query, as the query should not be multi row
169102
for _, v := range resp.Results[0] {
170-
val, ok := v.(float64)
171-
if ok {
103+
if val, ok := v.(float64); ok {
172104
return val, nil
173105
}
174106
}
175-
if s.metadata.noDataError {
176-
return 0, fmt.Errorf("query return no results %s", s.metadata.nrql)
107+
108+
if s.metadata.NoDataError {
109+
return 0, fmt.Errorf("query returned no numeric results: %s", s.metadata.NRQL)
177110
}
178111
return 0, nil
179112
}
@@ -186,21 +119,17 @@ func (s *newrelicScaler) GetMetricsAndActivity(ctx context.Context, metricName s
186119
}
187120

188121
metric := GenerateMetricInMili(metricName, val)
189-
190-
return []external_metrics.ExternalMetricValue{metric}, val > s.metadata.activationThreshold, nil
122+
return []external_metrics.ExternalMetricValue{metric}, val > s.metadata.ActivationThreshold, nil
191123
}
192124

193125
func (s *newrelicScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec {
194126
metricName := kedautil.NormalizeString(scalerName)
195-
196127
externalMetric := &v2.ExternalMetricSource{
197128
Metric: v2.MetricIdentifier{
198-
Name: GenerateMetricNameWithIndex(s.metadata.triggerIndex, metricName),
129+
Name: GenerateMetricNameWithIndex(s.metadata.TriggerIndex, metricName),
199130
},
200-
Target: GetMetricTargetMili(s.metricType, s.metadata.threshold),
201-
}
202-
metricSpec := v2.MetricSpec{
203-
External: externalMetric, Type: externalMetricType,
131+
Target: GetMetricTargetMili(s.metricType, s.metadata.Threshold),
204132
}
133+
metricSpec := v2.MetricSpec{External: externalMetric, Type: externalMetricType}
205134
return []v2.MetricSpec{metricSpec}
206135
}

‎pkg/scalers/newrelic_scaler_test.go

+28-15
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

88
"github.com/go-logr/logr"
9+
v2 "k8s.io/api/autoscaling/v2"
910

1011
"github.com/kedacore/keda/v2/pkg/scalers/scalersconfig"
1112
)
@@ -26,7 +27,7 @@ var testNewRelicMetadata = []parseNewRelicMetadataTestData{
2627
{map[string]string{}, map[string]string{}, true},
2728
// all properly formed
2829
{map[string]string{"account": "0", "threshold": "100", "queryKey": "somekey", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{}, false},
29-
// all properly formed
30+
// all properly formed with region and activationThreshold
3031
{map[string]string{"account": "0", "region": "EU", "threshold": "100", "activationThreshold": "20", "queryKey": "somekey", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{}, false},
3132
// account passed via auth params
3233
{map[string]string{"region": "EU", "threshold": "100", "queryKey": "somekey", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{"account": "0"}, false},
@@ -48,7 +49,7 @@ var testNewRelicMetadata = []parseNewRelicMetadataTestData{
4849
{map[string]string{"account": "0", "threshold": "100", "queryKey": "somekey"}, map[string]string{}, true},
4950
// noDataError invalid value
5051
{map[string]string{"account": "0", "threshold": "100", "queryKey": "somekey", "noDataError": "invalid", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{}, true},
51-
// noDataError valid value
52+
// noDataError valid values
5253
{map[string]string{"account": "0", "threshold": "100", "queryKey": "somekey", "noDataError": "true", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{}, false},
5354
{map[string]string{"account": "0", "threshold": "100", "queryKey": "somekey", "noDataError": "false", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{}, false},
5455
{map[string]string{"account": "0", "threshold": "100", "queryKey": "somekey", "noDataError": "0", "nrql": "SELECT average(cpuUsedCores) as result FROM K8sContainerSample WHERE containerName='coredns'"}, map[string]string{}, false},
@@ -61,27 +62,39 @@ var newrelicMetricIdentifiers = []newrelicMetricIdentifier{
6162
}
6263

6364
func TestNewRelicParseMetadata(t *testing.T) {
64-
for _, testData := range testNewRelicMetadata {
65-
_, err := parseNewRelicMetadata(&scalersconfig.ScalerConfig{TriggerMetadata: testData.metadata, AuthParams: testData.authParams}, logr.Discard())
66-
if err != nil && !testData.isError {
67-
fmt.Printf("X: %s", testData.metadata)
68-
t.Error("Expected success but got error", err)
69-
}
70-
if testData.isError && err == nil {
71-
fmt.Printf("X: %s", testData.metadata)
72-
t.Error("Expected error but got success")
73-
}
65+
for i, testData := range testNewRelicMetadata {
66+
t.Run(fmt.Sprintf("test case %d", i), func(t *testing.T) {
67+
_, err := parseNewRelicMetadata(&scalersconfig.ScalerConfig{
68+
TriggerMetadata: testData.metadata,
69+
AuthParams: testData.authParams,
70+
})
71+
if err != nil && !testData.isError {
72+
t.Errorf("Test case %d: Expected success but got error: %v\nMetadata: %v\nAuthParams: %v",
73+
i, err, testData.metadata, testData.authParams)
74+
}
75+
if testData.isError && err == nil {
76+
t.Errorf("Test case %d: Expected error but got success\nMetadata: %v\nAuthParams: %v",
77+
i, testData.metadata, testData.authParams)
78+
}
79+
})
7480
}
7581
}
82+
7683
func TestNewRelicGetMetricSpecForScaling(t *testing.T) {
7784
for _, testData := range newrelicMetricIdentifiers {
78-
meta, err := parseNewRelicMetadata(&scalersconfig.ScalerConfig{TriggerMetadata: testData.metadataTestData.metadata, AuthParams: testData.metadataTestData.authParams, TriggerIndex: testData.triggerIndex}, logr.Discard())
85+
meta, err := parseNewRelicMetadata(&scalersconfig.ScalerConfig{
86+
TriggerMetadata: testData.metadataTestData.metadata,
87+
AuthParams: testData.metadataTestData.authParams,
88+
TriggerIndex: testData.triggerIndex,
89+
})
7990
if err != nil {
8091
t.Fatal("Could not parse metadata:", err)
8192
}
8293
mockNewRelicScaler := newrelicScaler{
83-
metadata: meta,
84-
nrClient: nil,
94+
metadata: meta,
95+
nrClient: nil,
96+
logger: logr.Discard(),
97+
metricType: v2.AverageValueMetricType,
8598
}
8699

87100
metricSpec := mockNewRelicScaler.GetMetricSpecForScaling(context.Background())

‎pkg/scalers/scalersconfig/typed_config.go

+8
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,14 @@ func setConfigValueHelper(params Params, valFromConfig string, field reflect.Val
396396
if field.Kind() == reflect.Slice {
397397
return setConfigValueSlice(params, valFromConfig, field)
398398
}
399+
if field.Kind() == reflect.Bool {
400+
boolVal, err := strconv.ParseBool(valFromConfig)
401+
if err != nil {
402+
return fmt.Errorf("unable to parse boolean value %q: %w", valFromConfig, err)
403+
}
404+
field.SetBool(boolVal)
405+
return nil
406+
}
399407
if field.CanInterface() {
400408
ifc := reflect.New(field.Type()).Interface()
401409
if err := json.Unmarshal([]byte(valFromConfig), &ifc); err != nil {

0 commit comments

Comments
 (0)