Feature/validate metric name uniqueness #1390

ycabrer · 2020-12-02T03:57:20Z

Metric Names are required to be unique in Scaled Objects

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Tests have been added
[N/A] A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
Changelog has been updated

Fixes #

ycabrer · 2020-12-02T04:38:08Z

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

Also any tips on where to put the documentation update for the metric name validation?

zroubalik

Looking good so far, we should add a check though

zroubalik · 2020-12-07T09:17:17Z

controllers/scaledobject_controller.go

+	observedMetricNames := make(map[string]struct{})
+	for _, scaler := range scalers {
+		for _, metric := range scaler.GetMetricSpecForScaling() {
+			metricName := metric.External.Metric.Name


We should check whether the scaler contains External metrics (if != nil), becasue if Resource based Metrics are used, External Metrics are not set.

See:

keda/controllers/hpa.go

Lines 135 to 162 in af2b36f

func (r *ScaledObjectReconciler) getScaledObjectMetricSpecs(logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) ([]autoscalingv2beta2.MetricSpec, error) {

var scaledObjectMetricSpecs []autoscalingv2beta2.MetricSpec

var externalMetricNames []string

var resourceMetricNames []string

scalers, err := r.scaleHandler.GetScalers(scaledObject)

if err != nil {

logger.Error(err, "Error getting scalers")

return nil, err

}

for _, scaler := range scalers {

metricSpecs := scaler.GetMetricSpecForScaling()

for _, metricSpec := range metricSpecs {

if metricSpec.Resource != nil {

resourceMetricNames = append(resourceMetricNames, string(metricSpec.Resource.Name))

}

if metricSpec.External != nil {

// add the scaledObjectName label. This is how the MetricsAdapter will know which scaledobject a metric is for when the HPA queries it.

metricSpec.External.Metric.Selector = &metav1.LabelSelector{MatchLabels: make(map[string]string)}

metricSpec.External.Metric.Selector.MatchLabels["scaledObjectName"] = scaledObject.Name

externalMetricNames = append(externalMetricNames, metricSpec.External.Metric.Name)

}

}

scaledObjectMetricSpecs = append(scaledObjectMetricSpecs, metricSpecs...)

scaler.Close()

}

That's a great point! I went ahead and added a continue when external metrics is nil

zroubalik · 2020-12-07T09:21:10Z

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

The static check is strange, you can probably disable this specific one for the finalizers:

keda/.golangci.yml

Line 50 in af2b36f

exclude-rules:

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

zroubalik · 2020-12-21T14:39:05Z

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

The static check is strange, you can probably disable this specific one for the finalizers:

keda/.golangci.yml

Line 50 in af2b36f

exclude-rules:

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

@ycabrer any update please? :)

tomkerkhove · 2020-12-22T09:03:08Z

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

Missed this one, sorry @zroubalik, but I agree to have a section on multiple scalers and what to expect of them.

Not sure though if this should include technical details such as metric name validation but more how the decision making will happen maybe?

zroubalik · 2021-01-14T13:53:36Z

@ycabrer are you able to update this please? It is completely OK, if you don't have a time to do so. But please let us know, so we can manage this. Thanks!

ycabrer · 2021-01-14T19:34:48Z

@zroubalik Sorry for the delay. I've had a busy start to the new year! I'll go ahead and update the docs.

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

ycabrer · 2021-01-14T20:08:06Z

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

The static check is strange, you can probably disable this specific one for the finalizers:

keda/.golangci.yml

Line 50 in af2b36f

exclude-rules:

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

@ycabrer any update please? :)

I rebased my branch against kedacore/main. It seems like the static checks are still failing for me on the finalizers. I took a look at the golang ci config file and can't seem to ignore the failures. Any advice?

zroubalik · 2021-01-14T20:17:46Z

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

The static check is strange, you can probably disable this specific one for the finalizers:

keda/.golangci.yml

Line 50 in af2b36f

exclude-rules:

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

@ycabrer any update please? :)

I rebased my branch against kedacore/main. It seems like the static checks are still failing for me on the finalizers. I took a look at the golang ci config file and can't seem to ignore the failures. Any advice?

