-
Notifications
You must be signed in to change notification settings - Fork 176
Align workflow_task_schedule_to_start_latency #2669
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
base: master
Are you sure you want to change the base?
Align workflow_task_schedule_to_start_latency #2669
Conversation
.record(ProtobufTimeUtils.toM3Duration(startedTime, r.getScheduledTime())); | ||
.record(ProtobufTimeUtils.toM3Duration(r.getStartedTime(), r.getScheduledTime())); |
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.
Any idea of what the practical time difference could be for users that may be monitoring this metric? I guess we can just call it out in release notes
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.
clock skew between the Server and Worker, maybe some unaccounted for network time. The main issue is the async poller was measuring it differently then everywhere else which is a bug.
.record(ProtobufTimeUtils.toM3Duration(startedTime, r.getScheduledTime())); | ||
.record(ProtobufTimeUtils.toM3Duration(r.getStartedTime(), r.getScheduledTime())); |
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 see this same logic for activity, do we want to update that one to use the activity poll response start time too? I also saw this logic with Nexus, but no start time on its response (I also confirmed the activity and workflow started time fields on the response go as far back as Temporal, so we're safe for old server versions)
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.
ah good catch, fixed
3b581ea
to
b84aa7a
Compare
b84aa7a
to
647f690
Compare
Align how
workflow_task_schedule_to_start_latency
is measured across the Async and Multithreaded Pollersdk-java/temporal-sdk/src/main/java/io/temporal/internal/worker/WorkflowPollTask.java
Line 200 in 4631b16
Now both pollers will use the time from the task to calculate
workflow_task_schedule_to_start_latency
, this also aligns with Core and Go