-
Notifications
You must be signed in to change notification settings - Fork 64
🌱 Add prometheus to e2e workflow #1928
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1928 +/- ##
==========================================
+ Coverage 69.21% 69.26% +0.04%
==========================================
Files 79 79
Lines 7037 7051 +14
==========================================
+ Hits 4871 4884 +13
Misses 1885 1885
- Partials 281 282 +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:
|
So, this adds a new e2e that isn't run downstream? There's no workflow to run it in upstream CI? |
Yes - it currently only adds prometheus to the environment when you run the new e2e target, so I've kept the workflow the same for now. If you'd prefer I'm happy to make that change here - it doesn't add much overhead to the e2e at all. |
6c32ce6
to
b51c814
Compare
2daa74b
to
e8b18c9
Compare
bb99737
to
b7e65a7
Compare
egress: | ||
- {} # Allows all egress traffic for metrics requests | ||
ingress: | ||
- {} # Allows us to query 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.
I understand that we need this NP to allow us to get data from Prometheus integrate it with Grafana and etc. So, I am OK with 👍
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.
Thank you for the nice work. 🎉
I understand that we will need to improve it in the long run and that is just teh first step for we have the targets and quick start.
So, I just added a nit: #1928 (comment) but otherwise I am OK with it. LGTM
We might also want to have a second look and help from @tmshort too
[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 |
I reran it and it passed, seems a test flaked 🫤 |
Adds prometheus to the test-e2e Makefile target, which stands up a barebones prometheus scraper to gather metrics from the operator-controller and catalogd pods during the e2e test run. When finished, the prometheus server is queried for a raw output of the metrics and stores it in metrics.out. These metrics will be analyzed in a later PR. Signed-off-by: Daniel Franz <[email protected]>
New changes are detected. LGTM label has been removed. |
Description
Closes #1902 and #1903
Adds prometheus to the
test-e2e
Makefile target, which stands up a barebones prometheus scraper to gather metrics from the operator-controller and catalogd pods during the e2e test run. When finished, the prometheus server is queried for a raw output of the metrics and stores it inARTIFACTS_PATH/metrics.out
. These metrics will be analyzed in a later PR.Reviewer Checklist