Have you tried adding a new section to the exclude-rules section in .golangci.yaml?

Something like this:

# Excluding interfacer for finalizers, reason: https://github.com/kedacore/keda/pull/1390
 - path: _finalizer.go
      linters:
        - interfacer

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

ycabrer · 2021-01-14T20:53:22Z

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

The static check is strange, you can probably disable this specific one for the finalizers:

keda/.golangci.yml

Line 50 in af2b36f

exclude-rules:

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

@ycabrer any update please? :)

I rebased my branch against kedacore/main. It seems like the static checks are still failing for me on the finalizers. I took a look at the golang ci config file and can't seem to ignore the failures. Any advice?

Have you tried adding a new section to the exclude-rules section in .golangci.yaml?

Something like this:
# Excluding interfacer for finalizers, reason: https://github.com/kedacore/keda/pull/1390
 - path: _finalizer.go
      linters:
        - interfacer

That worked

@zroubalik I'm not sure why the static check is failing. It seems to be referencing code in the object finilizers. I didn't modify them (I think).

The static check is strange, you can probably disable this specific one for the finalizers:

keda/.golangci.yml

Line 50 in af2b36f

exclude-rules:

Also any tips on where to put the documentation update for the metric name validation?

We should probably add a new section unde KEDA Concepts, something like section Mutliple Scalers or Using Scalers ? @tomkerkhove WDYT?

@ycabrer any update please? :)

I rebased my branch against kedacore/main. It seems like the static checks are still failing for me on the finalizers. I took a look at the golang ci config file and can't seem to ignore the failures. Any advice?

Have you tried adding a new section to the exclude-rules section in .golangci.yaml?

Something like this:
# Excluding interfacer for finalizers, reason: https://github.com/kedacore/keda/pull/1390
 - path: _finalizer.go
      linters:
        - interfacer

That did it! I had chosen the wrong linter.

zroubalik · 2021-01-15T08:54:15Z

.golangci.yml

@@ -56,6 +56,9 @@ issues:
    - path: scale_handler.go
      linters:
        - gocyclo
+    - path: _finalizer.go


Could you please add a comment with a description why are we adding this exception, eg. with a link to this PR?

Sure thing! I missed that you had already given an example

zroubalik · 2021-01-15T08:54:59Z

CHANGELOG.md

