-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add taskStatus dimension to service/heartbeat metric #17488
base: master
Are you sure you want to change the base?
Conversation
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.
Left some suggestions, similar to what had been suggested in an older PR #17268 (comment).
|Metric|Description| Dimensions |Normal value| | ||
|------|-----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------| |
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.
Please revert these formatting changes.
* @return The status of the task. Note: this interface method is unstable at this time. | ||
*/ | ||
@Nullable | ||
default String getStatus() |
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.
Rather than adding a getStatus()
method here, I would prefer it if we just cast to SeekableStreamTask
in CliPeon
.
The Task.run()
method runs the task and returns the final status.
We shouldn't have or need access to the status of the task while the task is running.
Moreover, the status being returned here is not the same as the task status.
@@ -308,6 +308,9 @@ public Supplier<Map<String, Object>> heartbeatDimensions(Task task) | |||
builder.put(DruidMetrics.DATASOURCE, task.getDataSource()); | |||
builder.put(DruidMetrics.TASK_TYPE, task.getType()); | |||
builder.put(DruidMetrics.GROUP_ID, task.getGroupId()); | |||
if (task.getStatus() != null) { | |||
builder.put(DruidMetrics.TASK_STATUS, task.getStatus()); |
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 don't think we should use this dimension as it corresponds to the final status of a task which only takes values SUCCESS
or FAILURE
. We could use DruidMetrics.STATUS
or some other dimension instead.
Description
Added a taskStatus dimension to the service/heartbeat metric. This can be used to detect cases when a particular task has been reporting its heartbeat in a particular state for an unusually long time; for example if a streaming task is stuck in paused state. Only tasks derived from SeekableStreamIndexTask report their taskStatus with this metric for now, other task types do not.
This PR has: