Add recording rule to collect migrations data to telemetry. #1172
Conversation
Signed-off-by: Bella Khizgiyaev <[email protected]>
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1172 +/- ##
==========================================
- Coverage 15.71% 15.70% -0.02%
==========================================
Files 112 112
Lines 23052 23052
==========================================
- Hits 3623 3620 -3
- Misses 19142 19144 +2
- Partials 287 288 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
operator/roles/forkliftcontroller/templates/monitor/recordingrole-migrations.yml.j2
Show resolved
Hide resolved
|
@sradco do you mind giving this a brief look? |
Reviewed and suggested to @bkhizgiy to move the recording rule code under /pkg/monitoring/rules to have all observability related code in a dedicated, since in CNV we show that this brings value for maintenance and also for creating automatic docs generation for the metrics and recording rule. |
|
Adding @avlitman to also review this. |
|
@sradco It's not possible to move this code under /pkg/monitoring/ as you suggested, since this is the operator code and is being built using a different target. It should be located under the operator folder. Since we are building the monitoring stack differently from CNV (using Ansible vs. Go), it should be added as a template to the operator during its creation. We could add another folder under /templates/monitoring, but honestly, I don’t think it’s necessary. |
|
That was a suggestion, and if it doesn't fit your project, please feel free to ignore. |
| - record: cluster:mtv_migrations_status_total:max | ||
| expr: max by(status, provider, mode, target) (mtv_migrations_status_total) | ||
| labels: | ||
| app: {{ app_name }} |
There was a problem hiding this comment.
I would add also annotations
annotations:
description: "The maximum number of migration statuses, grouped by status, provider, mode, and target."
There was a problem hiding this comment.
sorry for delayed review was sick.. and nice work!
| - name: mtv-migrations | ||
| rules: | ||
| - record: cluster:mtv_migrations_status_total:max | ||
| expr: max by(status, provider, mode, target) (mtv_migrations_status_total) |
There was a problem hiding this comment.
are we sure we need all 4 labels? telemetry team is very strict about the number of records added per query. if we can aggregate by less it will be better.
There was a problem hiding this comment.
These labels are needed. It should be fine.
|
/lgtm |
Following the request of adding recording rule in openshift/cluster-monitoring-operator#2461