feat(metrics): add run latency to executor metrics #16190
Conversation
AlirieGray
force-pushed
the
tasks/query-interval-metric
branch
5 times, most recently
from
3a5bb81 to
47d6def
Compare
AlirieGray
force-pushed
the
tasks/query-interval-metric
branch
2 times, most recently
from
ea4da8f to
139dc88
Compare
There was a problem hiding this comment.
Really great work, @AlirieGray 👍 The improvements to consistency will really help future peers following along.
I only had a couple of very minor nits and you can merge it.
| ScheduledFor time.Time `json:"scheduledFor"` // ScheduledFor is the Now time used in the task's query | ||
| RunAt time.Time `json:"runAt"` // RunAt is the time the task is scheduled to be run, which is ScheduledFor + Offset |
There was a problem hiding this comment.
💯 Thank you for adding clarifying documentation so the next person will have an easier time understanding the fields
| schLogger.Info( | ||
| "error in scheduler run", | ||
| zap.String("taskID", platform.ID(taskID).String()), | ||
| zap.Time("scheduledAt", scheduledAt), | ||
| zap.Time("scheduledFor", scheduledFor), |
There was a problem hiding this comment.
Really great to see the consistency with scheduledFor and runAt 🎉
task/backend/scheduler.go
Outdated
| // time.Time for when the next run is due (includes offset) | ||
| RunAt time.Time | ||
|
|
There was a problem hiding this comment.
This is not necessary, as you'll be removing the RunCreation type with your cleanup PR
| @@ -22,10 +22,10 @@ type TaskControlService interface { | |||
| NextDueRun(ctx context.Context, taskID influxdb.ID) (int64, error) | |||
|
|
|||
| // CreateRun creates a run with a schedule for time. | |||
| // This differes from CreateNextRun in that it should not to use some scheduling system to determin when the run | |||
| // This differs from CreateNextRun in that it should not to use some scheduling system to determine when the run | |||
| Namespace: namespace, | ||
| Subsystem: subsystem, | ||
| Name: "schedule_interval", | ||
| Help: "The interval between the time the run was scheduled for and the time the task's next run is due at, by task type", |
There was a problem hiding this comment.
The name and wording of this metric is a little confusing. I would consider calling the field runLatency and the metric run_latency_seconds. Prometheus guidelines suggest adding the units (plural) to the end of the metric name.
The Help could the be something like:
Records the latency between the time a task was due to run and the time the task started execution, by task type
AlirieGray
force-pushed
the
tasks/query-interval-metric
branch
2 times, most recently
from
9ee7758 to
2d01100
Compare
AlirieGray
force-pushed
the
tasks/query-interval-metric
branch
from
2d01100 to
7711940
Compare
AlirieGray
changed the title
AlirieGray
force-pushed
the
tasks/query-interval-metric
branch
from
7711940 to
2ec452b
Compare
This PR adds a histogram metric
scheduleIntervalto the Executor metrics that records the time lag between when a task is scheduled to be run and when it actually runs. This can be used to determine the "per minute system concurrency" as described in the queryd SLO RFC.This PR also adds a property
RunAtto the Run object, which is used to record the time the Run is scheduled to be executed, including the task's Offset. This is added to the Run object because it is possible that the Run object will be changed by the user in the time between the Run being scheduled and being run, which would make our metrics for that Run inaccurate.The value
ScheduledAtis changed toScheduledForto maintain consistency.