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.

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

docs: Provide New Relic scaler #591

Merged
merged 22 commits into from
Jan 3, 2022
Merged

docs: Provide New Relic scaler #591

merged 22 commits into from
Jan 3, 2022

Conversation

JoshuaJackson-jobvite
Copy link
Contributor

@JoshuaJackson-jobvite JoshuaJackson-jobvite commented Nov 22, 2021

Why do we need this change?

Newrelic is a common Application Performance Monitoring(APM) tool that provides capabilities for team's to take relevant actions based on the gathered data/metrics from within their system.

What effects does this change have?

  • Add's in newrelic scaler documentation for 2.6+ release of keda

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Relates to kedacore/keda#2387
Relates to kedacore/keda#2290

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@JoshuaJackson-jobvite
Copy link
Contributor Author

Supporting documentation for: kedacore/keda#2286 and kedacore/keda#2290

…iona and set values

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@tomkerkhove tomkerkhove changed the title Newrelic Scaler Supporting documentation docs: Provide New Relic scaler Nov 22, 2021
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
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.

Is the prefix nr required? I mean, this is New Relic scaler, so I understand that parameters are related with it.
WDYT?
BTW, thanks for this amazing contribution!! ❤️

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Did a first run, but mainly nits - THanks for your PR!

…bit more look at the code

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
…ew Relic

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
…riable> names

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
… newrelic

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Just a few remarks

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
…work natively

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Almost there, just a few alignments.

Thanks!

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@JorTurFer
Copy link
Member

hi @JoshuaJackson-jobvite,
We have just released the v2.5, could you update your PR and point your changes against v2.6 please?
Basically, you will need to move the file to the folder 2.6 and the version where this is available from.
Thanks a lot!

@JoshuaJackson-jobvite
Copy link
Contributor Author

JoshuaJackson-jobvite commented Nov 25, 2021

hi @JoshuaJackson-jobvite, We have just released the v2.5, could you update your PR and point your changes against v2.6 please? Basically, you will need to move the file to the folder 2.6 and the version where this is available from. Thanks a lot!

Done, thanks for the heads up that 2.5 was marked/released. Updated the main message for the pr to reflect the same change to 2.6 instead of 2.5 as well.

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@tomkerkhove
Copy link
Member

Don't forget about this consistency comment please?
#591 (comment)

And thank you for all your effort!

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Requesting some consistency changes as per #591 (comment)

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@JoshuaJackson-jobvite
Copy link
Contributor Author

Requesting some consistency changes as per #591 (comment)

@tomkerkhove hopefully the new update is more in line with what you are looking for.

…everaged n the crd, no reason to really mention it as such

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@tomkerkhove
Copy link
Member

Thanks a ton @JoshuaJackson-jobvite !

Copy link
Member

@tomkerkhove tomkerkhove 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 for this!

account: 1234567
# Required: QueryKey - Api key to connect to New Relic
queryKey: "NRAK-xxxxxxxxxxxxxxxxxxxxxxxxxxx"
# Optional: nrRegion - Region to query data for
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice, to mention that US is default in here as well.

Copy link

@marcelobartsch-jt marcelobartsch-jt Dec 9, 2021

Choose a reason for hiding this comment

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

shall I rename queryKey to userApiKey to make it more similar to the name it have in new relic?
In new relic this is 'ApiKey' of type 'User' it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now mention that US is the default in this yaml section

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.

I don't see noDataErr in here and I'd name it probably noDataError or somethig similar, no need to shorten it here. I

@marcelobartsch-jt
Copy link

I don't see noDataErr in here and I'd name it probably noDataError or somethig similar, no need to shorten it here. I

Agreed, I'm changing this to noDataError in the scaler.

Why do we need this change?
=======================
A new property was added for what to do when No data is returned by the
nrql message. Need to document this

What effects does this change have?
=======================
* Add noDataError information for users
* Update the yaml section to mention the default value for region is US

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
that's explicit in the documentation

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
…ed to add

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Comment on lines 24 to 25
# Required: metricName
metricName: "duration"
Copy link
Member

Choose a reason for hiding this comment

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

metricName is not being used anymore

- `account` - The account within New Relic that the request should be targeted against.
- `queryKey` - The API key that will be leveraged to connect to New Relic and make requests. [official documentation](https://docs.newrelic.com/docs/apis/intro-apis/new-relic-api-keys/)
- `region` - The region to connect to for the New Relic apis. (Values: `LOCAL`, `EU`, `STAGING`, `US`, Default: `US`, Optional)
- `metricName` - The metric to pull from the query result.
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
@zroubalik zroubalik merged commit 02244a9 into kedacore:main Jan 3, 2022
daniel-yavorovich pushed a commit to dysnix/keda-docs that referenced this pull request Jan 18, 2022
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Daniel Yavorovych <daniel@dysnix.com>
daniel-yavorovich pushed a commit to dysnix/keda-docs that referenced this pull request Jan 18, 2022
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Daniel Yavorovych <daniel@dysnix.com>
daniel-yavorovich pushed a commit to dysnix/keda-docs that referenced this pull request Jan 26, 2022
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Daniel Yavorovych <daniel@dysnix.com>
daniel-yavorovich pushed a commit to dysnix/keda-docs that referenced this pull request Jan 26, 2022
Signed-off-by: Joshua Jackson <joshua.jackson@jobvite-inc.com>
Signed-off-by: Daniel Yavorovych <daniel@dysnix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants