fix maintenance_job_triggered metric to be published in all code paths #436
Conversation
| Attributes taskAttributes = | ||
| Attributes.of( | ||
| AttributeKey.stringKey(AppConstants.ENTITY_NAME), | ||
| metadata.getEntityName(), | ||
| AttributeKey.stringKey(AppConstants.ENTITY_TYPE), | ||
| metadata.getClass().getSimpleName().replace("Metadata", ""), | ||
| AttributeKey.stringKey(AppConstants.JOB_TYPE), | ||
| jobType.getValue()); | ||
| otelEmitter.count(METRICS_SCOPE, "maintenance_job_triggered", 1, taskAttributes); |
There was a problem hiding this comment.
Why not add these metrics in the OperationTask.submitJob that's where actual job submission happens and that is the triggering point. This is basically processing data and data preparation step. This is common for all the job type and some may not be relevant some job type which are not interested. This could also lead to more cardinality. We could also enable metrics emission for on specific job type going forward.
There was a problem hiding this comment.
Also I commented on the other PR and this metric may not be needed to start with. We are emitting the final status and that should capture the status of a triggered job. We can actually avoid this additional metric.
There was a problem hiding this comment.
In cases where the maintenance job is skipped, the operationTask is not submitted. This method simply returns an empty Task list without submitting the task. So, operationTask.submitJob is not called at all.
There was a problem hiding this comment.
maintenance_job_skipped + maintenance_job_completed should give the total number. We can submit maintenance_job_skipped to get skip count if needed. Skip maintenance is based on config or policy is not present etc.
There was a problem hiding this comment.
Ideally, yes. But there is one more code path where the reportJobStatus is not called. In this case, the maintenance_job_completed would not be registered. Given this, I feel like having all the 3 metrics makes debugging easier. Thoughts ?
There was a problem hiding this comment.
Empty state is rare failure scenario and skipped is returned for that. So we can report SKIPPED. That way we can avoid one additional metric. I am mainly concern on the cardinality and scrapping limit. We can incrementally add additional metrics by observing how it behaves.
2 participants
Summary
Fix maintenance_job_triggered metric so that it is emitted in all code paths.
Changes
Testing Done
Updating metrics, no additional tests changed/ added