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

Update prometheus_metrics_test.go #4437

Merged
merged 6 commits into from
Apr 25, 2023

Conversation

Parthiba-Hazra
Copy link
Contributor

@Parthiba-Hazra Parthiba-Hazra commented Apr 5, 2023

write test cases for Prometheus Metrics e2e tests, covered this metrics

keda_scaler_errors
keda_scaler_error_totals
keda_scaled_object_errors
test cases check that Prometheus metrics are correctly incremented in case of an error in a scaler/scaledobject, etc.

Checklist

Fixes #4127

Relates to #4127

@Parthiba-Hazra Parthiba-Hazra requested a review from a team as a code owner April 5, 2023 16:59
@Parthiba-Hazra
Copy link
Contributor Author

Hey @JorTurFer , sorry for the trouble.
I recreate the PR with new branch, thank you.

@JorTurFer
Copy link
Member

JorTurFer commented Apr 5, 2023

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

@zroubalik
Copy link
Member

zroubalik commented Apr 6, 2023

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

@Parthiba-Hazra
Copy link
Contributor Author

Parthiba-Hazra commented Apr 6, 2023

Hey, I'm seeing that testScalerErrors function failing.
I tried running the tests many times in my local minikube cluster, it passed each time.
any suggestion?

@JorTurFer
Copy link
Member

JorTurFer commented Apr 10, 2023

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

@JorTurFer
Copy link
Member

JorTurFer commented Apr 24, 2023

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

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!
Could you add a record in the changelog linking the issue? The other section looks as the correct place for it

@JorTurFer
Copy link
Member

JorTurFer commented Apr 25, 2023

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

@JorTurFer JorTurFer enabled auto-merge (squash) April 25, 2023 08:14
@Parthiba-Hazra
Copy link
Contributor Author

Hey @JorTurFer should I mark as checked the all checklists in pr description?

@JorTurFer
Copy link
Member

Hey @JorTurFer should I mark as checked the all checklists in pr description?

I have marked them 😄

@JorTurFer JorTurFer merged commit 517ca0d into kedacore:main Apr 25, 2023
@JorTurFer
Copy link
Member

@Parthiba-Hazra , after the merge, the test is failing in the integration cluster, could you take a look? 🙏
https://github.com/kedacore/keda/actions/runs/4795974230/jobs/8533699499#step:6:38507

Comment on lines +365 to +366
time.Sleep(2 * time.Second)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to increase this time to ensure that the metric is exposed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I increase it from 2 to 4?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even bigger xD, we need to ensure that the test sin't flaky. I mean, in this case, waiting 2,4,10 seconds doesn't change the test because it's checking > 0, never mind if 1 or 100. If you think that 10 is safer, go with 10 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually its confusing, cause in my local cluster it's worked well. At least it should pass after 4 seconds, I will change it to 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I create a new PR on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please 🙏

JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request May 2, 2023
geoffrey1330 pushed a commit to geoffrey1330/keda that referenced this pull request Oct 4, 2023
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.

Create e2e tests for all exposed Prometheus metrics
3 participants