feat: Provide New Relic scaler #2387

marcelobartsch-jt · 2021-12-07T04:58:09Z

Provide New Relic scaler.

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable) docs: Provide New Relic scaler keda-docs#591
Changelog has been updated

Relates to #2290

This is a 'redo' of PR #2286 as I had an issue in my REPO and had to start over, but I manage to rescue the files.

Add an option to make empty data returned by query as an error Make sure test-app has at least 1 pod and keda-test-app is 0 before running tests Signed-off-by: Marcelo Bartsch <[email protected]>

Signed-off-by: Marcelo Bartsch <[email protected]>

add noDataErr to tests add missinng threshold to tests add better error description when NerdGraph query fails Signed-off-by: Marcelo Bartsch <[email protected]>

also fix the test so newrelic is replaced by new-relic to keep consistency on the names Signed-off-by: Marcelo Bartsch <[email protected]>

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer · 2021-12-07T12:27:26Z

hi @marcelobartsch
What do we need to set up in order to run e2e tests? I mean, AFAIK New Relic has a free tier that should be enough for our use case. Am I right?

JorTurFer

LGTM in general, but there is a pending change to make e2e test possible.
Please, update the (GitHub) workloads to pass the new environment variables to the actions where tests are executed.
There are 3 places to update:

You should add these lines:

    NEWRELIC_API_KEY: ${{ secrets.NEWRELIC_API_KEY}}
    NEWRELIC_ACCOUNT_ID: ${{ secrets.NEWRELIC_ACCOUNT_ID}}

I have just added them in the repository. After your change, I will trigger e2e test to check it :)

Thanks for this contribution! ❤️

marcelobartsch-jt · 2021-12-07T15:23:59Z

hi @marcelobartsch What do we need to set up in order to run e2e tests? I mean, AFAIK New Relic has a free tier that should be enough for our use case. Am I right?

Yes, in fact all my tests have been done using a free account tier, so I don't mix my test with our company data, so it is perfectly good!

marcelobartsch-jt · 2021-12-07T15:32:36Z

LGTM in general, but there is a pending change to make e2e test possible. Please, update the (GitHub) workloads to pass the new environment variables to the actions where tests are executed. There are 3 places to update:
* https://github.com/kedacore/keda/blob/main/.github/workflows/main-build.yml#L64-L82

* https://github.com/kedacore/keda/blob/main/.github/workflows/nightly-e2e.yml#L18-L36

* https://github.com/kedacore/keda/blob/main/.github/workflows/pr-e2e.yml#L62-L84
You should add these lines:
    NEWRELIC_API_KEY: ${{ secrets.NEWRELIC_API_KEY}}
    NEWRELIC_ACCOUNT_ID: ${{ secrets.NEWRELIC_ACCOUNT_ID}}
I have just added them in the repository. After your change, I will trigger e2e test to check it :)

Thanks for this contribution! ❤️

For the E2E you will also need the NEWRELIC_LICENSE environment variable, the scaler itself don't need it, but the nri-bundle which install the components needed in the kubernetes cluster use it.

NEWRELIC_API_KEY always start with NRAK
NEWRELIC_LICENSE always ENDs with NRAL

This is a common issue , at least for us , we always mix up them when we started years ago :)

I added those lines to the files you mentions

NEWRELIC_ACCOUNT_ID: ${{ secrets.NEWRELIC_ACCOUNT_ID}}
NEWRELIC_API_KEY: ${{ secrets.NEWRELIC_API_KEY}}
NEWRELIC_LICENSE: ${{ secrets.NEWRELIC_LICENSE}}

NEWRELIC_LICENSE is new so probably you will need to add it to secrets.

Signed-off-by: Marcelo Bartsch <[email protected]>

…v variables Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer · 2021-12-07T15:41:11Z

LGTM in general, but there is a pending change to make e2e test possible. Please, update the (GitHub) workloads to pass the new environment variables to the actions where tests are executed. There are 3 places to update:
* https://github.com/kedacore/keda/blob/main/.github/workflows/main-build.yml#L64-L82

* https://github.com/kedacore/keda/blob/main/.github/workflows/nightly-e2e.yml#L18-L36

* https://github.com/kedacore/keda/blob/main/.github/workflows/pr-e2e.yml#L62-L84
You should add these lines:
    NEWRELIC_API_KEY: ${{ secrets.NEWRELIC_API_KEY}}
    NEWRELIC_ACCOUNT_ID: ${{ secrets.NEWRELIC_ACCOUNT_ID}}
