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

Expose prometheus metrics for ScaledJob resources #4913

Merged
merged 11 commits into from
Jan 15, 2024

Conversation

yoongon
Copy link
Contributor

@yoongon yoongon commented Aug 26, 2023

Provide a description of what has been changed

Checklist

Fixes #4798
Relates to kedacore/keda-docs#1263

@yoongon yoongon requested a review from a team as a code owner August 26, 2023 09:37
@github-actions
Copy link

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

🏖️ Over the summer, the response time will be longer than usual due to maintainers taking time off so please bear with us.

While you are waiting, make sure to:

Learn more about:

@yoongon
Copy link
Contributor Author

yoongon commented Aug 26, 2023

Hi @JorTurFer
#4817

The previous PR was broken, so I opened this new PR.
I am sorry but I couldn't roll it back.

Please take a look at this again.

Thanks.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Show resolved Hide resolved
@yoongon yoongon force-pushed the feature/prometheus-x branch 2 times, most recently from a24d627 to 84c81dc Compare September 6, 2023 13:01
@zroubalik zroubalik changed the title Expose prometheus metrics at ScaledJob like ScaledObject Expose prometheus metrics for ScaledJob resources Sep 6, 2023
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.

Could you please add e2e test for this feature? You can see existing tests for Prom metrics for ScaledObjects: https://github.com/kedacore/keda/blob/main/tests/sequential/prometheus_metrics/prometheus_metrics_test.go

@yoongon
Copy link
Contributor Author

yoongon commented Sep 25, 2023

Could you please add e2e test for this feature? You can see existing tests for Prom metrics for ScaledObjects: https://github.com/kedacore/keda/blob/main/tests/sequential/prometheus_metrics/prometheus_metrics_test.go

Hi @zroubalik I will update it together, once the above discussion is done.
Thanks for the suggestion.

@zroubalik
Copy link
Member

Could you please add e2e test for this feature? You can see existing tests for Prom metrics for ScaledObjects: https://github.com/kedacore/keda/blob/main/tests/sequential/prometheus_metrics/prometheus_metrics_test.go

Hi @zroubalik I will update it together, once the above discussion is done. Thanks for the suggestion.

@yoongon thanks, just FYI we plan to do release tomorrow (Wednesday)

@yoongon
Copy link
Contributor Author

yoongon commented Oct 31, 2023

Hi @zroubalik
I updated patch.
Cause I am not familiar with e2e, It takes time. However, I think we can move in parallel.
Could you please check this change first?

BTW, there are some change between this PR and master, I am rebasing it.
I will reupdate it.

Thanks

@yoongon yoongon force-pushed the feature/prometheus-x branch 3 times, most recently from 091abcd to f225e4f Compare November 1, 2023 13:50
@yoongon yoongon force-pushed the feature/prometheus-x branch 6 times, most recently from af4e86e to aa5d56c Compare November 6, 2023 11:28
@yoongon
Copy link
Contributor Author

yoongon commented Jan 9, 2024

@zroubalik Thanks for checking. I should have known this one. I closed that PR.
BTW, cause e2e build is not working properly on my local machine, it drags me down.
If you see any missing point on this PR, it will really be appreciated.
I am focusing on e2e now for more debugging.

@JorTurFer
Copy link
Member

I'm deploying the code into my cluster, I'll give you some feedback about the problem soon

@JorTurFer
Copy link
Member

You didn't register the metric in Prometheus server:
https://github.com/kedacore/keda/blob/main/pkg/metricscollector/prommetrics.go#L160-L177

That's why now it's not available

@JorTurFer
Copy link
Member

You probably need to add also a metric record for the jobs here:

metrics, isTriggerActive, _, err := cache.GetMetricsAndActivityForScaler(ctx, i, metricSpecs[0].External.Metric.Name)
if err != nil {

You can do it adding the line metricscollector.RecordScaledJobError(scaledJob.Namespace, scaledJob.Name, err)

Signed-off-by: Yoon Park <[email protected]>
@zroubalik
Copy link
Member

zroubalik commented Jan 12, 2024

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

@yoongon
Copy link
Contributor Author

yoongon commented Jan 12, 2024

@JorTurFer
Thank you so much for the checking.
TBH, I had registered it but i missed after rolled it back a few times. Eventually I forgot.
Thanks again and I've just updated it

@yoongon
Copy link
Contributor Author

yoongon commented Jan 12, 2024

image e2e failed here.

I changed metric name to the original one.
We did a discussion a lot, so I might be confusing to you.

BTW, if you have any concern about the metric name, please let me know again.

@JorTurFer
Copy link
Member

JorTurFer commented Jan 12, 2024

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

@yoongon
Copy link
Contributor Author

yoongon commented Jan 13, 2024

image

Some e2e errors appear to be related with time.Sleep between creating and deleting wrong scaled resource.
I guess it has been newly added.
I've just added this commit.
44ffcae

@JorTurFer
Copy link
Member

JorTurFer commented Jan 13, 2024

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

@yoongon
Copy link
Contributor Author

yoongon commented Jan 14, 2024

image Looks like final error on e2e. https://github.com//pull/4913/commits/75476df9115d6071234c970e904722577a9460c6

I think this commit will fix it. I must have missed it during the rebase.

@JorTurFer
Copy link
Member

JorTurFer commented Jan 14, 2024

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

@zroubalik zroubalik mentioned this pull request Jan 15, 2024
25 tasks
@JorTurFer JorTurFer merged commit b220384 into kedacore:main Jan 15, 2024
19 checks passed
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.

Implement Prometheus metrics for ScaledJob
6 participants