@@ -40,6 +40,7 @@
 - Improve error reporting in prometheus scaler ([PR #1497](https://github.com/kedacore/keda/pull/1497))

 ### Breaking Changes
+- Require metricNames be unique in scaled objects.


Suggested change

- Require metricNames be unique in scaled objects.

- Require metricNames be unique in scaled objects ([#1390](https://github.com/kedacore/keda/pull/1390))

I just updated it

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

zroubalik

LGTM, thanks @ycabrer!

* add metricName uniqueness check Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

ycabrer requested review from ahmelsayed and zroubalik as code owners December 2, 2020 03:57

zroubalik requested changes Dec 7, 2020

View reviewed changes

zroubalik mentioned this pull request Dec 7, 2020

docs: add documentation for influxdb scaler kedacore/keda-docs#315

Merged

zroubalik mentioned this pull request Jan 4, 2021

Ensure that all query based scalers handles multiple triggers correctly #1473

Closed

ycabrer added 7 commits January 14, 2021 12:43

add metricName uniqueness check

6d72e81

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

fix metricName uniqueness check nil error

11b78c5

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

add metricName uniqueness check tests

11bad05

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

update changelog for metricName uniqueness

5819429

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

fix static checks

7a37494

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

fix spelling

bdba460

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

Only validate external metric names

5e5ddbb

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

ycabrer force-pushed the feature/validate-metric-name-uniqueness branch from e6f2d6c to 5e5ddbb Compare January 14, 2021 19:44

ycabrer requested a review from zroubalik January 14, 2021 19:45

Ignore interfacer linter in golang ci (_finalizer.go)

3ef1aef

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

zroubalik reviewed Jan 15, 2021

View reviewed changes

ycabrer added 2 commits January 19, 2021 00:18

Ignore interfacer linter comments

fa955d1

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

Update Changelog

fa7aba7

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

ycabrer requested a review from zroubalik January 19, 2021 07:27

zroubalik approved these changes Jan 19, 2021

View reviewed changes

zroubalik merged commit 05f015c into kedacore:main Jan 19, 2021

ycabrer added a commit to ycabrer/keda that referenced this pull request Mar 1, 2021

Feature/validate metric name uniqueness (kedacore#1390)

0fa169d

* add metricName uniqueness check Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/validate metric name uniqueness #1390

Feature/validate metric name uniqueness #1390

ycabrer commented Dec 2, 2020 •

edited by zroubalik

Loading

ycabrer commented Dec 2, 2020

zroubalik left a comment

zroubalik Dec 7, 2020

ycabrer Jan 14, 2021

zroubalik commented Dec 7, 2020

zroubalik commented Dec 21, 2020

tomkerkhove commented Dec 22, 2020

zroubalik commented Jan 14, 2021

ycabrer commented Jan 14, 2021

ycabrer commented Jan 14, 2021

zroubalik commented Jan 14, 2021 •

edited

Loading

ycabrer commented Jan 14, 2021

zroubalik Jan 15, 2021

ycabrer Jan 19, 2021

zroubalik Jan 15, 2021

ycabrer Jan 19, 2021

zroubalik left a comment

	func (r ScaledObjectReconciler) getScaledObjectMetricSpecs(logger logr.Logger, scaledObject kedav1alpha1.ScaledObject) ([]autoscalingv2beta2.MetricSpec, error) {
	var scaledObjectMetricSpecs []autoscalingv2beta2.MetricSpec
	var externalMetricNames []string
	var resourceMetricNames []string

	scalers, err := r.scaleHandler.GetScalers(scaledObject)
	if err != nil {
	logger.Error(err, "Error getting scalers")
	return nil, err
	}

	for _, scaler := range scalers {
	metricSpecs := scaler.GetMetricSpecForScaling()

	for _, metricSpec := range metricSpecs {
	if metricSpec.Resource != nil {
	resourceMetricNames = append(resourceMetricNames, string(metricSpec.Resource.Name))
	}
	if metricSpec.External != nil {
	// add the scaledObjectName label. This is how the MetricsAdapter will know which scaledobject a metric is for when the HPA queries it.
	metricSpec.External.Metric.Selector = &metav1.LabelSelector{MatchLabels: make(map[string]string)}
	metricSpec.External.Metric.Selector.MatchLabels["scaledObjectName"] = scaledObject.Name
	externalMetricNames = append(externalMetricNames, metricSpec.External.Metric.Name)
	}
	}
	scaledObjectMetricSpecs = append(scaledObjectMetricSpecs, metricSpecs...)
	scaler.Close()
	}

	- Require metricNames be unique in scaled objects.
	- Require metricNames be unique in scaled objects ([#1390](https://github.com/kedacore/keda/pull/1390))

Feature/validate metric name uniqueness #1390

Feature/validate metric name uniqueness #1390

Conversation

ycabrer commented Dec 2, 2020 • edited by zroubalik Loading

Checklist

ycabrer commented Dec 2, 2020

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik Dec 7, 2020

Choose a reason for hiding this comment

ycabrer Jan 14, 2021

Choose a reason for hiding this comment

zroubalik commented Dec 7, 2020

zroubalik commented Dec 21, 2020

tomkerkhove commented Dec 22, 2020

zroubalik commented Jan 14, 2021

ycabrer commented Jan 14, 2021

ycabrer commented Jan 14, 2021

zroubalik commented Jan 14, 2021 • edited Loading

ycabrer commented Jan 14, 2021

zroubalik Jan 15, 2021

Choose a reason for hiding this comment

ycabrer Jan 19, 2021

Choose a reason for hiding this comment

zroubalik Jan 15, 2021

Choose a reason for hiding this comment

ycabrer Jan 19, 2021

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

ycabrer commented Dec 2, 2020 •

edited by zroubalik

Loading

zroubalik commented Jan 14, 2021 •

edited

Loading