I have just added them in the repository. After your change, I will trigger e2e test to check it :)
Thanks for this contribution! ❤️
For the E2E you will also need the NEWRELIC_LICENSE environment variable, the scaler itself don't need it, but the nri-bundle which install the components needed in the kubernetes cluster use it.

NEWRELIC_API_KEY always start with NRAK NEWRELIC_LICENSE always ENDs with NRAL

This is a common issue , at least for us , we always mix up them when we started years ago :)

I added those lines to the files you mentions
NEWRELIC_ACCOUNT_ID: ${{ secrets.NEWRELIC_ACCOUNT_ID}}
NEWRELIC_API_KEY: ${{ secrets.NEWRELIC_API_KEY}}
NEWRELIC_LICENSE: ${{ secrets.NEWRELIC_LICENSE}}
NEWRELIC_LICENSE is new so probably you will need to add it to secrets.

I will add it right now, thanks :)

JorTurFer · 2021-12-07T15:44:20Z

The terms are a bit tricky, but I have updated both following your instructions :)
Thanks

JorTurFer · 2021-12-07T15:45:04Z

/run-e2e new-relic.test*
Update: You can check the progres here

JorTurFer · 2021-12-07T16:29:03Z

/run-e2e new-relic.test*
Update: You can check the progres here

zroubalik · 2021-12-07T18:49:52Z

/run-e2e new-relic.test*
Update: You can check the progres here

Signed-off-by: Marcelo Bartsch <[email protected]>

marcelobartsch-jt · 2021-12-07T19:33:51Z

Woking on fix the E2E test, for some reason it works on local (which I know is no excuse :D )

marcelobartsch-jt · 2021-12-07T20:06:25Z

@JorTurFer any chance the e2e test had limited from where it can download the helm charts? I got this error

Error: failed to download "newrelic/nri-bundle"

https://github.com/kedacore/keda/runs/4448161285?check_suite_focus=true#step:8:96

JorTurFer · 2021-12-07T20:14:33Z

@JorTurFer any chance the e2e test had limited from where it can download the helm charts? I got this error
Error: failed to download "newrelic/nri-bundle"
kedacore/keda/runs/4448161285?check_suite_focus=true#step:8:96

I think that it's not. I will try your PR in my local

Signed-off-by: Marcelo Bartsch <[email protected]>

tests/scalers/new-relic.test.ts

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer · 2021-12-07T21:03:42Z

/run-e2e new-relic.test*
Update: You can check the progres here

JorTurFer

There is a typo in newRelicHelmPackageName. Look the suggestion to see where is and how it could be fixed

tests/scalers/new-relic.test.ts

JorTurFer

Please, update the e2e test to use EU region because our new relic account is in EU. Thanks :)

tests/scalers/new-relic.test.ts

Make use of variables for helm package repo instead of hardcoded it (Thanks Jorge!) Split long lines on the test to make more easy to ready and catch errors Signed-off-by: Marcelo Bartsch <[email protected]>

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer · 2021-12-08T09:55:01Z

/run-e2e new-relic.test*
Update: You can check the progres here

JorTurFer

LGTM!
Thanks fot your contribution ❤️

JorTurFer

Please, update CHANGELOG.md

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer

LGTM!
Thanks fot your contribution ❤️

zroubalik

Generally looking good, I have some minor comments.

pkg/scalers/newrelic_scaler.go

Co-authored-by: Zbynek Roubalik <[email protected]> Signed-off-by: Marcelo Bartsch <[email protected]>

Rename noDataErr to noDataError Move 'new-relic' scaler name to a const so don't repeat the string Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer · 2021-12-09T14:37:22Z

/run-e2e new-relic.test*
Update: You can check the progres here

zroubalik

LGTM, thanks!

zroubalik · 2021-12-09T15:48:52Z

/run-e2e new-relic.test*
Update: You can check the progres here

zroubalik

Please remove metricName from the test and fix the conflict in the workflows & Changelog and we can merge this!

Thanks

pkg/scalers/newrelic_scaler_test.go

Signed-off-by: Marcelo Bartsch <[email protected]>

…elic Signed-off-by: Marcelo Bartsch <[email protected]>

Signed-off-by: Marcelo Bartsch <[email protected]>

Signed-off-by: Marcelo Bartsch <[email protected]> Co-authored-by: Zbynek Roubalik <[email protected]> Signed-off-by: alex60217101990 <[email protected]>

Marcelo Bartsch added 2 commits December 7, 2021 05:55

new-relic scaler

dfbaad3

Add an option to make empty data returned by query as an error Make sure test-app has at least 1 pod and keda-test-app is 0 before running tests Signed-off-by: Marcelo Bartsch <[email protected]>

go.mod/go.sum from main

17d9d29

Signed-off-by: Marcelo Bartsch <[email protected]>

marcelobartsch-jt requested a review from a team as a code owner December 7, 2021 04:58

marcelobartsch-jt changed the title ~~New relic~~ feat: Provide New Relic scaler Dec 7, 2021

Fix noDataErr string case

d8441e4

add noDataErr to tests add missinng threshold to tests add better error description when NerdGraph query fails Signed-off-by: Marcelo Bartsch <[email protected]>

tomkerkhove mentioned this pull request Dec 7, 2021

docs: Provide New Relic scaler kedacore/keda-docs#591

Merged

1 task

Marcelo Bartsch added 2 commits December 7, 2021 09:02

Make metricName optional

1fd4536

also fix the test so newrelic is replaced by new-relic to keep consistency on the names Signed-off-by: Marcelo Bartsch <[email protected]>

fix e2e test

7c33ba4

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer requested changes Dec 7, 2021

View reviewed changes

JorTurFer requested a review from a team December 7, 2021 13:28

Marcelo Bartsch added 2 commits December 7, 2021 16:32

add NewRelic variables to e2e test

0a97185

Signed-off-by: Marcelo Bartsch <[email protected]>

Added missing NEWRELIC_ACCOUNT_ID to test description on requiered en…

ea03c28

…v variables Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer mentioned this pull request Dec 7, 2021

Add New Relic variables for e2e tests #2390

Merged

5 tasks

add go.mod/go.sum files

91ee020

Signed-off-by: Marcelo Bartsch <[email protected]>

marcelobartsch-jt force-pushed the new-relic branch from 6aa6f9c to 91ee020 Compare December 7, 2021 19:29

Marcelo Bartsch added 2 commits December 7, 2021 21:14

Update a variable name to make more sense of it use

06ee0ea

Signed-off-by: Marcelo Bartsch <[email protected]>

fix imports

6c122b1

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer reviewed Dec 7, 2021

View reviewed changes

tests/scalers/new-relic.test.ts Outdated Show resolved Hide resolved

add helm repo update to test

a925013

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer requested changes Dec 7, 2021

View reviewed changes

tests/scalers/new-relic.test.ts Outdated Show resolved Hide resolved

tests/scalers/new-relic.test.ts Outdated Show resolved Hide resolved

JorTurFer requested changes Dec 8, 2021

View reviewed changes

tests/scalers/new-relic.test.ts Show resolved Hide resolved

Marcelo Bartsch added 2 commits December 8, 2021 10:33

another fix to e2e test.

3238c91

Make use of variables for helm package repo instead of hardcoded it (Thanks Jorge!) Split long lines on the test to make more easy to ready and catch errors Signed-off-by: Marcelo Bartsch <[email protected]>

added region as parameter to the e2e test

d9470da

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer approved these changes Dec 8, 2021

View reviewed changes

JorTurFer requested changes Dec 8, 2021

View reviewed changes

update CHANGELOG.md

6ed8d22

Signed-off-by: Marcelo Bartsch <[email protected]>

JorTurFer approved these changes Dec 8, 2021

View reviewed changes

JorTurFer requested a review from a team December 8, 2021 11:48

JorTurFer added this to the v2.6.0 milestone Dec 8, 2021

zroubalik reviewed Dec 9, 2021

View reviewed changes

pkg/scalers/newrelic_scaler.go Outdated Show resolved Hide resolved

pkg/scalers/newrelic_scaler.go Outdated Show resolved Hide resolved

pkg/scalers/newrelic_scaler.go Outdated Show resolved Hide resolved

pkg/scalers/newrelic_scaler.go Outdated Show resolved Hide resolved

Update pkg/scalers/newrelic_scaler.go

8aa68e4

Co-authored-by: Zbynek Roubalik <[email protected]> Signed-off-by: Marcelo Bartsch <[email protected]>

marcelobartsch-jt force-pushed the new-relic branch from c7947d5 to 8aa68e4 Compare December 9, 2021 11:44

Remove metricName from scaler

01ce224

Rename noDataErr to noDataError Move 'new-relic' scaler name to a const so don't repeat the string Signed-off-by: Marcelo Bartsch <[email protected]>

zroubalik approved these changes Dec 9, 2021

View reviewed changes

zroubalik reviewed Jan 3, 2022

View reviewed changes

pkg/scalers/newrelic_scaler_test.go Outdated Show resolved Hide resolved

