✨ API Call Alerts #2139
✨ API Call Alerts #2139
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
dtfranz
force-pushed
the
api-call-alerts
branch
from
5051cf6 to
ef52591
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2139 +/- ##
==========================================
+ Coverage 72.75% 72.77% +0.02%
==========================================
Files 79 79
Lines 7340 7340
==========================================
+ Hits 5340 5342 +2
+ Misses 1653 1652 -1
+ Partials 347 346 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dtfranz
force-pushed
the
api-call-alerts
branch
3 times, most recently
from
441eca5 to
ac2ecd6
Compare
| @@ -24,7 +25,7 @@ var ( | |||
| ) | |||
|
|
|||
| const ( | |||
| testSummaryOutputEnvVar = "GITHUB_STEP_SUMMARY" | |||
| testSummaryOutputEnvVar = "E2E_SUMMARY_OUTPUT" | |||
There was a problem hiding this comment.
Nice 👍
I think is better keep generic since we can run the same with or not GITHUB
Very cool
There was a problem hiding this comment.
Hi @dtfranz
Thank you a lot just remove the commented code and I think we are good to move forward
Do not fail e2e run when issues with summary generation are encountered. Add prometheus alerts for excessive API calls from operator-controller or catalogd, as well as summary graphs to match. Signed-off-by: Daniel Franz <[email protected]>
dtfranz
force-pushed
the
api-call-alerts
branch
from
ac2ecd6 to
6c168e2
Compare
| @@ -39,8 +40,18 @@ func TestMain(m *testing.M) { | |||
| utilruntime.Must(err) | |||
|
|
|||
| res := m.Run() | |||
| err = utils.PrintSummary(testSummaryOutputEnvVar) | |||
There was a problem hiding this comment.
I was looking and seem that should be skiped already see:
https://github.com/operator-framework/operator-controller/blob/main/test/utils/summary.go#L175-L198
Then, why we just call it here and not in all e2e tests?
Could you help me understand how it works?
There was a problem hiding this comment.
I wanted to keep the mechanism of selecting output destination distinct in summary.go and e2e_suite_test.go. For PrintSummary() you supply a file path, and if that's empty then we warn and skip, since that's an invalid use of the func. In e2e_suite_test.go you supply env, and if that's empty then we skip the summary with a note, because it is totally valid to run e2e without summary. This allowed me to keep the e2e test runner as simple as possible and follow the convention of transparently supplying e2e arguments via env. But I probably overthought it 🤷
There was a problem hiding this comment.
But each test has one suite right:
- https://github.com/operator-framework/operator-controller/blob/main/test/experimental-e2e/experimental_e2e_test.go#L45-L56
- https://github.com/operator-framework/operator-controller/blob/main/test/upgrade-e2e/upgrade_e2e_suite_test.go#L30-L60
- https://github.com/operator-framework/operator-controller/blob/main/test/extension-developer-e2e/extension_developer_test.go#L24-L214
So, why we use it only in this one?
There was a problem hiding this comment.
The summary is also generated for experimental e2e, you can see it in here. The upgrade and extension developer tests are pretty pointless to run prometheus on because they only have about a few seconds of actual runtime.
Description
Adds prometheus alerts for excessive API calls from operator-controller or catalogd, as well as summary graphs to match.
Do not fail e2e run when issues with summary generation are encountered or when the output isn't specified.
Reviewer Checklist