feat: adding influxdb scaler #1326

yquansah · 2020-11-13T04:21:25Z

This PR seeks to add the influxdb scaler and relevant tests. InfluxDB is a time series database, which has a tried-and-true use case for application monitoring supporting a wide range of user defined metrics. The scaler gives the user the flexibility to define a threshold value, and that threshold value is compared against the actual queried value.

Checklist

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

Relates to #1239

yquansah · 2020-11-13T04:31:14Z

I will be working to update the docs by submitting a PR

silenceper · 2020-11-13T05:57:53Z

pkg/scalers/influxdb_scaler.go

+		return false, err
+	}
+
+	return value > s.metadata.thresholdValue, nil


silenceper

Have questions about the value of IsActive?

zroubalik · 2020-11-13T13:28:09Z

pkg/scalers/influxdb_scaler.go

+	targetMetricValue := resource.NewQuantity(int64(s.metadata.thresholdValue), resource.DecimalSI)
+	externalMetric := &v2beta2.ExternalMetricSource{
+		Metric: v2beta2.MetricIdentifier{
+			Name: kedautil.NormalizeString(fmt.Sprintf("%s-%s", "influxdb", s.metadata.organizationName)),


The metric name should be unique enough, to allow specify multiple triggers in one Scaler and it should be possible to distinguish between those individual metric names.

I wonder wheter is s.metadata.organizationName enough?

I think that could be enough.

What if I want to create two (or more) triggers that use the same organization name? I am not an InfluxDB expert, but I suppose this could be possible?

@zroubalik Yes that definitely could be possible

So in this case (when I define two or more influxdb scalers in ScaledObject, that will use the same organization) the generated metric name will be same, right? And that's not ideal :)

@zroubalik Agreed. There is no passed in info that would make this name unique enough. Would using a timestamp suffice?

zroubalik

Could you please add an entry to the Changelog? (Unreleased sectin)
Do you think you can add e2e tests for this scaler? https://github.com/kedacore/keda/tree/main/tests

yquansah · 2020-11-13T18:06:54Z

Thanks for all of the feedback.
Yes @zroubalik I can add both of these.

yquansah · 2020-11-14T05:42:02Z

I want to create a client instance that will write data to my influxdb server in kubernetes. Is it okay if I use my own docker image for a job that will write data to server? Or should I port-forward the service (influxdb server) and run a script that will write data to the server?

This is for the e2e tests.

@zroubalik

zroubalik · 2020-11-16T10:00:36Z

@yquansah yeah, it is completely ok to use your Docker image, we have started to collect these kind of client apps in the separate repo https://github.com/kedacore/test-tools/tree/master/e2e/images, but you can start with your client first.

yquansah · 2020-11-23T01:31:19Z

@zroubalik Looks like there is a lot of moving parts here. I think i'd rather wait till influxdb 2.0 becomes released on docker hub to reference a stable version of it. Can the e2e tests wait, or is it a requirement for this to get merged?

zroubalik · 2020-11-23T09:04:13Z

@zroubalik Looks like there is a lot of moving parts here. I think i'd rather wait till influxdb 2.0 becomes released on docker hub to reference a stable version of it. Can the e2e tests wait, or is it a requirement for this to get merged?

@yquansah so influxdb 2.0 is not released yet? So you would like to wait with this PR for a stable influxdb version? If so, convert this PR to the draft please.

For the e2e test, it is not a hard requirement, but we would like to see it coming in this PR or in a separate one, but as soon as possible after merging the first one. e2e tests are important and even more with the growing list of supported scalers in KEDA :) Thanks!

yquansah · 2020-11-25T06:55:07Z

@zroubalik I found an official release candidate version I can use of influxdb v2. Will be getting a e2e test out. Sorry for the back and forth!!

zroubalik · 2020-11-25T09:14:52Z

@yquansah no worries and thanks for update! :)

yquansah · 2020-11-26T05:58:16Z

@zroubalik e2e tests, and changes to CHANGELOG section added 😄

