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

Prometheus scaler add option to ignore null value #3073

Merged
merged 11 commits into from
Jun 1, 2022
Merged

Prometheus scaler add option to ignore null value #3073

merged 11 commits into from
Jun 1, 2022

Conversation

bamboo12366
Copy link
Contributor

@bamboo12366 bamboo12366 commented May 27, 2022 •
edited by zroubalik
Loading

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • [N/A] 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)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #3065
Relates to #3064
Relates to kedacore/keda-docs#769 (docs)

@bamboo12366
add ignoreNullValues in prometheus scaler
5b7ac8f 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366
using enable/disable insteads of true/false
656b300 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366
add metadata test for ignoreNullValues
f7ded30 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366
add execute prom query test
4bede22 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366
update CHANGELOG
7393d8a 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366 bamboo12366 requested a review from a team as a code owner May 27, 2022 07:46
@bamboo12366 bamboo12366 mentioned this pull request May 27, 2022
Closed
JorTurFer
JorTurFer reviewed May 27, 2022
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.

The change looks good in general. Thanks for this improvement
Some nits to be addressed:

  • There is a test failing TestPrometheusScalerCortexHeader
  • Open a PR in keda-docs updating the doc about prometheus scaler (only in the latest version)
  • Inline comment in changelog

Apart from that, IDK if it makes sense adding another e2e test to check this new behaviour, WDYT?

CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from zroubalik May 27, 2022 20:32
@bamboo12366
update CHANGELOG and test according to review
88e7fe7 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366
Copy link
Contributor Author

The change looks good in general. Thanks for this improvement Some nits to be addressed:

  • There is a test failing TestPrometheusScalerCortexHeader
  • Open a PR in keda-docs updating the doc about prometheus scaler (only in the latest version)
  • Inline comment in changelog

Apart from that, IDK if it makes sense adding another e2e test to check this new behaviour, WDYT?

The change looks good in general. Thanks for this improvement Some nits to be addressed:

  • There is a test failing TestPrometheusScalerCortexHeader
  • Open a PR in keda-docs updating the doc about prometheus scaler (only in the latest version)
  • Inline comment in changelog

Apart from that, IDK if it makes sense adding another e2e test to check this new behaviour, WDYT?

The changelog and TestPrometheusScalerCortexHeader already updated.
I will update the keda-doc these days.
I think it unnecessary to do e2e in this case, cause I already cover this one in test file by mocking the prometheus return data, which will be the same thing in e2e.

@bamboo12366 bamboo12366 changed the title Bamboo/prometheus scaler add option to ignore null value prometheus scaler add option to ignore null value May 29, 2022
@bamboo12366
update scaller to pass the ci
a4be858 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366 bamboo12366 mentioned this pull request May 29, 2022
Merged
1 task
@bamboo12366
Copy link
Contributor Author

The PR related to keda-doc: kedacore/keda-docs#769

tomkerkhove
tomkerkhove suggested changes May 30, 2022
View reviewed changes
pkg/scalers/prometheus_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/prometheus_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/prometheus_scaler.go Outdated Show resolved Hide resolved
@bamboo12366
use true/false insteads of enable/disable
2f2cfe7 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented May 30, 2022 •
edited by github-actions bot
Loading

/run-e2e prometheus*
Update: You can check the progres here

JorTurFer
JorTurFer reviewed May 30, 2022
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, only 1 thing inline
Thanks for the improvement! ❤️

pkg/scalers/prometheus_scaler.go Outdated Show resolved Hide resolved
zroubalik
zroubalik reviewed May 30, 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.

Looking good, just a few nits.

@NaveLevi would be nice if you can take a look on this

pkg/scalers/prometheus_scaler.go Outdated Show resolved Hide resolved
@bamboo12366
update type and delete else condition
b620db4 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@zroubalik
Copy link
Member

Static check fails as well: https://github.com/kedacore/keda/runs/6652557343?check_suite_focus=true#step:6:84

@bamboo12366
fix typo
5b80eda 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366
fix lint
792357f 
Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented May 31, 2022 •
edited by github-actions bot
Loading

/run-e2e prometheus*
Update: You can check the progres here

zroubalik
zroubalik approved these changes May 31, 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.

LGTM

@zroubalik zroubalik changed the title prometheus scaler add option to ignore null value Prometheus scaler add option to ignore null value May 31, 2022
JorTurFer
JorTurFer approved these changes May 31, 2022
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! ❤️

@tomkerkhove tomkerkhove merged commit 97fb357 into kedacore:main Jun 1, 2022
@yardenshoham yardenshoham mentioned this pull request Mar 7, 2023
Merged
4 tasks
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

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
ok-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prometheus scaler - add option to consider null values as errors
4 participants
@bamboo12366 @JorTurFer @zroubalik @tomkerkhove