Marcelo Bartsch added 4 commits January 3, 2022 13:52

Merge remote-tracking branch 'origin/main' into new-relic

ae1b136

Signed-off-by: Marcelo Bartsch <[email protected]>

Merge branch 'main' into new-relic

fb8f0c4

Merge branch 'new-relic' of github.com:marcelobartsch/keda into new-r…

ddc3636

…elic Signed-off-by: Marcelo Bartsch <[email protected]>

remove metricName from tests

22519f7

Signed-off-by: Marcelo Bartsch <[email protected]>

marcelobartsch-jt force-pushed the new-relic branch from a9830a8 to 22519f7 Compare January 3, 2022 12:58

zroubalik merged commit 9c6d7f9 into kedacore:main Jan 3, 2022

marcelobartsch-jt deleted the new-relic branch January 3, 2022 14:17

JorTurFer mentioned this pull request Jan 4, 2022

Revert unintended changes during merge #2434

Closed

5 tasks

zroubalik mentioned this pull request Jan 4, 2022

Restore commits that were unintentionally deleted in #2387 #2435

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Provide New Relic scaler #2387

feat: Provide New Relic scaler #2387

marcelobartsch-jt commented Dec 7, 2021 •

edited by JorTurFer

Loading

JorTurFer commented Dec 7, 2021

JorTurFer left a comment

marcelobartsch-jt commented Dec 7, 2021

marcelobartsch-jt commented Dec 7, 2021

JorTurFer commented Dec 7, 2021

JorTurFer commented Dec 7, 2021

JorTurFer commented Dec 7, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Dec 7, 2021 •

edited by github-actions bot

Loading

zroubalik commented Dec 7, 2021 •

edited by github-actions bot

Loading

marcelobartsch-jt commented Dec 7, 2021

marcelobartsch-jt commented Dec 7, 2021 •

edited

Loading

JorTurFer commented Dec 7, 2021

JorTurFer commented Dec 7, 2021 •

edited by github-actions bot

Loading

JorTurFer left a comment

JorTurFer left a comment

JorTurFer commented Dec 8, 2021 •

edited by github-actions bot

Loading

JorTurFer left a comment

JorTurFer left a comment

JorTurFer left a comment •

edited

Loading

zroubalik left a comment

JorTurFer commented Dec 9, 2021 •

edited by github-actions bot

Loading

zroubalik left a comment

zroubalik commented Dec 9, 2021 •

edited by github-actions bot

Loading

zroubalik left a comment

feat: Provide New Relic scaler #2387

feat: Provide New Relic scaler #2387

Conversation

marcelobartsch-jt commented Dec 7, 2021 • edited by JorTurFer Loading

Checklist

JorTurFer commented Dec 7, 2021

JorTurFer left a comment

Choose a reason for hiding this comment

marcelobartsch-jt commented Dec 7, 2021

marcelobartsch-jt commented Dec 7, 2021

JorTurFer commented Dec 7, 2021

JorTurFer commented Dec 7, 2021

JorTurFer commented Dec 7, 2021 • edited by github-actions bot Loading

JorTurFer commented Dec 7, 2021 • edited by github-actions bot Loading

zroubalik commented Dec 7, 2021 • edited by github-actions bot Loading

marcelobartsch-jt commented Dec 7, 2021

marcelobartsch-jt commented Dec 7, 2021 • edited Loading

JorTurFer commented Dec 7, 2021

JorTurFer commented Dec 7, 2021 • edited by github-actions bot Loading

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Dec 8, 2021 • edited by github-actions bot Loading

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer left a comment • edited Loading

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

JorTurFer commented Dec 9, 2021 • edited by github-actions bot Loading

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Dec 9, 2021 • edited by github-actions bot Loading

zroubalik left a comment

Choose a reason for hiding this comment

marcelobartsch-jt commented Dec 7, 2021 •

edited by JorTurFer

Loading

JorTurFer commented Dec 7, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Dec 7, 2021 •

edited by github-actions bot

Loading

zroubalik commented Dec 7, 2021 •

edited by github-actions bot

Loading

marcelobartsch-jt commented Dec 7, 2021 •

edited

Loading

JorTurFer commented Dec 7, 2021 •

edited by github-actions bot

Loading

JorTurFer commented Dec 8, 2021 •

edited by github-actions bot

Loading

JorTurFer left a comment •

edited

Loading

JorTurFer commented Dec 9, 2021 •

edited by github-actions bot

Loading

zroubalik commented Dec 9, 2021 •

edited by github-actions bot

Loading