tomkerkhove · 2020-11-26T06:35:21Z

Any news on te docs @yquansah?

yquansah · 2020-11-26T07:10:21Z

@tomkerkhove Yeah I will get that in. I just wanted to get eyes on these last bits here code-wise.

yquansah · 2020-11-26T08:38:41Z

@tomkerkhove As requested here is the documentation PR: kedacore/keda-docs#315

zroubalik

Looking good, but still have the concern on metricName uniqueness

zroubalik · 2020-11-26T10:03:25Z

pkg/scalers/influxdb_scaler.go

+	targetMetricValue := resource.NewQuantity(int64(s.metadata.thresholdValue), resource.DecimalSI)
+	externalMetric := &v2beta2.ExternalMetricSource{
+		Metric: v2beta2.MetricIdentifier{
+			Name: kedautil.NormalizeString(fmt.Sprintf("%s-%s", "influxdb", s.metadata.organizationName)),


So in this case (when I define two or more influxdb scalers in ScaledObject, that will use the same organization) the generated metric name will be same, right? And that's not ideal :)

tomkerkhove · 2020-11-26T10:38:51Z

Added a remark on the docs but moving it here - Is the org name purely for InfluxDB Cloud? If so, can the scaler work with other servers as well?

yquansah · 2020-11-26T20:26:54Z

@tomkerkhove No, org name is used in open source version as well

tomkerkhove · 2020-11-26T20:28:04Z

Ok, thanks!

zroubalik · 2020-11-27T08:33:06Z

@yquansah you will need to fix the linter errors in Static Checks, see the errors.

to fix the cyclomatic problem, we can ignore this check for the scale_handler.go, to do so, add this part of the code:

    - path: scale_handler.go
      linters:
        - gocyclo

to this line:

keda/.golangci.yml

Line 56 in 29cedc7

And then there is the other problem, that you need to fix in your code.

yquansah · 2020-11-27T17:49:43Z

@zroubalik Static check errors fixed. Thank you!

@zroubalik Agreed. There is no passed in info that would make this name unique enough. Would using a timestamp suffice?

There is still this issue here. What would you suggest?

zroubalik · 2020-11-30T10:02:20Z

@zroubalik Static check errors fixed. Thank you!

@zroubalik Agreed. There is no passed in info that would make this name unique enough. Would using a timestamp suffice?

There is still this issue here. What would you suggest?

We should use the similar approach that's being used for Prometheus and Postgresql (change coming in this PR: #1381).
You can refer to this discussion: #1200 (comment)
ie. do the best effort to generate unique metric name, but add optional metricName parameter to the scaler config.

Signed-off-by: Yoofi Quansah <[email protected]>

yquansah · 2020-11-30T17:35:33Z

@zroubalik Awesome. I pushed a commit, let me know if this suffices. d95dccf

zroubalik

LGTM, thanks for the contribution!

yquansah requested review from ahmelsayed and zroubalik as code owners November 13, 2020 04:21

yquansah force-pushed the yq-add-influx branch 3 times, most recently from 7d44d5d to d93b82a Compare November 13, 2020 04:29

silenceper reviewed Nov 13, 2020

View reviewed changes

zroubalik reviewed Nov 13, 2020

View reviewed changes

yquansah force-pushed the yq-add-influx branch 2 times, most recently from 7b34f13 to d4bfcea Compare November 26, 2020 05:51

yquansah requested a review from zroubalik November 26, 2020 06:00

yquansah mentioned this pull request Nov 26, 2020

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

Merged

zroubalik reviewed Nov 26, 2020

View reviewed changes

yquansah force-pushed the yq-add-influx branch from 50298c1 to b4a024c Compare November 26, 2020 21:55

yquansah requested a review from zroubalik November 27, 2020 07:03

yquansah added 9 commits November 30, 2020 08:19

feat: adding influxdb scaler

a39f088

Signed-off-by: Yoofi Quansah <[email protected]>

chore: address feedback comparing values and fixing CI issue

5b3de8e

Signed-off-by: Yoofi Quansah <[email protected]>

chore: add e2e tests for influxdb

9d5a375

Signed-off-by: Yoofi Quansah <[email protected]>

chore: add to unreleased section on CHANGELOG

9e021e3

Signed-off-by: Yoofi Quansah <[email protected]>

chore: running go mod tidy

c920d4c

Signed-off-by: Yoofi Quansah <[email protected]>

chore: import misname, use v2

1b917fd

Signed-off-by: Yoofi Quansah <[email protected]>

chore: fix static ci checks

abd90bf

Signed-off-by: Yoofi Quansah <[email protected]>

chore: fix static check errors

c832abd

Signed-off-by: Yoofi Quansah <[email protected]>

feat: add logic for specifying metric name and default if not provided

d95dccf

Signed-off-by: Yoofi Quansah <[email protected]>

yquansah force-pushed the yq-add-influx branch from be9d65f to d95dccf Compare November 30, 2020 17:32

zroubalik approved these changes Dec 1, 2020

View reviewed changes

zroubalik merged commit 342136e into kedacore:main Dec 1, 2020

yquansah deleted the yq-add-influx branch January 9, 2021 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding influxdb scaler #1326

feat: adding influxdb scaler #1326

yquansah commented Nov 13, 2020 •

edited

Loading

yquansah commented Nov 13, 2020

silenceper Nov 13, 2020

silenceper left a comment

zroubalik Nov 13, 2020

yquansah Nov 14, 2020

zroubalik Nov 16, 2020

yquansah Nov 26, 2020

zroubalik Nov 26, 2020

yquansah Nov 26, 2020

zroubalik left a comment

yquansah commented Nov 13, 2020

yquansah commented Nov 14, 2020

zroubalik commented Nov 16, 2020

yquansah commented Nov 23, 2020

zroubalik commented Nov 23, 2020

yquansah commented Nov 25, 2020

zroubalik commented Nov 25, 2020

yquansah commented Nov 26, 2020

tomkerkhove commented Nov 26, 2020

yquansah commented Nov 26, 2020 •

edited

Loading

yquansah commented Nov 26, 2020

zroubalik left a comment

zroubalik Nov 26, 2020

tomkerkhove commented Nov 26, 2020

yquansah commented Nov 26, 2020 •

edited

Loading

tomkerkhove commented Nov 26, 2020

zroubalik commented Nov 27, 2020 •

edited

Loading

yquansah commented Nov 27, 2020

zroubalik commented Nov 30, 2020

yquansah commented Nov 30, 2020

zroubalik left a comment

feat: adding influxdb scaler #1326

feat: adding influxdb scaler #1326

Conversation

yquansah commented Nov 13, 2020 • edited Loading

Checklist

yquansah commented Nov 13, 2020

Choose a reason for hiding this comment

silenceper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

yquansah commented Nov 13, 2020

yquansah commented Nov 14, 2020

zroubalik commented Nov 16, 2020

yquansah commented Nov 23, 2020

zroubalik commented Nov 23, 2020

yquansah commented Nov 25, 2020

zroubalik commented Nov 25, 2020

yquansah commented Nov 26, 2020

tomkerkhove commented Nov 26, 2020

yquansah commented Nov 26, 2020 • edited Loading

yquansah commented Nov 26, 2020

zroubalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkerkhove commented Nov 26, 2020

yquansah commented Nov 26, 2020 • edited Loading

tomkerkhove commented Nov 26, 2020

zroubalik commented Nov 27, 2020 • edited Loading

yquansah commented Nov 27, 2020

zroubalik commented Nov 30, 2020

yquansah commented Nov 30, 2020

zroubalik left a comment

Choose a reason for hiding this comment

yquansah commented Nov 13, 2020 •

edited

Loading

yquansah commented Nov 26, 2020 •

edited

Loading

yquansah commented Nov 26, 2020 •

edited

Loading

zroubalik commented Nov 27, 2020 •

edited

Loading