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

add pause metric in prometheus for scaledobject #5045

Merged
merged 6 commits into from
Oct 30, 2023
Merged

add pause metric in prometheus for scaledobject #5045

merged 6 commits into from
Oct 30, 2023

Conversation

geoffrey1330
Copy link
Contributor

@geoffrey1330 geoffrey1330 commented Oct 4, 2023 •
edited by JorTurFer
Loading

Provide a description of what has been changed

Checklist

Fixes #

Relates to #4430
Relates to kedacore/keda-docs#1111

@geoffrey1330 geoffrey1330 requested a review from a team as a code owner October 4, 2023 01:00
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik here is my PR and I made all the fix referenced in #4446

@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik could you please review my PR

JorTurFer
JorTurFer reviewed Oct 6, 2023
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.

Looking good! Only one stuff with changelog
@zroubalik PTAL too

CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Oct 6, 2023 •
edited by github-actions bot
Loading

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

JorTurFer
JorTurFer reviewed Oct 6, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
JorTurFer
JorTurFer reviewed Oct 6, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
pkg/metricscollector/opentelemetry.go Outdated Show resolved Hide resolved
zroubalik
zroubalik requested changes Oct 6, 2023
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 make sure to only set/unset the Prometheus metrics for paused state in this PR. The functionality for pausing scaling is already there.

The original PR that you used as a source for your work is quite old, in the meantime there were changes in the code and in the way we pause ScaledObjects. For example see this PR: #4809

controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
pkg/metricscollector/opentelemetry.go Outdated Show resolved Hide resolved
pkg/metricscollector/opentelemetry.go Outdated Show resolved Hide resolved
zroubalik
zroubalik reviewed Oct 10, 2023
View reviewed changes
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
@geoffrey1330
Copy link
Contributor Author

@zroubalik Could you please review my PR when you have the time.
cc @JorTurFer

zroubalik
zroubalik reviewed Oct 12, 2023
View reviewed changes
pkg/metricscollector/opentelemetry.go Outdated Show resolved Hide resolved
pkg/metricscollector/prommetrics.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
zroubalik
zroubalik reviewed Oct 12, 2023
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 make sure that your PR only includes necessary changes and doesn't contain unnecessary changes like empty lines etc.

pkg/metricscollector/prommetrics.go Outdated Show resolved Hide resolved
pkg/metricscollector/prommetrics.go Outdated Show resolved Hide resolved
pkg/metricscollector/prommetrics.go Outdated Show resolved Hide resolved
pkg/metricscollector/prommetrics.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
tests/sequential/prometheus_metrics/prometheus_metrics_test.go Outdated Show resolved Hide resolved
tests/sequential/opentelemetry_metrics/opentelemetry_metrics_test.go Outdated Show resolved Hide resolved
zroubalik
zroubalik reviewed Oct 12, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
zroubalik
zroubalik requested changes Oct 13, 2023
View reviewed changes
controllers/keda/scaledobject_finalizer.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
@geoffrey1330 geoffrey1330 mentioned this pull request Oct 16, 2023
Merged
1 task
@geoffrey1330
Copy link
Contributor Author

@zroubalik could you please review my PR again. as i have fixed all suggested changes.
cc @JorTurFer

zroubalik
zroubalik reviewed Oct 16, 2023
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.

@geoffrey1330 there are still unresolved comments on this PR

controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
@geoffrey1330
Copy link
Contributor Author

@geoffrey1330 there are still unresolved comments on this PR

I will go ahead and mark all unresolved comments as resolved.

@zroubalik
Copy link
Member

@geoffrey1330 there are still unresolved comments on this PR

I will go ahead and mark all unresolved comments as resolved.

No, it means that these things should be resolved.

@geoffrey1330
Copy link
Contributor Author

@geoffrey1330 there are still unresolved comments on this PR

I will go ahead and mark all unresolved comments as resolved.

No, it means that these things should be resolved.

Resolved most of them. will check for the one's I'm yet to resolve.

@zroubalik
Copy link
Member

zroubalik commented Oct 19, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330 geoffrey1330 mentioned this pull request Oct 19, 2023
Closed
@zroubalik
Copy link
Member

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

@geoffrey1330 unfortunately the e2e has failed, have you tried to run it locally?

@geoffrey1330
Copy link
Contributor Author

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

@geoffrey1330 unfortunately the e2e has failed, have you tried to run it locally?

Not yet. Let me try that now.

@geoffrey1330
Copy link
Contributor Author

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

@geoffrey1330 unfortunately the e2e has failed, have you tried to run it locally?

@zroubalik I'm using make e2e-test-local to test locally any advise on this?

@zroubalik
Copy link
Member

zroubalik commented Oct 23, 2023 •
edited by github-actions bot
Loading

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

@tomkerkhove
Copy link
Member

tomkerkhove commented Oct 24, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330
Copy link
Contributor Author

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

@tomkerkhove Could you please trigger /run-e2e sequential again

@zroubalik
Copy link
Member

zroubalik commented Oct 24, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik could you please trigger /run-e2e sequential as I've fixed all failing test

@zroubalik
Copy link
Member

zroubalik commented Oct 24, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330
Copy link
Contributor Author

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

@zroubalik I see the test failed but it passed successfully locally.

@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik could you please trigger /run-e2e sequential as I've fixed all failing test

@JorTurFer
Copy link
Member

JorTurFer commented Oct 25, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330
one commit on feat/pause-metric
35c0412 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Merge branch 'main' into feat/pause-metric
886a55c 
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
@geoffrey1330
Copy link
Contributor Author

Hi @JorTurFer could you please trigger /run-e2e sequential the initial trigger passed successfully but I fixed DCO issue and would love to test e2e also on this new change

@JorTurFer
Copy link
Member

JorTurFer commented Oct 25, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330
renamed RecordScalerPaused to RecordScaledObjectPaused
5b1c230 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
renamed RecordScalerPaused to RecordScaledObjectPaused
b96f6d7 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik could you please review my PR as all e2e test passed successfully.
cc @JorTurFer @tomkerkhove

@geoffrey1330
Merge branch 'main' into feat/pause-metric
cc73ca0 
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
JorTurFer
JorTurFer approved these changes Oct 29, 2023
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!
@SpiritZhou , could you include this metric on your PR improving OTEL process once it's merged?

@JorTurFer
Copy link
Member

JorTurFer commented Oct 29, 2023 •
edited by github-actions bot
Loading

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

JorTurFer
JorTurFer reviewed Oct 29, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@geoffrey1330 @JorTurFer
Update CHANGELOG.md
8e21bd9 
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
@JorTurFer
Copy link
Member

/skip-e2e

zroubalik
zroubalik approved these changes Oct 30, 2023
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 d39c920 into kedacore:main Oct 30, 2023
20 checks passed
@geoffrey1330 geoffrey1330 mentioned this pull request Oct 30, 2023
Closed
@zroubalik zroubalik mentioned this pull request Nov 13, 2023
Closed
7 tasks
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
@geoffrey1330 @JorTurFer @toniiiik
add pause metric in prometheus for scaledobject (kedacore#5045)
66be99e 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: anton.lysina <alysina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SpiritZhou SpiritZhou SpiritZhou left review comments

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
skip-e2e
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@geoffrey1330 @JorTurFer @zroubalik @tomkerkhove @SpiritZhou @keda-automation