Fix stackdriver client returning 0 for metric types of double #3788

octothorped · 2022-10-31T16:50:48Z

Updated the Stackdriver client to detect if a metric value type of double or int64 has been retrieved. Then it properly casts the value to the float64 to be used downstream by the scalars.
This fixes a bug in which the Stackdriver metric is always returning a value of 0 for a metric value type of double. This is because the monitoring client is asserting the value type and if false will return 0.

Checklist

When introducing a new scaler, I agree with the scaling governance policy
Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
Changelog has been updated and is aligned with our changelog requirements

Fixes #3777

JorTurFer

Looking good
Thanks a lot for the fix, I left some comments inline and IDK if adding test to avoid regressions here is possible, but I'd be nice

pkg/scalers/gcp_pubsub_scaler.go

CHANGELOG.md

pkg/scalers/gcp_stackdriver_scaler.go

JorTurFer · 2022-10-31T22:46:37Z

/run-e2e gcp*
Update: You can check the progress here

octothorped · 2022-10-31T22:59:05Z

IDK if adding test to avoid regressions here is possible, but I'd be nice

Appreciate the feedback. It would be nice to have a test. I tried looking into but we are using the Google API so it makes it a little tougher. We could abstract the retrieval of the metrics call into an interface and use that throughout though. Then mock it in the tests. It might be okay assuming the client footprint is small.

I'll update the PR based on your comments first and then see if I can get a test going.

JorTurFer · 2022-10-31T23:20:53Z

Appreciate the feedback. It would be nice to have a test. I tried looking into but we are using the Google API so it makes it a little tougher. We could abstract the retrieval of the metrics call into an interface and use that throughout though. Then mock it in the tests. It might be okay assuming the client footprint is small.

Could we test this using e2e tests? I mean, we have e2e test using gcp services, so if we can force this behaviour there (IDK how), we can check it, we don't need to mock anything. I asked because maybe unit test would cover this, but if we need e2e tests, we can do it. Maybe we could just update current e2e tests to enforce the usage of float64

zroubalik · 2022-11-28T10:41:27Z

@octothorped any update on this please? we plan to do a release in ~2 weeks.

octothorped · 2022-11-28T16:56:45Z

@octothorped any update on this please? we plan to do a release in ~2 weeks.

Sorry about the long delay. I will try to get an update in the next few days. I have been swapped with other work and this suddenly took a backseat. I will see about getting this finished.

Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

tomkerkhove · 2022-12-02T08:39:05Z

@JorTurFer @zroubalik Can you re-review this PR please to see if we need to change things before our release on Thrusday?

@octothorped Can you fix the open merge conflicts please?

JorTurFer · 2022-12-02T17:39:37Z

/run-e2e gcp*
Update: You can check the progress here

JorTurFer · 2022-12-02T17:44:29Z

One thing, if activationValue is float64, value should be too. Could you update it?
This change also requires a documentation update to reflect that it's float.
Basically, you need to update these pages:

You can check how them should be comparing the with threshold and activationThresold from prometheus scaler

octothorped · 2022-12-02T23:59:16Z

Updated the target value parameters to be float64 as well.

Put out a PR that updates the documentation for the float changes.

Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

JorTurFer

LGTM!
Thanks for the improvement! Let me trigger e2e test and once they pass, the PR is ready :)

JorTurFer · 2022-12-03T14:46:57Z

/run-e2e gcp*
Update: You can check the progress here

…re#3788) * Update stackdriver client to handle metrics of value type double Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * move change log note to below general Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * parse activation value as float64 Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * change target value to float64 for GCP pub/sub and stackdriver Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

…re#3788) * Update stackdriver client to handle metrics of value type double Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * move change log note to below general Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * parse activation value as float64 Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * change target value to float64 for GCP pub/sub and stackdriver Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>

…re#3788) * Update stackdriver client to handle metrics of value type double Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * move change log note to below general Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * parse activation value as float64 Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * change target value to float64 for GCP pub/sub and stackdriver Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>

* fix: CVE-2022-3172 (#3693) Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fix: Respect optional parameter inside envs for ScaledJobs (#3694) Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fix(prometheus scaler): Detect Inf before casting float to int (#3762) * fix(prometheus scaler): Detect Inf before casting float to int Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es> * Improve the log message Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es> Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fix(nats-jetstream): correctly count messages that should be redelivered (waiting for ack) towards keda value (#3809) * fix: keda now include the messages that should be retried in the count of pending messages used for scaling Signed-off-by: Antoine Laffargue <antoine.laffargue@gmail.com> * chore: update changelog Signed-off-by: Antoine Laffargue <antoine.laffargue@gmail.com> Signed-off-by: Antoine Laffargue <antoine.laffargue@gmail.com> Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> * NewRelic scaler crashes on logging (#3946) Signed-off-by: Laszlo Kishalmi <laszlo.kishalmi@partech.com> Signed-off-by: Laszlo Kishalmi <laszlo.kishalmi@partech.com> Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Fix stackdriver client returning 0 for metric types of double (#3788) * Update stackdriver client to handle metrics of value type double Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * move change log note to below general Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * parse activation value as float64 Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> * change target value to float64 for GCP pub/sub and stackdriver Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Fixing conflicts after cherry-pick Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> * fix: Close is called twice on PushScaler's deletion (#3599) Signed-off-by: ytz <1020560484@qq.com> Signed-off-by: taenyang <1020560484@qq.com> Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> * fix/datadog-scaler-null-last-point (#3954) Signed-off-by: Tony Lee <dogzzdogzz@gmail.com> Signed-off-by: Tony Lee <tony.lee@shopback.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: Tony Lee <tony.lee@shopback.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com> Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> * fix(mongodb): escape username and password (#3989) Fixes #3992 Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Hacking generated files to version CI expects Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> * Updating aws-sdk and golang packages to fix CVEs Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Updating golang/text package to fix CVE Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Using same version of aws sdk as in main Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es> Signed-off-by: Antoine Laffargue <antoine.laffargue@gmail.com> Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com> Signed-off-by: Laszlo Kishalmi <laszlo.kishalmi@partech.com> Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com> Signed-off-by: ytz <1020560484@qq.com> Signed-off-by: taenyang <1020560484@qq.com> Signed-off-by: Tony Lee <dogzzdogzz@gmail.com> Signed-off-by: Tony Lee <tony.lee@shopback.com> Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es> Co-authored-by: Antoine LAFFARGUE <antoine.laffargue@gmail.com> Co-authored-by: Laszlo Kishalmi <laszlo.kishalmi@gmail.com> Co-authored-by: Eric Takemoto <eric.takemoto@gocrisp.com> Co-authored-by: taenyang <1020560484@qq.com> Co-authored-by: Tony Lee <dogzzdogzz@gmail.com> Co-authored-by: Tony Lee <tony.lee@shopback.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>

octothorped requested a review from a team as a code owner October 31, 2022 16:50

octothorped changed the title ~~Fix stackdriver client double metric~~ Fix stackdriver client returning 0 for metric types of double Oct 31, 2022

JorTurFer reviewed Oct 31, 2022

View reviewed changes

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

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

CHANGELOG.md Outdated Show resolved Hide resolved

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

octothorped added 3 commits November 30, 2022 05:40

Update stackdriver client to handle metrics of value type double

402ffd7

Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

move change log note to below general

e8bb52f

Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

parse activation value as float64

24dafb1

Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

octothorped force-pushed the fix-stackdriver-client-double-metric branch from cb780e5 to 24dafb1 Compare November 30, 2022 07:54

Merge branch 'main' into fix-stackdriver-client-double-metric

c69e250

octothorped requested a review from JorTurFer December 2, 2022 16:52

octothorped mentioned this pull request Dec 2, 2022

Update documents to reference float64 for GCP pub sub and stackdriver kedacore/keda-docs#995

Merged

1 task

change target value to float64 for GCP pub/sub and stackdriver

6809f98

Signed-off-by: Eric Takemoto <24865872+octothorped@users.noreply.github.com>

octothorped force-pushed the fix-stackdriver-client-double-metric branch from 2cd39eb to 6809f98 Compare December 3, 2022 00:25

JorTurFer approved these changes Dec 3, 2022

View reviewed changes

JorTurFer merged commit d5f3349 into kedacore:main Dec 5, 2022

JorTurFer mentioned this pull request Jan 17, 2023

Release v2.8.2 #4132

Closed

1 task

pedro-stanaka mentioned this pull request Jan 18, 2023

Backport fixes to release 2.8 #4135

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stackdriver client returning 0 for metric types of double #3788

Fix stackdriver client returning 0 for metric types of double #3788

octothorped commented Oct 31, 2022 •

edited

Loading

JorTurFer left a comment

JorTurFer commented Oct 31, 2022 •

edited by github-actions bot

Loading

octothorped commented Oct 31, 2022

JorTurFer commented Oct 31, 2022 •

edited

Loading

zroubalik commented Nov 28, 2022

octothorped commented Nov 28, 2022

tomkerkhove commented Dec 2, 2022

JorTurFer commented Dec 2, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Dec 2, 2022

octothorped commented Dec 2, 2022

JorTurFer left a comment

JorTurFer commented Dec 3, 2022 •

edited by github-actions bot

Loading

Fix stackdriver client returning 0 for metric types of double #3788

Fix stackdriver client returning 0 for metric types of double #3788

Conversation

octothorped commented Oct 31, 2022 • edited Loading

Checklist

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Oct 31, 2022 • edited by github-actions bot Loading

octothorped commented Oct 31, 2022

JorTurFer commented Oct 31, 2022 • edited Loading

zroubalik commented Nov 28, 2022

octothorped commented Nov 28, 2022

tomkerkhove commented Dec 2, 2022

JorTurFer commented Dec 2, 2022 • edited by github-actions bot Loading

JorTurFer commented Dec 2, 2022

octothorped commented Dec 2, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Dec 3, 2022 • edited by github-actions bot Loading

octothorped commented Oct 31, 2022 •

edited

Loading

JorTurFer commented Oct 31, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Oct 31, 2022 •

edited

Loading

JorTurFer commented Dec 2, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Dec 3, 2022 •

edited by github-actions bot

Loading