-
Notifications
You must be signed in to change notification settings - Fork 31
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
Adding monitoring orchestration in the operator to collect metrics for Telemetry. #932
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #932 +/- ##
=======================================
Coverage 15.83% 15.83%
=======================================
Files 108 108
Lines 19911 19911
=======================================
Hits 3153 3153
Misses 16479 16479
Partials 279 279
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -84,7 +85,7 @@ func Add(mgr manager.Manager) error { | |||
} | |||
|
|||
// Gather migration metrics | |||
recordMetrics(mgr.GetClient()) |
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.
since you removed this. did you check if now you need the permissions and monitor.yaml
within the operator(operator/config/prometheus/monitor.yaml
and within operator/config/rbac/leader_election_role.yaml
)?
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 didn't removed it, I just moved it to the monitor directory to have all the monitoring related things organized in the same place, but any way the yaml you referring is not related to the controller metrics (i'm not sure for what it used), but it configured on a different namespace and port. i'm creating the relevant file for the metric recording in the controller addition.
62d6b20
to
5670d8f
Compare
CI issue was fixed, CR changes in progress. |
Remove all the rules setup since it is redundant, and move all the monitoring orchestration under metrics. This should simplify the PR. It was tested on OpenShift to ensure all the metrics are exposed. |
6a1682e
to
2b01dc2
Compare
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
…nitor in the controller. Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
PR was rebased after merging #916. |
to collect data* |
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.
why do we let forklift-controller add the metrics-related resources rather than the operator?
Moved all the relevant resource creation for monitoring to be managed by the operator rather than the controller. |
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.
minor comment about phrasing inside, other than that lgtm
Signed-off-by: Bella Khizgiyaev <[email protected]>
Following the addition of metrics to forklift-controller, this PR adds monitoring orchestration to add the ability to collect data and expose it for OpenShift-Monitor. These changes include the addition of different resources such as
Service
, theServiceMonitor
, and various labels needed for the metrics collection.