-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Signed-off-by: Parthiba hazra <parthibahazra@gmail.com>
Hey @JorTurFer , sorry for the trouble. |
/run-e2e prometheus |
/run-e2e prometheus |
Hey, I'm seeing that testScalerErrors function failing. |
/run-e2e prometheus |
Signed-off-by: Parthiba hazra <parthibahazra@gmail.com>
/run-e2e prometheus |
There was a problem hiding this 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
Signed-off-by: Parthiba hazra <parthibahazra@gmail.com>
/run-e2e prometheus |
Hey @JorTurFer should I mark as checked the all checklists in pr description? |
I have marked them 😄 |
@Parthiba-Hazra , after the merge, the test is failing in the integration cluster, could you take a look? 🙏 |
time.Sleep(2 * time.Second) | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please 🙏
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
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