Skip to content

fix maintenance_job_triggered metric to be published in all code paths #436

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.

Sign up for GitHub

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

Open
kamanavishnu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix maintenance_job_triggered metric to be published in all code paths #436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions ...park/src/main/java/com/linkedin/openhouse/jobs/scheduler/tasks/OperationTasksBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,6 @@ private List<OperationTask<?>> processMetadataList(
if (optionalOperationTask.isPresent()) {
taskList.add(optionalOperationTask.get());
}

// Publish entity metrics for triggered tasks
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);
}
return taskList;
}
Expand All @@ -193,16 +184,22 @@ public Optional<OperationTask<?>> processMetadata(
try {
OperationTask<?> task = taskFactory.create(metadata);
task.setOtelEmitter(otelEmitter);

// Publish entity metrics for triggered tasks
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);
Comment on lines +189 to +197
Copy link
Copy Markdown
Member

@abhisheknath2011 abhisheknath2011 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Member

@abhisheknath2011 abhisheknath2011 Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Collaborator Author

@kamanavishnu kamanavishnu Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Member

@abhisheknath2011 abhisheknath2011 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Collaborator Author

@kamanavishnu kamanavishnu Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?
Copy link
Copy Markdown
Member

@abhisheknath2011 abhisheknath2011 Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!task.shouldRun()) {
log.info("Skipping task {}", task);

// Publish entity metrics for skipped tasks
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), task.getType().getValue());
otelEmitter.count(METRICS_SCOPE, "maintenance_job_skipped", 1, taskAttributes);
return Optional.empty();
} else {
Expand Down