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

feat: Provide New Relic scaler #2387

Merged
merged 20 commits into from
Jan 3, 2022
Merged

feat: Provide New Relic scaler #2387

merged 20 commits into from
Jan 3, 2022

Conversation

marcelobartsch-jt
Copy link
Contributor

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

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.

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 <marcelo.bartsch@letgo.com>
go.mod/go.sum from main
17d9d29 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@marcelobartsch-jt marcelobartsch-jt requested a review from a team as a code owner December 7, 2021 04:58
@marcelobartsch-jt 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 <marcelo.bartsch@letgo.com>
@tomkerkhove tomkerkhove mentioned this pull request Dec 7, 2021
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 <marcelo.bartsch@letgo.com>
fix e2e test
7c33ba4 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@JorTurFer
Copy link
Member

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
JorTurFer requested changes Dec 7, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! ❤️

@JorTurFer JorTurFer requested a review from a team December 7, 2021 13:28
@marcelobartsch-jt
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Marcelo Bartsch added 2 commits December 7, 2021 16:32
add NewRelic variables to e2e test
0a97185 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
Added missing NEWRELIC_ACCOUNT_ID to test description on requiered en…
ea03c28 
…v variables

Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@JorTurFer
Copy link
Member

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
Copy link
Member

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

@JorTurFer
Copy link
Member

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

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

@JorTurFer
Copy link
Member

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

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

@JorTurFer JorTurFer mentioned this pull request Dec 7, 2021
Merged
5 tasks
@zroubalik
Copy link
Member

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

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

add go.mod/go.sum files
91ee020 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@marcelobartsch-jt
Copy link
Contributor Author

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

@marcelobartsch-jt
Copy link
Contributor Author

marcelobartsch-jt commented Dec 7, 2021 •
edited
Loading

@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
Copy link
Member

@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

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 <marcelo.bartsch@letgo.com>
fix imports
6c122b1 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
add helm repo update to test
a925013 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@JorTurFer
Copy link
Member

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

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

JorTurFer
JorTurFer requested changes Dec 7, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
tests/scalers/new-relic.test.ts Outdated Show resolved Hide resolved
JorTurFer
JorTurFer requested changes Dec 8, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 <marcelo.bartsch@letgo.com>
added region as parameter to the e2e test
d9470da 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@JorTurFer
Copy link
Member

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

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

JorTurFer
JorTurFer approved these changes Dec 8, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks fot your contribution ❤️

JorTurFer
JorTurFer requested changes Dec 8, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, update CHANGELOG.md

update CHANGELOG.md
6ed8d22 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
JorTurFer
JorTurFer approved these changes Dec 8, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks fot your contribution ❤️

@JorTurFer JorTurFer requested a review from a team December 8, 2021 11:48
@JorTurFer JorTurFer added this to the v2.6.0 milestone Dec 8, 2021
zroubalik
zroubalik reviewed Dec 9, 2021
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looking good, I have some minor comments.

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
@zroubalik
Update pkg/scalers/newrelic_scaler.go
8aa68e4 
Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
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 <marcelo.bartsch@letgo.com>
@JorTurFer
Copy link
Member

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

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

zroubalik
zroubalik approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@zroubalik
Copy link
Member

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

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

zroubalik
zroubalik reviewed Jan 3, 2022
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 <marcelo.bartsch@letgo.com>
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 <marcelo.bartsch@letgo.com>
remove metricName from tests
22519f7 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
@zroubalik zroubalik merged commit 9c6d7f9 into kedacore:main Jan 3, 2022
@marcelobartsch-jt marcelobartsch-jt deleted the new-relic branch January 3, 2022 14:17
@JorTurFer JorTurFer mentioned this pull request Jan 4, 2022
Closed
5 tasks
@zroubalik zroubalik mentioned this pull request Jan 4, 2022
Merged
1 task
alex60217101990 pushed a commit to dysnix/keda that referenced this pull request Jan 10, 2022
@zroubalik @alex60217101990
feat: Provide New Relic scaler (kedacore#2387)
2b86f5c 
Signed-off-by: Marcelo Bartsch <marcelo.bartsch@letgo.com>
Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Signed-off-by: alex60217101990 <alex6021710@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@marcelobartsch-jt @JorTurFer @zroubalik