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

Graphite scaler should use latest datapoint, not earliest, returned #2365

Merged
merged 5 commits into from
Dec 1, 2021
Merged

Graphite scaler should use latest datapoint, not earliest, returned #2365

merged 5 commits into from
Dec 1, 2021

Conversation

bpinske
Copy link
Contributor

@bpinske bpinske commented Nov 29, 2021 •
edited by zroubalik
Loading

Graphite query responses return values in order of oldest to latest. With the final element of the response array being the most up-to-date timestamp. Graphite's scaler should be on the most recent datapoint, not on data potentially hours old.

https://graphite-api.readthedocs.io/en/latest/api.html#json
https://github.com/kedacore/keda/blob/main/pkg/scalers/graphite_scaler.go#L197

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Changelog has been updated

Fixes #2366

@bpinske bpinske requested a review from a team as a code owner November 29, 2021 19:57
Graphite scaler should use latest datapoint, not earliest, returned.
ca8a90b 
Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
@bpinske bpinske force-pushed the brandon/graphite-scaler-latest-datapoint branch from bcf752b to ca8a90b Compare November 29, 2021 19:58
@JorTurFer
Copy link
Member

Hi!,
Is this related with any issue? Could you explain a bit the change and it reasons? (Maybe opening an issue)
Thanks! ❤️

Changelog
87e8449 
Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
@bpinske bpinske changed the title Graphite scaler should use latest datapoint, not earliest, returned. Fixes #2366 Graphite scaler should use latest datapoint, not earliest, returned. Nov 29, 2021
@bpinske bpinske changed the title Fixes #2366 Graphite scaler should use latest datapoint, not earliest, returned. Graphite scaler should use latest datapoint, not earliest, returned. Fixes #2366 Nov 29, 2021
@bpinske bpinske changed the title Graphite scaler should use latest datapoint, not earliest, returned. Fixes #2366 Graphite scaler should use latest datapoint, not earliest, returned. Fixes issue #2366 Nov 29, 2021
@bpinske
Copy link
Contributor Author

bpinske commented Nov 29, 2021

Hi!, Is this related with any issue? Could you explain a bit the change and it reasons? (Maybe opening an issue) Thanks! ❤️

#2366

Here you can see the graphite server response returns values in order of oldest to latest. With the final element of the response array being the most up-to-date timestamp. Graphite's scaler should be acting on the final element of the response array, not the first.

https://graphite-api.readthedocs.io/en/latest/api.html#json
https://github.com/kedacore/keda/blob/main/pkg/scalers/graphite_scaler.go#L197

@JorTurFer
Copy link
Member

JorTurFer commented Nov 29, 2021 •
edited by github-actions bot
Loading

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

JorTurFer
JorTurFer requested changes Nov 29, 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, only small nit
Thanks for the contribution :)

CHANGELOG.md Outdated Show resolved Hide resolved
Brandon Pinske added 2 commits November 29, 2021 12:20
Changelog whitespace
2eb4c21 
Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
Changelog whitespace
08d8cdb 
Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
@tomkerkhove tomkerkhove changed the title Graphite scaler should use latest datapoint, not earliest, returned. Fixes issue #2366 Graphite scaler should use latest datapoint, not earliest, returned Nov 30, 2021
@tomkerkhove
Copy link
Member

I'm going to be a pain @bpinske, but would you mind updating the PR description to be more complete please?

zroubalik
zroubalik reviewed Nov 30, 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.

Generaly looking good, one Q.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scalers/graphite_scaler.go Show resolved Hide resolved
Fix changelog, add check for whether zero data is returned
5f79338 
Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
zroubalik
zroubalik approved these changes Nov 30, 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

@zroubalik zroubalik merged commit 8e4f201 into kedacore:main Dec 1, 2021
@JorTurFer JorTurFer mentioned this pull request Dec 5, 2021
Merged
5 tasks
bpinske added a commit to bpinske/keda that referenced this pull request Jan 19, 2022
@bpinske
Graphite scaler should use latest datapoint, not earliest, returned (k…
73cbe54 
…edacore#2365)

Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.com>
bpinske added a commit to bpinske/keda that referenced this pull request Apr 28, 2022
@bpinske
Graphite scaler should use latest datapoint, not earliest, returned (k…
617f35c 
…edacore#2365)

Signed-off-by: Brandon Pinske <brandon.pinske@crowdstrike.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
No milestone
Development

Successfully merging this pull request may close these issues.

Graphite scaler is returning the earliest datapoint of a query, not the latest.
4 participants
@bpinske @JorTurFer @tomkerkhove @zroubalik